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

Disable ActionView::Template finalizers in test environment #32418

Merged
merged 1 commit into from Apr 2, 2018

Conversation

Projects
None yet
5 participants
@urbanautomaton
Contributor

urbanautomaton commented Apr 2, 2018

Summary

To avoid expensive finalization of ActionView::Template instances at the end of large test suites, this adds a configuration option to disable the finalizers, and uses it to disable them in the test environment.

Description

ActionView::Template instances compile their source to methods on the ActionView::CompiledTemplates module. To prevent leaks in development mode, where templates can frequently change, a finalizer is added that undefines these methods when the templates are garbage-collected.

This is undesirable in the test environment, however, as templates don't change during the life of the test, so there's no leak to be avoided. Moreover, the cost of undefining a method is proportional to the number of descendants a class or module has, since the method cache must be cleared for all descendant classes.

As ActionView::CompiledTemplates is mixed into ActionView::TestCase (or in RSpec suites, every view example group), it can end up with a very large number of descendants, and undefining its methods can become very expensive. In large test suites, this results in a long delay at the end of the test suite as all template finalizers are run, only for the process to then exit.

To avoid this unnecessary cost, this change adds a config option, action_view.finalize_compiled_template_methods, defaulting to true, and sets it to false in the test environment only.

Other Information

I've built a small reproduction of this issue using RSpec, illustrating a test suite that has 1,000 templates. For this suite the finalization cost at the end of the run is approximately 30 seconds, which is comparable to the delay seen in the app I'm currently working on (~45s).

# spec/views/repro.html.erb_spec.rb
require 'rails_helper'

(1..1000).each do |i|
  RSpec.describe "many/#{i}.html.erb" do
    it 'renders the template' do
      render

      expect(rendered).to include("some-contents-#{i}")
    end
  end
end
$ time rspec spec/views/repro.html.erb_spec.rb
..... [many examples snipped] ....

Finished in 32.51 seconds (files took 2.17 seconds to load)
1000 examples, 0 failures


real    1m7.304s
user    1m6.304s
sys     0m0.841s

Feedback

I've confirmed that this patch addresses the performance issue. However, I'm not entirely sure that exposing a configuration option is the best thing in this instance, since it's inevitably a rather obscure flag that the vast majority of users probably won't want to touch.

Any and all feedback is obviously welcome, but in particular if anyone has a better idea for making this environment-specific change, I'd be very open to suggestions.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Apr 2, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

rails-bot commented Apr 2, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 2, 2018

Member

Can you add a changelog entry?

Member

rafaelfranca commented Apr 2, 2018

Can you add a changelog entry?

@urbanautomaton

This comment has been minimized.

Show comment
Hide comment
@urbanautomaton

urbanautomaton Apr 2, 2018

Contributor

Done! (Ack, sorry, just realised I squashed it onto the same commit despite rails-bot's advice; I'll push any further code changes separately.)

Contributor

urbanautomaton commented Apr 2, 2018

Done! (Ack, sorry, just realised I squashed it onto the same commit despite rails-bot's advice; I'll push any further code changes separately.)

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 2, 2018

Member

It is good squashed. I was going to ask to squash anyway. Thanks.

Member

rafaelfranca commented Apr 2, 2018

It is good squashed. I was going to ask to squash anyway. Thanks.

Template finalization can be expensive in large view test suites.
Add a configuration option,
`action_view.finalize_compiled_template_methods`, and turn it off in

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 2, 2018

Member

This reminder me that we need to update the configuration guide to add this new option. Can you do that?

@rafaelfranca

rafaelfranca Apr 2, 2018

Member

This reminder me that we need to update the configuration guide to add this new option. Can you do that?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 2, 2018

Member

Can you squash your commits now?

Member

rafaelfranca commented Apr 2, 2018

Can you squash your commits now?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Apr 2, 2018

Member

Question - wouldn't this also slow down the exiting of an app in production?

Member

pixeltrix commented Apr 2, 2018

Question - wouldn't this also slow down the exiting of an app in production?

Add `action_view.finalize_compiled_template_methods` config option
ActionView::Template instances compile their source to methods on the
ActionView::CompiledTemplates module. To prevent leaks in development
mode, where templates can frequently change, a finalizer is added that
undefines these methods[1] when the templates are garbage-collected.

This is undesirable in the test environment, however, as templates don't
change during the life of the test. Moreover, the cost of undefining a
method is proportional to the number of descendants a class or module
has, since the method cache must be cleared for all descendant classes.

As ActionView::CompiledTemplates is mixed into every
ActionView::TestCase (or in RSpec suites, every view spec example
group), it can end up with a very large number of descendants, and
undefining its methods can become very expensive.

In large test suites, this results in a long delay at the end of the
test suite as all template finalizers are run, only for the process to
then exit.

To avoid this unnecessary cost, this change adds a config option,
`action_view.finalize_compiled_template_methods`, defaulting to true,
and sets it to false in the test environment only.

[1] https://github.com/rails/rails/blob/09b2348f7fc8d4e7191e70e06608c5909067e2aa/actionview/lib/action_view/template.rb#L118-L126
@urbanautomaton

This comment has been minimized.

Show comment
Hide comment
@urbanautomaton

urbanautomaton Apr 2, 2018

Contributor

@rafaelfranca squashed and rebased!

@pixeltrix In theory, yes. However in practice, because very few application classes are descendants of ActionView::CompiledTemplates[1], the cost of finalization is tiny, so it's not really a problem. It's only once you get up to the hundreds/thousands of descendants that undefining methods starts taking ~50ms a pop and it starts to get noticeable.

[1] In most apps it'll just be ActionView::Base.

Contributor

urbanautomaton commented Apr 2, 2018

@rafaelfranca squashed and rebased!

@pixeltrix In theory, yes. However in practice, because very few application classes are descendants of ActionView::CompiledTemplates[1], the cost of finalization is tiny, so it's not really a problem. It's only once you get up to the hundreds/thousands of descendants that undefining methods starts taking ~50ms a pop and it starts to get noticeable.

[1] In most apps it'll just be ActionView::Base.

@rafaelfranca rafaelfranca merged commit 9facd9a into rails:master Apr 2, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@urbanautomaton urbanautomaton deleted the urbanautomaton:disable-template-finalizer-in-test branch Apr 2, 2018

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Apr 3, 2018

Use `:default` option in order to set default value of `finalize_comp…
…iled_template_methods`

Since we introduced default option for `class_attribute` and
`mattr_accessor` family of methods and changed all occurrences of setting
default values by using of `:default` option I think it would be fine to use
`:default` option in order to set default value of `finalize_compiled_template_methods`
since it expresses itself very well.

Related to rails#29294, rails#32418

y-yagi added a commit that referenced this pull request May 20, 2018

@GulajavaMinistudio GulajavaMinistudio referenced this pull request May 20, 2018

Merged

Update upstream #365

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment