Don't clear digest cache in test environment #27271

printercu · 2016-12-05T18:37:01Z

TL/DR rspec spec/integration. Before: 35sec. After patch: 20sec.

#20384 implemented template digest expiry for dev requests. It's enabled by default for test env, which is not required and degradates test performance.

I'm not sure if there should be testcase for this commit.

UPD There is failing test on travis. But it's for actioncable, and passes locally for me.

rails-bot · 2016-12-05T18:37:05Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (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.

printercu · 2016-12-05T19:18:45Z

@matthewd it would be great to have this one in 5.0.1.

rafaelfranca · 2016-12-05T19:54:54Z

Why it degradates performance? I prefer to fix the performance issue before disabling it.

printercu · 2016-12-05T20:34:26Z

Because it recompiles templates on every request, instead of using cached ones.

matthewd · 2016-12-05T20:43:18Z

To be clear, you're not saying that something got slower... you're saying that the change in #20384 can be adjusted to cache more aggressively in tests, and that makes them faster. Right?

printercu · 2016-12-05T20:56:33Z

Nope. The change in #20384 totally disables caching templates between requests. I've been migrating from 4.2 and found that integration tests got almost 2 times slower. With stackprof I've found that there got too much time in ActionView::Template#compile.

printercu · 2016-12-05T20:59:04Z

I've added this lines to test helper, and this decreased tests time back to 4.2's time:

class << ActionView::LookupContext::DetailsKey
  def clear
  end
end

printercu · 2016-12-05T21:05:09Z

Indeed, I think it's not good idea to clear digest cache if consider_all_requests_local is enabled. Original request (#20361) was about development mode. Should I change it to if Rails.env.development??

matthewd · 2016-12-05T21:12:19Z

actionview/lib/action_view/railtie.rb

@@ -39,7 +39,7 @@ class Railtie < Rails::Engine # :nodoc:

    initializer "action_view.per_request_digest_cache" do |app|
      ActiveSupport.on_load(:action_view) do
-        if app.config.consider_all_requests_local
+        if app.config.consider_all_requests_local && !Rails.env.test?
          app.executor.to_run ActionView::Digestor::PerExecutionDigestCacheExpiry


It looks like we could get rid of the if entirely, and change this to use app.reloader.to_run instead of the executor.

Then it'll automatically get run as part of the reloading in development (if reloading is enabled), and be skipped in test and production (unless reloading is enabled). If it works, that feels like a better statement of our intent -- this current consider_all_requests_local check feels more like an indirect attempt to avoid running in production.

Are you up for trying that instead?

(FYI, this won't make 5.0.1 -- as we're in RC stage, the only things that will be added are fixes for regressions relative to 5.0.0. But it does seem a reasonable candidate for backporting for the future 5.0.2.)

Ok. I'll try it tomorrow.

Just to clarify. Does reloader run when templates changed, or it tracks only .rb files?

Ah, good point. I thought it did, but it looks like I was wrong.

So looking at how views actually get handled... it seems cache_template_loading is what should be deciding whether the digest cache should persist (true), or get cleared for each execution (false).

Don't you think it's overkill? I mean, the only place where it's used is dev env. I can't think any other scenario up.

The point of the config settings is to abstract away the environment definitions. People can define their own environments, for example.

Unless I'm confused, I'm just suggesting we change the if to use ! cache_template_loading where it currently uses consider_all_requests_local, so I'm not sure where the overkill is.

It uses existing config option, but we would define one more. Not really a problem for me.
What if we use clear_compiled_templates instead? The idea is to make it safe for production by default, and not to treat missing value (nil) as true. It would require to enable it explicitly in development.

Or recompile_templates.

cache_template_loading is an already-existing config setting

printercu · 2016-12-06T19:25:26Z

actionview/lib/action_view/railtie.rb

@@ -39,7 +39,7 @@ class Railtie < Rails::Engine # :nodoc:

    initializer "action_view.per_request_digest_cache" do |app|
      ActiveSupport.on_load(:action_view) do
-        if app.config.consider_all_requests_local
+        if ActionView::Resolver.caching?


I use this instead of cache_template_loading, because this one is set to default value if not configured on line 35.

Is this doing what we want? I think it needs to be unless.

Yes, ActionView::Resolver.caching? is on in production by default (because of the cache_classes fallback above).

Nice catch! 😅

Fixed. Waiting for github getting up to repush.

printercu · 2016-12-06T20:19:33Z

Not sure. It just fixes this leak when ActionView::Resolver.caching? is enabled. Dev will remain untouched.

It disables recompilation of templates on every request in test env.

aguynamedben · 2016-12-12T18:40:52Z

@printercu I don't have experience in this area of the code, but this looks sweet!!!

rails-bot assigned schneems Dec 5, 2016

matthewd reviewed Dec 5, 2016

View reviewed changes

maclover7 added actionview needs feedback labels Dec 5, 2016

printercu force-pushed the permantent_digest_in_tests branch from 07c0912 to 9f31baa Compare December 6, 2016 19:22

printercu commented Dec 6, 2016

View reviewed changes

printercu mentioned this pull request Dec 6, 2016

Memory leak in development #27273

Closed

Fixed usage of cache_template_loading

2380354

It disables recompilation of templates on every request in test env.

printercu force-pushed the permantent_digest_in_tests branch from 1031d6a to 2380354 Compare December 6, 2016 20:36

matthewd mentioned this pull request Dec 7, 2016

Fix memory leak in action view #27296

Closed

matthewd merged commit 42b873f into rails:master Dec 7, 2016

printercu deleted the permantent_digest_in_tests branch December 7, 2016 15:10

maclover7 removed the needs feedback label Dec 7, 2016

jcoyne mentioned this pull request Dec 13, 2016

Speed up tests by not clearing the asset cache samvera/hyrax#34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't clear digest cache in test environment #27271

Don't clear digest cache in test environment #27271

Don't clear digest cache in test environment #27271

Don't clear digest cache in test environment #27271

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment