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

Don't clear digest cache in test environment #27271

Merged
merged 1 commit into from Dec 7, 2016

Conversation

@printercu
Copy link
Contributor

@printercu printercu commented Dec 5, 2016

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
Copy link

@rails-bot rails-bot commented Dec 5, 2016

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
Copy link
Contributor Author

@printercu printercu commented Dec 5, 2016

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

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 5, 2016

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

@printercu
Copy link
Contributor Author

@printercu printercu commented Dec 5, 2016

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

@matthewd
Copy link
Member

@matthewd matthewd commented Dec 5, 2016

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
Copy link
Contributor Author

@printercu printercu commented Dec 5, 2016

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
Copy link
Contributor Author

@printercu printercu commented Dec 5, 2016

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
Copy link
Contributor Author

@printercu printercu commented Dec 5, 2016

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??

actionview/lib/action_view/railtie.rb Outdated
@@ -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

This comment has been minimized.

@matthewd

matthewd Dec 5, 2016
Member

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

This comment has been minimized.

@printercu

printercu Dec 5, 2016
Author Contributor

Ok. I'll try it tomorrow.

This comment has been minimized.

@printercu

printercu Dec 5, 2016
Author Contributor

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

This comment has been minimized.

@matthewd

matthewd Dec 5, 2016
Member

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

This comment has been minimized.

@printercu

printercu Dec 5, 2016
Author Contributor

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.

This comment has been minimized.

@matthewd

matthewd Dec 6, 2016
Member

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.

This comment has been minimized.

@printercu

printercu Dec 6, 2016
Author Contributor

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.

This comment has been minimized.

@printercu

printercu Dec 6, 2016
Author Contributor

Or recompile_templates.

This comment has been minimized.

@matthewd

matthewd Dec 6, 2016
Member

cache_template_loading is an already-existing config setting

@printercu printercu force-pushed the printercu:permantent_digest_in_tests branch Dec 6, 2016
actionview/lib/action_view/railtie.rb Outdated
@@ -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?

This comment has been minimized.

@printercu

printercu Dec 6, 2016
Author Contributor

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

This comment has been minimized.

@matthewd

matthewd Dec 6, 2016
Member

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

This comment has been minimized.

@kaspth

kaspth Dec 6, 2016
Member

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

This comment has been minimized.

@printercu

printercu Dec 6, 2016
Author Contributor

Nice catch! 😅

This comment has been minimized.

@printercu

printercu Dec 6, 2016
Author Contributor

Fixed. Waiting for github getting up to repush.

This comment has been minimized.

@printercu

printercu Dec 6, 2016
Author Contributor

pushed

@printercu
Copy link
Contributor Author

@printercu printercu commented Dec 6, 2016

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.
@printercu printercu force-pushed the printercu:permantent_digest_in_tests branch to 2380354 Dec 6, 2016
@matthewd matthewd merged commit 42b873f into rails:master Dec 7, 2016
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@printercu printercu deleted the printercu:permantent_digest_in_tests branch Dec 7, 2016
@aguynamedben
Copy link
Contributor

@aguynamedben aguynamedben commented Dec 12, 2016

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

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

8 participants
You can’t perform that action at this time.