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 compiled ERB to an AV::Base subclass #35036

Merged
merged 10 commits into from Feb 7, 2019
Merged

Move compiled ERB to an AV::Base subclass #35036

merged 10 commits into from Feb 7, 2019

Conversation

@tenderlove
Copy link
Member

@tenderlove tenderlove commented Jan 24, 2019

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 the AV::Base subclass is thrown away every time DetailsKey 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:

untitled 2019-01-23 16-00-57

The bad news is that we are still leaking memory:

untitled 2019-01-23 16-01-41

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.

Copy link
Member

@jeremy jeremy left a comment

Nice approach.

How will devs writing new tests know to use with_empty_template_cache or to clear other caches beforehand?

actionview/lib/action_view/template.rb Show resolved Hide resolved
actionview/lib/action_view/base.rb Outdated Show resolved Hide resolved
@@ -53,28 +54,6 @@ module Accessors #:nodoc:
register_detail(:variants) { [] }
register_detail(:handlers) { Template::Handlers.extensions }

class DetailsKey #:nodoc:
Copy link
Member

@jeremy jeremy Jan 24, 2019

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

Copy link
Member Author

@tenderlove tenderlove Jan 24, 2019

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)

actionview/lib/action_view/rendering.rb Show resolved Hide resolved
@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)
Copy link
Member

@jeremy jeremy Jan 24, 2019

Seeing this "clear view path and fallback caches" boilerplate repeated. Worth wrapping up so we have one spot to ask AC to start fresh?

Copy link
Member Author

@tenderlove tenderlove Jan 24, 2019

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).

Copy link
Member Author

@tenderlove tenderlove Jan 24, 2019

@jeremy I want to keep this repetition until the leak is fixed, then consolidate expiration and eliminate the repetitive code.

actionview/lib/action_view/railtie.rb Outdated Show resolved Hide resolved
actionview/lib/action_view/template.rb Outdated Show resolved Hide resolved
@tenderlove
Copy link
Member Author

@tenderlove tenderlove commented Jan 24, 2019

How will devs writing new tests know to use with_empty_template_cache or to clear other caches beforehand?

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 with_empty_template_cache as the method name though.

@tenderlove tenderlove force-pushed the av-base-subclass branch from 3df7b96 to 681cca1 Jan 24, 2019
actionview/CHANGELOG.md Show resolved Hide resolved
actionview/lib/action_view/base.rb Outdated Show resolved Hide resolved
kaspth
kaspth approved these changes Jan 24, 2019
Copy link
Member

@kaspth kaspth left a comment

❤️💙💚💛💜

@tenderlove tenderlove force-pushed the av-base-subclass branch from 4fae2d4 to 570bcda Feb 7, 2019
@tenderlove tenderlove merged commit 973b62d into master Feb 7, 2019
3 checks passed
@tenderlove tenderlove deleted the av-base-subclass branch Feb 7, 2019
y-yagi added a commit to y-yagi/rails that referenced this issue Feb 15, 2019
…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.

I am not sure that using `self.class` is correct, but I think that it is
better to show a deprecate message rather than raise exception.
y-yagi added a commit to y-yagi/rails that referenced this issue Feb 15, 2019
…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.

I am not sure that using `self.class` is correct, but I think that it is
better to show a deprecate message rather than raise exception.
y-yagi added a commit to y-yagi/rails that referenced this issue Feb 16, 2019
…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.
vipulnsward added a commit to vipulnsward/rails that referenced this issue Mar 12, 2019
…led container.

Use the suggested with_empty_template_cache to overcome warning
kamipo added a commit that referenced this issue May 6, 2019
The `GC.start` was added at b29e893, but the finalizer has been removed
at 7d0ce78 in #35036.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants