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 cache on to the DetailsKey object #23756

Merged
merged 1 commit into from
Feb 18, 2016
Merged

move digest cache on to the DetailsKey object #23756

merged 1 commit into from
Feb 18, 2016

Conversation

tenderlove
Copy link
Member

This moves digest calculation cache on to the details key object.
Before, the digest cache was a class level ivar, and one of the keys was
the hash value of the details key object:

cache_key = ([ name, finder.details_key.hash ].compact + Array.wrap(options[:dependencies])).join('.')

An object's hash value is not unique, so it's possible for this cache
key to produce colliding keys with no resolution. This commit move
cache on to the details key object itself, so we know that the digests
are always unique per details key object.

This moves digest calculation cache on to the details key object.
Before, the digest cache was a class level ivar, and one of the keys was
the hash value of the details key object:

  https://github.com/rails/rails/blob/13c4cc3b5aea02716b7459c0da641438077f5236/actionview/lib/action_view/digestor.rb#L28

An object's hash value is not unique, so it's possible for this cache
key to produce colliding keys with no resolution.  This commit move
cache on to the details key object itself, so we know that the digests
are always unique per details key object.
@rafaelfranca
Copy link
Member

LGTM

@tenderlove
Copy link
Member Author

I don't like some of the code left over in the Digestor, but I wanted a gut check on the cache move. I think we can refactor this to be a bit more nice later.

@jeremy
Copy link
Member

jeremy commented Feb 18, 2016

Dig it. Nice simplification for the cache key. Would move the middleware along with the refactoring later.

tenderlove added a commit that referenced this pull request Feb 18, 2016
move digest cache on to the DetailsKey object
@tenderlove tenderlove merged commit 3bd9fe1 into rails:master Feb 18, 2016

def self.digest_caches
@details_keys.values.map(&:digest_cache)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't see this used in lib. Was this just for the tests? 😁

@kaspth
Copy link
Contributor

kaspth commented Feb 18, 2016

Nice ❤️

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

Successfully merging this pull request may close these issues.

None yet

5 participants