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 compiled ERB to an AV::Base subclass #35036
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice approach.
How will devs writing new tests know to use with_empty_template_cache
or to clear other caches beforehand?
@@ -53,28 +54,6 @@ module Accessors #:nodoc: | |||
register_detail(:variants) { [] } | |||
register_detail(:handlers) { Template::Handlers.extensions } | |||
|
|||
class DetailsKey #:nodoc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing a decent amount of code (mostly vendored Rails; hard to exclude) that references this class directly: https://github.com/search?l=Ruby&q=detailskey&type=Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only been able to find vendored Rails copies referencing this. It's been :nodoc:d forever. Do you think it's worthwhile to maintain the API? I only moved the constant because of require order, but my plan is to move the resolver cache inside this class too (possibly those view path caches too)
@view = ::ActionView::Base.new(ActionController::Base.view_paths, {}) | ||
view_paths = ActionController::Base.view_paths | ||
view_paths.each(&:clear_cache) | ||
ActionView::LookupContext.fallbacks.each(&:clear_cache) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this "clear view path and fallback caches" boilerplate repeated. Worth wrapping up so we have one spot to ask AC to start fresh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, agree. I've kept the repetition because I don't have a clear idea on what all the caches are (also, why aren't fallbacks
part of view_paths
? We search both of them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremy I want to keep this repetition until the leak is fixed, then consolidate expiration and eliminate the repetitive code.
I think I can add that to the exception message when it tries to look up the compiled method location. That should help folks migrate. I'm not thrilled with |
3df7b96
to
681cca1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️💙💚💛💜
Now we can throw away the subclass and the generated methods will get GC'd too
Then we don't need the extra module.
Revert "Remove finalizer and configuration" This reverts commit 9e7b4a3.
4fae2d4
to
570bcda
Compare
…d_container` method Since rails#35036, the subclasses of `ActionView::Base` requires the `compiled_method_container`. This is incompatible. For example, `web-console` use view class that subclass of `ActionView::Base`, and does not work it now cause of this. Actually, since it seems that it is only `ActionView::Base` that `compiled_method_container` is necessary, modified the condition that emits a warning.
…led container. Use the suggested with_empty_template_cache to overcome warning
…pecification Specifically, the new specification added in rails/rails#35036, which expects us to initialize the class using a particular method when we're using it in a oneoff context like this. Also enables us to stop suppressing the deprecation warning.
This is part one for #35032
This PR moves methods generated from ERB to a subclass of
AV::Base
. I was able to verify that theAV::Base
subclass is thrown away every timeDetailsKey
gets cleared, so I also removed the finalizer from the Template object as we don't need to remove methods anymore.The good news is that the number of available methods seen from an ERB file is now constant:
The bad news is that we are still leaking memory:
This patch doesn't fix the unbounded growth of this cache, so it isn't surprising that we still leak. The good news is that the leak is slower, before this patch we leak about 137 objects per request, after we leak about 111 objects per request.
I think the next step is to move this cache on to the
DetailsKey
cache, so they both get cleared at the same time.After that, we need to give
DetailsKey
a better name.