Refactor how details are handled for template selection#56
Open
etiennebarrie wants to merge 8 commits into
Open
Refactor how details are handled for template selection#56etiennebarrie wants to merge 8 commits into
etiennebarrie wants to merge 8 commits into
Conversation
Template lookup threaded a details_key alongside the details hash: an interned TemplateDetails::Requested computed by LookupContext::DetailsKey when caching was on, nil when off. In the resolver that key did two things -- stand in as the requested-details object for matching, and, by being non-nil, select the persistent @unbound_templates cache over a throwaway one. It was never part of the cache key itself, which is just the virtual path; the interning bought the resolver nothing. Pass the @cache boolean through find/find!/find_all/exists? instead. The resolver builds Requested from the details hash directly and picks its cache by the flag. LookupContext's own details are always valid formats (formats= validates), so dropping the DetailsKey formats normalization on this path is a no-op; it still guards the digest cache key, which keeps using DetailsKey for now. Removes the now-unused LookupContext#details_key.
The requested template details -- the four registered axes locale, formats, variants and handlers -- were held in a plain Hash, with the accessors generated by register_detail writing into it and a default_procs registry supplying defaults. LookupContext mixed in the generated Accessors plus a DetailsCache module, and reached into @details[:key] from its own overrides. Replace the Hash with a small mutable value object, LookupContext::Details, that owns the four axes, their lazy defaults, and the formats normalization (expanding "*/*", validating symbols, the :js -> :html fallback) that used to live in LookupContext#formats=. Each accessor sets its own instance variable directly. LookupContext delegates the registered accessors to it and keeps only the locale (symbol vs array) and I18n-config special cases. This is behaviour-preserving: details are still threaded downstream as a Hash via Details#to_h, so the resolver, digest cache and lookup path are unchanged. register_detail / default_procs / the generated Accessors module are dropped in favour of the explicit REGISTERED_DETAILS list.
Template matching went through TemplateDetails::Requested: the resolver built one from the details hash, then asked each candidate template whether it matched? it and, if several did, computed a sort_key_for each. Ranking lived on the template's own details, keyed off precomputed index hashes on Requested. Move ranking onto the requested Details with #template_rank, which returns the per-axis index tuple ([format, locale, variant, handler]) used to order matches, or nil when a template doesn't match. The Details object now flows through find/find_all unchanged -- LookupContext#detail_args_for returns it directly instead of a hash -- and the resolver ranks against it. This also lets lookup ask for the single best match without materializing the whole candidate list: add Resolver#find / PathSet#find that rank-and-keep-best (find_best_by_details) and short-circuit per resolver, which is what the non-raising #find introduced earlier now uses. TemplateDetails#matches? and #sort_key_for are gone.
The per-details digest cache was keyed by the interned TemplateDetails::Requested that DetailsKey.details_cache_key returns: equal detail hashes were folded to a shared Requested object so they'd hash-equal as a cache key. Have Details produce the key itself with #to_cache_key -- a frozen, value-equal Array snapshot of the resolved axes (with formats normalized, which is the only thing the interner's normalization was still doing) -- and key the digest cache by that. Value-equal Arrays already hash-equal, so the interning is no longer needed to share a bucket, and the mutable Details object never has to double as a Hash key. Details#to_h, now only used by #inspect, is inlined there.
The process-wide store of per-details digest buckets lived on LookupContext::DetailsKey (@digest_cache, plus digest_cache and digest_caches), even though every reader goes through Digestor.digest and the node digests. Move it next to that code as Digestor.cache / Digestor.digest_caches / Digestor.clear_cache. Details#digest_cache now interns its bucket through Digestor.cache(to_cache_key), and DetailsKey.clear resets it via Digestor.clear_cache. DetailsKey is left holding only the Requested interner and clear.
With ranking moved onto Details#template_rank and the digest cache keyed by a frozen Array, nothing builds a TemplateDetails::Requested or its precomputed index hashes anymore, and nothing looks up DetailsKey.details_cache_key. The @details_keys interner that handed out shared Requested instances is dead. Delete the Requested class and the interner. DetailsKey is left holding only clear.
DetailsKey existed to intern detail hashes and hold the digest cache; both are gone. All it had left was clear, which resets the resolver caches, the shared view-context class and the digest cache. Fold that into LookupContext.clear and delete DetailsKey. Callers that reached for ActionView::LookupContext::DetailsKey.clear now call ActionView::LookupContext.clear.
The renderers turned the render options into a details hash with AbstractRenderer#extract_details (filtering to the registered keys and Array-wrapping each), then handed that hash to LookupContext, which merged it onto its own details on every lookup. Have the renderers ask the LookupContext for a resolved Details up front: LookupContext#details_for is now public and, besides the no-options and overrides cases, returns an already-resolved Details untouched so a renderer's @details flows straight back through find/find_all without being re-merged. TemplateRenderer#resolve_layout builds its format-scoped lookups with Details#merge. extract_details / NO_DETAILS are gone. find/find!/find_all default options to nil, and PathSet#exists? short-circuits on the first matching resolver rather than gathering all matches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently template lookup uses a ranking which interns ActionView::LookupContext::DetailsKey objects which hold hashes with index for faster comparison/ranking of templates and their details against the incoming request's details.
Since shared mutable caches like this are annoying for Ractors, I've looked into refactoring this to avoid the interning/caching, relying on rails#57783 to avoid having to build a rank for all templates before selecting the best one.