Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable Performance/ChainArrayAllocation cop #8404

Merged
merged 5 commits into from
Sep 30, 2020
Merged

Enable Performance/ChainArrayAllocation cop #8404

merged 5 commits into from
Sep 30, 2020

Conversation

ashmaroli
Copy link
Member

Summary

RuboCop Performance has a cop that allows detecting method chains that duplicate starting array with each chain segment.
But the cop is disabled by default.

This pull request enables the cop and fixes offenses reported by it by switching non-mutating methods with mutating counterparts.

@jekyll jekyll deleted a comment from RamboCacing666 Sep 20, 2020
DirtyF
DirtyF approved these changes Sep 20, 2020
View reviewed changes
Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass, looks like it's safe to mutate in those methods.
The gain is hard to notice in our test suite.

@ashmaroli
Copy link
Member Author

ashmaroli commented Sep 21, 2020 •
edited

Thanks for the review @DirtyF :)
When you say The gain is hard to notice.., if you meant speed-wise, then yeah, this is a micro-optimization. Any speed gain will be shadowed by a drain elsewhere. However, if you meant allocation-wise, our docs site is a small sample. The third-party repo workflow shows an approx. gain of 2MB.

That said, I'll wait to see if @parkr or @mattr- sees something we missed or overlooked

parkr
parkr reviewed Sep 23, 2020
View reviewed changes
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good! I left a couple comments about some optimizations we might be able to make here.

lib/jekyll/drops/drop.rb Show resolved Hide resolved
lib/jekyll/filters/url_filters.rb Show resolved Hide resolved
lib/jekyll/page.rb Outdated Show resolved Hide resolved
lib/jekyll/renderer.rb Outdated Show resolved Hide resolved
@ashmaroli ashmaroli mentioned this pull request Sep 24, 2020
Merged
@ashmaroli ashmaroli added this to the 4.2 milestone Sep 24, 2020
@ashmaroli ashmaroli marked this pull request as draft September 25, 2020 04:56
@ashmaroli
Copy link
Member Author

Requires #8408 to be reviewed and merged first...

@ashmaroli ashmaroli marked this pull request as ready for review September 30, 2020 06:46
@ashmaroli
Copy link
Member Author

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit f7292ec into jekyll:master Sep 30, 2020
jekyllbot added a commit that referenced this pull request Sep 30, 2020
@jekyllbot
Update history to reflect merge of #8404 [ci skip]
b17cb5f
@ashmaroli ashmaroli deleted the enable-rubocop-perf-cop branch September 30, 2020 06:49
This was referenced Mar 12, 2021
Closed
Merged
Merged
Merged
Merged
@jekyll jekyll locked and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DirtyF DirtyF DirtyF approved these changes

@parkr parkr parkr approved these changes

@mattr- mattr- Awaiting requested review from mattr-

Assignees
No one assigned
Labels
bug fix frozen-due-to-age
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ashmaroli @DirtyF @parkr @jekyllbot