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.

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

Don’t allocate array on no args #33817

Merged
merged 1 commit into from Sep 8, 2018

Conversation

Projects
None yet
3 participants
@schneems
Copy link
Member

schneems commented Sep 7, 2018

When no dependencies are present to be digested there is no reason to build an array just to turn around and turn it back into a string.

The dependencies array is not mutated in this method so we can use the same empty array across all invocations.

Total allocated: 791402 bytes (7294 objects)
Total allocated: 777442 bytes (7132 objects)

(791402 - 777442) / 791402.0 # => 1.76 % speed improvement

actionview/lib/action_view/digestor.rb Outdated
def digest(name:, finder:, dependencies: [])
dependencies ||= []
cache_key = [ name, finder.rendered_format, dependencies ].flatten.compact.join(".")
def digest(name:, finder:, dependencies: EMPTY_ARRAY)

This comment has been minimized.

@kamipo

kamipo Sep 7, 2018

Member

How about just using default nil for dependencies?

This comment has been minimized.

@schneems

schneems Sep 7, 2018

Author Member

Seems good. Thanks.

@schneems schneems force-pushed the schneems:schneems/dig-simple branch 3 times, most recently from 6699fa9 to 2c6b732 Sep 7, 2018

Don’t allocate array on no args
When no dependencies are present to be digested there is no reason to build an array just to turn around and turn it back into a string.

The dependencies array is not mutated in this method so we can use the same empty array across all invocations.

Total allocated: 791402 bytes (7294 objects)
Total allocated: 777442 bytes (7132 objects)

(791402 - 777442) / 791402.0 # => 1.76 % speed improvement

@schneems schneems force-pushed the schneems:schneems/dig-simple branch from 2c6b732 to 1bd578f Sep 7, 2018

@schneems

This comment has been minimized.

Copy link
Member Author

schneems commented Sep 7, 2018

This is ready to go

@schneems schneems merged commit 273e73d into rails:master Sep 8, 2018

1 of 2 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
if dependencies.nil? || dependencies.empty?
cache_key = "#{name}.#{finder.rendered_format}"
else
cache_key = [ name, finder.rendered_format, dependencies ].flatten.compact.join(".")

This comment has been minimized.

@matthewd

matthewd Sep 8, 2018

Member

Can the dependencies array contain nested arrays and/or nils? The loop below suggests not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.