Skip to content

Enable Performance/ChainArrayAllocation cop for import/export module

Matthias Käppler requested to merge 209918-enable-chain-alloc-cop into master

Description of the proposal

Closes #209918 (closed)

The Performance/ChainArrayAllocation cop is disabled by default. However, making copies of collections in memory that can grow large can be quite costly, and it has bitten us before in those areas of the code base that are more batch-y in nature (such as import/export.)

I have enabled the cop for that module only for now, and fixed 3 occurrences where we were creating temporary copies of the data structures we were processing. Incidentally they were all the same: calling compact after a transformation via map. I used the tap { |this| this.compact! } idiom to replace any plain compact, which will not create an additional copy but rather mutate the previous copy that is already created by map.*

*note that I was not able to get rid of the map calls, since there is no map! on Hash, and trying to mutate the collections manually resulted in test failures that looked legit, so I left it at just removing the extraneous compact copy.

Check-list

  • Make sure this MR enables a static analysis check rule for new usage but ignores current offenses
  • Mention this proposal in the relevant Slack channels (e.g. #development, #backend, #frontend)
  • [-] If there is a choice to make between two potential styles, set up an emoji vote in the MR:
    • CHOICE_A: 🅰
    • CHOICE_B: 🅱
    • Vote yourself for both choices so that people know these are the choices
  • The MR doesn't have significant objections, and is getting a majority of 👍 vs 👎 (remember that we don't need to reach a consensus)
  • [-] (If applicable) One style is getting a majority of vote (compared to the other choice)
  • [-] (If applicable) Update the MR with the chosen style
  • [-] Create a follow-up issue to fix the current offenses as a separate iteration: ISSUE_LINK
  • Follow the review process as usual
  • Once approved and merged by a maintainer, mention it again:
    • In the relevant Slack channels (e.g. #development, #backend, #frontend)
    • (Optional depending on the impact of the change) In the Engineering Week in Review

/cc @gitlab-org/maintainers/rails-backend

Edited by 🤖 GitLab Bot 🤖

Merge request reports