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

Only de-dup/cache FileSystemResolvers in ViewPaths #47786

Merged
merged 3 commits into from
Mar 30, 2023

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Mar 27, 2023

Fixes #47449

Previously I attempted to de-dup and cache any FileSystemResolver under the same path. This created issues because each Resolver caches its templates. Those cached templates only get compiled once into whatever compiled_method_container is passed on its first render. So separate compiled_method_containers must have different copies of a Template, and therefore separate copies of each Resolvers.

I tried a few approaches where I would have a separate cache of resolvers per-compiled-method-container, but I wasn't really successful as in most of the places we performed the casting we did not know or have access to the view context class and therefore compiled method container (it isn't stored in the LookupContext or ViewPath) and trying to add that relating created incompatibilities in some not-quite-public-but-still-widely used APIs for using ActionView directly.

This approach instead limits where we perform the de-dup and cache, to only descendants of ActionView::ViewPaths. These can be assumed to also inherit from ActionView::Rendering (I think ActionController::API is an exception here, which should be addressed in a future refactor) which means that they should all share the same compiled_method_container and therefore can all be cached safely together.

This should continue to ensure we don't leak memory from {append,prepend}_view_path at runtime, while maintaining compatibility with some of the more specialized uses of ActionView::Base as a standalone API. However this leaves open the possibility (which has always existed) of using that standalone API in a "leaky" way.

@rails-bot rails-bot bot added the actionview label Mar 27, 2023
@rails-bot rails-bot bot added the docs label Mar 27, 2023
@jhawthorn jhawthorn marked this pull request as ready for review March 28, 2023 23:12
@jhawthorn
Copy link
Member Author

@rafaelfranca updated this to avoid as many rebuilds of the watcher as possible. Hopefully that addresses the performance regression you saw.

@rafaelfranca
Copy link
Member

I'm not sure I got how this decrease the number of times the file watcher is initialized when a new engine path is added. prepend_view_path is always called with an array of strings, and if I got this code correctly, cast_file_system_resolvers would still be called for each one of engines. Am I missing some other cache layer?

@jhawthorn
Copy link
Member Author

I see. I was hoping that your view paths would be appended as an array. I suppose that doesn't block this (as it fixes the other issue) but may not solve the regression you saw.

@rafaelfranca
Copy link
Member

The view paths are appended by this code here

initializer :add_view_paths do
views = paths["app/views"].existent
unless views.empty?
ActiveSupport.on_load(:action_controller) { prepend_view_path(views) if respond_to?(:prepend_view_path) }
ActiveSupport.on_load(:action_mailer) { prepend_view_path(views) }
end
end

That doesn't block this fix for sure.

jhawthorn and others added 3 commits March 30, 2023 13:51
This was cached in a single ivar so passing any different klass would
still return the same instance.
Previously I attempted to de-dup and cache any FileSystemResolver under
the same path. This created issues because each Resolver caches its
templates. Those cached templates only get compiled once into whatever
compiled_method_container is passed on its first render. So separate
compiled_method_containers must have different copies of a Template, and
therefore separate copies of each Resolvers.

I tried a few approaches where I would have a separate cache of
resolvers per-compiled-method-container, but I wasn't really successful
as in most of the places we performed the casting we did not know or
have access to the view context class and therefore compiled method
container (it isn't stored in the LookupContext or ViewPath) and trying
to add that relating created incompatibilities in some
not-quite-public-but-still-widely used APIs for using ActionView
directly.

This approach instead limits where we perform the de-dup and cache, to
only descendants of ActionView::ViewPaths. These can be assumed to also
inherit from ActionView::Rendering (I think ActionController::API is an
exception here, which should be addressed in a future refactor) which
means that they should all share the same compiled_method_container and
therefore can all be cached safely together.

This should continue to ensure we don't leak memory from
{append,prepend}_view_path at runtime, while maintaining compatibility
with some of the more specialized uses of ActionView::Base as a
standalone API. However this leaves open the possibility (which has
always existed) of using that standalone API in a "leaky" way.

* Invoke callback only once building view Resolvers

Previously we would run the callback after each resolver built. We only
need to possibly recreate the watcher once all new resolvers have been
instantiated.

Additionally this now only invokes the callback when a new resolver has
actually been built, if one was found in the cache no work is needed.

Co-authored-by: Rafael Mendonça França <rafael@franca.dev>
@jhawthorn jhawthorn merged commit 08a18c7 into rails:main Mar 30, 2023
@jhawthorn jhawthorn deleted the view_caching_attempt4 branch March 30, 2023 22:57
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.

ActionView::Template::Error: undefined method after recent ActiveView changes
2 participants