Cache ActionView::FixtureResolvers created by stub_template #1979
Conversation
I'd like to add caching to this method to avoid a performance problem caused by repeated compilation of stubbed template contents. Since there aren't currently any unit tests around it, this adds a simple spec to make sure basic behaviour is preserved.
| module StubResolverCache | ||
| def self.resolver_for(hash) | ||
| @resolvers ||= {} | ||
| @resolvers[hash] ||= ActionView::FixtureResolver.new(hash) |
There was a problem hiding this comment.
My only concern is that this introduces shared state across tests (I know this is by design) but if we were to freeze this object it wouldn't be mutable which would alleviate this concern
There was a problem hiding this comment.
Sure, that's a straightforward change to make, have pushed with the extra .freeze. 👍
However, there's some further mutable internal state in the resolver that I don't think we'll be able to freeze; it only builds and caches Template instances when it receives queries for a specific path, so this happens on first render, not when we're instantiating the resolver.
My intuition is that this shared state should be safe, though. Since we're indexing the cache on the full stub definition (i.e. filename and stubbed contents), and the output of a compiled template should be a function of its contents, I don't think there's any way for the cache to return incorrect values.
urbanautomaton
force-pushed
the
cache-stub-template-resolvers
branch
from
fabcd51 to
309885a
Compare
|
Thanks for the feedback @JonRowe - I've made the requested change. I also got bored and wrote a small repro that constructs a pathological case for the performance problem the caching addresses. The caching turns a 4 minute run into a 13s one, which is nice. 😄 |
|
Ah; it seems that freezing the On the grounds that freezing it doesn't actually prevent its internal state from being mutated anyway, I'd be tempted to argue that it's okay to leave it unfrozen, for the reasons expressed earlier. By way of comparison, the |
|
My hesitation is I have no idea if there will be any unintended consequences for allowing it to be mutated, I believe our |
The `#stub_template` view spec helper works by prepending an ActionView::FixtureResolver[1] to the view instance's view_paths. This intercepts queries for specific template files, providing a canned response that's used to instantiate a dummy ActionView::Template. These templates still require compilation and finalization[2], however. As a new resolver is created for each stub in every example, this means that a single `stub_template` call in a `before` block that covers 20 examples will result in the same template being compiled and finalized 20 times. Both compilation and finalization can be expensive, so to avoid this where possible, this change adds a global resolver cache for stubbed templates. [1] https://github.com/rails/rails/blob/9f65d2a08bc80a94bbb2c0b6e00957c7059aed25/actionview/lib/action_view/testing/resolvers.rb#L6-L9 [2] https://github.com/rails/rails/blob/9f65d2a08bc80a94bbb2c0b6e00957c7059aed25/actionview/lib/action_view/template.rb#L118-L126
urbanautomaton
force-pushed
the
cache-stub-template-resolvers
branch
from
309885a to
3666c35
Compare
|
I definitely share your concerns; it'd be nice to have more confidence than just a code reading to assure us this is safe. From the code reading perspective: the only significant internal state to both So I'm pretty confident it's okay, but that's obviously not the same as "there can't be a problem". I've monkey-patched this change into the app I'm currently working on. While this wouldn't conclusively prove the absence of problems either, would it be worth sitting on this PR for a week or so to see if any issues crop up in our heavily-trafficked build? |
|
Okeydoke - thanks very much! ❤️ |
[skip-ci]
The
#stub_templateview spec helper works by prepending anActionView::FixtureResolverto the view instance'sview_paths. This intercepts queries for specific template files, providing a canned response that's used to instantiate a dummyActionView::Template.These canned responses still require compilation and finalization, however. As the resolver holds all the references to
ActionView::Templateinstances, and a new resolver is created for each stub in every example, this means that a singlestub_templatecall in abeforeblock covering 20 examples will result in the same template being compiled and finalized 20 times.Both compilation and finalization can be expensive. To avoid this expense where possible, this change adds a global resolver cache for stubbed templates.
By way of anecdote ('cos everyone loves those), this dropped our ~4k example view spec suite with ~500
stub_templatecalls from 6 min to 2 min total runtime.If there's anything you'd like done differently, or if there are any negative consequences of this change I haven't thought of, just let me know; I'll be happy to update the PR accordingly.
Cheers,
Simon
edit: ugh, the only build job that failed looks transient...