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

Move digest path calculation out of loop #33821

Merged
merged 1 commit into from Sep 11, 2018

Conversation

@schneems
Copy link
Member

@schneems schneems commented Sep 8, 2018

On every iteration of generating a cache for a collection a “digest path” is calculated even though it’s exactly the same for every element.

This PR exposes a method digest_path_from_virtual that returns back a “digest_path”. This can, in turn, be passed back into cache_fragment_name. This not only does less work but it also (you guessed it) uses less memory.

before: Total allocated: 762539 bytes (7035 objects)
after: Total allocated: 743590 bytes (6621 objects)

(762539 - 743590)/ 762539.0 # => 2.4% faster ⚡️⚡️

@schneems schneems force-pushed the schneems:schneems/digester-no-array branch from bac4494 to 9590d2c Sep 8, 2018
actionview/lib/action_view/helpers/cache_helper.rb Outdated Show resolved Hide resolved
private
def digest_path_from_virtual(virtual_path)
digest = Digestor.digest(name: virtual_path, finder: lookup_context, dependencies: view_cache_dependencies)
return "#{virtual_path}:#{digest}" if digest.present?

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 10, 2018
Member

if/else here? Both branches are likely to happen so it doesn't make sense to have special treatment to one, and empty line before the conditional.

name = controller.url_for(name).split("://").last if name.is_a?(Hash)
def fragment_name_with_digest(name, virtual_path, digest_path)
return name if !virtual_path && !digest_path
name = controller.url_for(name).split("://").last if name.is_a?(Hash)

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 10, 2018
Member

Empty line after the conditional

@schneems schneems force-pushed the schneems:schneems/digester-no-array branch 2 times, most recently from 724d33f to 4d8133b Sep 10, 2018
@schneems
Copy link
Member Author

@schneems schneems commented Sep 11, 2018

Fixed my weird rebase issue.

Copy link
Member

@jeremy jeremy left a comment

Nice savings! 💸

def digest_path
@digest_path ||= @view.send(:digest_path_from_virtual, @template.virtual_path)
end

This comment has been minimized.

@jeremy

jeremy Sep 11, 2018
Member

Perhaps should be public nodoc API?

This comment has been minimized.

@schneems

schneems Sep 11, 2018
Author Member

👍

end
end

def fragment_name_with_digest(name, virtual_path, digest_path)
return name if !virtual_path && !digest_path

This comment has been minimized.

@jeremy

jeremy Sep 11, 2018
Member

Do we have cases of nil virtual path but present digest path?

This comment has been minimized.

@jeremy

jeremy Sep 11, 2018
Member

I'd tend to a conditional expression over a guard clause here too.

This comment has been minimized.

@schneems

schneems Sep 11, 2018
Author Member

Do we have cases of nil virtual path but present digest path?

We don't but since i'm modifying the method signature of a public method cache_fragment_name other people can in theory end up here with a nil virtual path but a present digest_path.

I'd tend to a conditional expression over a guard clause here too.

Sounds good.

def fragment_name_with_digest(name, virtual_path, digest_path)
return name if !virtual_path && !digest_path

name = controller.url_for(name).split("://").last if name.is_a?(Hash)

This comment has been minimized.

@jeremy

jeremy Sep 11, 2018
Member

Worth extracting to a utility method to normalize name?

This comment has been minimized.

@schneems

schneems Sep 11, 2018
Author Member

I would like to leave it as is for now. Can pretty it up in another PR.

if skip_digest
name
else
fragment_name_with_digest(name, virtual_path)
virtual_path ||= @virtual_path

This comment has been minimized.

@jeremy

jeremy Sep 11, 2018
Member

Feels like this should still be the called method's responsibility, particularly if it's internal-public API.

This comment has been minimized.

@schneems

schneems Sep 11, 2018
Author Member

Moving into the sub-method

@schneems schneems force-pushed the schneems:schneems/digester-no-array branch from 4d8133b to f511661 Sep 11, 2018
@schneems
Copy link
Member Author

@schneems schneems commented Sep 11, 2018

Updated based on comments

On every iteration of generating a cache for a collection a “digest path” is calculated even though it’s exactly the same for every element.

This PR exposes a method `digest_path_from_virtual` that returns back a “digest_path”. This can in turn be passed back into `cache_fragment_name`. This not only does less work, but it also (you guessed it) uses  less memory.

before: Total allocated: 762539 bytes (7035 objects)
after: Total allocated: 743590 bytes (6621 objects) 


(762539 - 743590)/ 762539.0 # => 2.4% faster ⚡️⚡️
@schneems schneems force-pushed the schneems:schneems/digester-no-array branch from f511661 to 99d2602 Sep 11, 2018
@schneems schneems merged commit 9e24603 into rails:master Sep 11, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sriedel
Copy link

@sriedel sriedel commented Sep 19, 2018

I'm confused about the presented statistic: Total allocation for after is larger than for before - so wouldn't it use more memory despite allocating less objects?

Also, how do you get a speed increase from multiplying and dividing bytes allocated? They way I'm reading it, it isn't 2.4% faster - it's allocating 2.4% more memory.

What am I missing?

@tomca32
Copy link

@tomca32 tomca32 commented Sep 20, 2018

Are the before and after memory stats flipped in the PR description? The way it's presented it seems that it's using more memory after the change

before: Total allocated: 743590 bytes (7035 objects)
after: Total allocated: 762539 bytes (6621 objects)

after is larger than before

@schneems
Copy link
Member Author

@schneems schneems commented Sep 20, 2018

Correct I flipped the bytes when making the PR.

I just re-ran the calculation and this is what I get

Before:

⛄  2.5.1 🚀  ~/Documents/projects/codetriage (schneems/6.ohhh)
$ bundle exec derailed exec perf:objects | grep Total
/Users/rschneeman/.gem/ruby/2.5.1/gems/will_paginate-3.1.0/lib/will_paginate/view_helpers/link_renderer.rb:27: warning: constant ::Fixnum is deprecated
/Users/rschneeman/.gem/ruby/2.5.1/gems/will_paginate-3.1.0/lib/will_paginate/view_helpers/link_renderer.rb:91: warning: constant ::Fixnum is deprecated
Total allocated: 760595 bytes (7037 objects)
Total retained:  18789 bytes (166 objects)

After:

⛄  2.5.1 🚀  ~/Documents/projects/codetriage (schneems/6.ohhh)
$ bundle exec derailed exec perf:objects | grep Total
/Users/rschneeman/.gem/ruby/2.5.1/gems/will_paginate-3.1.0/lib/will_paginate/view_helpers/link_renderer.rb:27: warning: constant ::Fixnum is deprecated
/Users/rschneeman/.gem/ruby/2.5.1/gems/will_paginate-3.1.0/lib/will_paginate/view_helpers/link_renderer.rb:91: warning: constant ::Fixnum is deprecated
Total allocated: 741698 bytes (6646 objects)
Total retained:  18804 bytes (165 objects)
@tomca32
Copy link

@tomca32 tomca32 commented Sep 20, 2018

👍 Thanks for taking the time to explain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.