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

Only clear ActionView caches in dev when files change #35629

Merged
merged 1 commit into from Apr 1, 2019

Conversation

@jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Mar 15, 2019

Builds on #35623 and #35628

This changes the dev-mode actionview cache expiration, which used to run before each request, to only change if any files in the view directories have changed.

This should make dev mode template rendering the same speed as production! ⚡️ (though I'm sure there will be other dev mode things slowing it down)

Todo

  • Find out what people use that isn't a FileSystemResolver Make a changelog entry that those with non-FileSystemResolver resolvers will need to clear caches themselves if they want dev mode reloading.
  • We will no longer get dev-mode reloading when using render file: (we can't exactly listen to all changes on/). Decide if that's okay.
  • Measure how much it improves dev mode
Copy link
Member

@kaspth kaspth left a comment

I'm really excited about this! Thanks for working on it.

actionview/lib/action_view.rb Outdated Show resolved Hide resolved
@@ -81,7 +81,7 @@ class Railtie < Rails::Engine # :nodoc:
initializer "action_view.per_request_digest_cache" do |app|
ActiveSupport.on_load(:action_view) do
unless ActionView::Resolver.caching?
app.executor.to_run ActionView::Digestor::PerExecutionDigestCacheExpiry
app.executor.to_run ActionView::CacheExpiry::Executor.new(watcher: app.config.file_watcher)
Copy link
Member

@kaspth kaspth Mar 18, 2019

Did you look into swapping app.executor to app.reloader?

actionview/lib/action_view/cache_expiry.rb Outdated Show resolved Hide resolved
actionview/lib/action_view/cache_expiry.rb Outdated Show resolved Hide resolved
actionview/lib/action_view/cache_expiry.rb Show resolved Hide resolved
@kaspth
Copy link
Member

@kaspth kaspth commented Mar 20, 2019

Lovely! Code wise this is good to go for me.

As for the TODOs:

  1. I'm guessing the strike means you've talked it over with @tenderlove or @eileencodes?

  2. I didn't even know we had render file:, should we continue to support it? GitHub search only brought Action Pack tests on forks: https://github.com/search?l=Ruby&q=render+file%3A&type=Code

    Coupled with the fact that this says "old default" here:

    # * <tt>:file</tt> - Renders an explicit template file (this used to be the old default), add :locals to pass in those.

    I see what you mean about the caching too. Perhaps there's a way to just not bother with any caching in development for file: renders?

    cached(key, [name, prefix, partial], details, locals) do

  3. Looking forward to seeing the measure!

@jhawthorn
Copy link
Member Author

@jhawthorn jhawthorn commented Mar 20, 2019

I didn't even know we had render file:

PR incoming related to this 😉

EDIT: See #35688

@jhawthorn jhawthorn force-pushed the actionview_cache_wip branch from bbde52e to ba0828c Mar 27, 2019
@jhawthorn jhawthorn marked this pull request as ready for review Mar 27, 2019
@jhawthorn
Copy link
Member Author

@jhawthorn jhawthorn commented Mar 27, 2019

This is ready for review!

Benchmarks in my (very view heavy, not doing much else) test app look very promising!

Before

Warming up --------------------------------------
               GET /     5.000  i/100ms
Calculating -------------------------------------
               GET /     71.057  (±15.5%) i/s -    340.000  in   5.066397s

After

Warming up --------------------------------------
               GET /     8.000  i/100ms
Calculating -------------------------------------
               GET /     97.241  (±30.9%) i/s -    400.000  in   5.306943s

@jhawthorn jhawthorn changed the title [WIP] Only clear ActionView caches in dev when files change Only clear ActionView caches in dev when files change Mar 29, 2019
@kaspth
Copy link
Member

@kaspth kaspth commented Mar 31, 2019

@jhawthorn since #35688 has been merged, do we still need to talk about no dev mode reloading for render file: or can that be checked off?

kaspth
kaspth approved these changes Mar 31, 2019
@jhawthorn jhawthorn force-pushed the actionview_cache_wip branch from ba0828c to 126f4cc Apr 1, 2019
@jhawthorn
Copy link
Member Author

@jhawthorn jhawthorn commented Apr 1, 2019

since #35688 has been merged, do we still need to talk about no dev mode reloading for render file: or can that be checked off?

Yep! We've lost reloading on the deprecated behaviour, which I think is fine.

I've added a CHANGELOG entry noting custom ActionView::Resolver subclasses (which should be pretty rare) may need to add their own cache clearing.

I think this is good to go!

@kaspth kaspth merged commit c8bf334 into rails:master Apr 1, 2019
2 checks passed
@kaspth
Copy link
Member

@kaspth kaspth commented Apr 1, 2019

@jhawthorn ❤️

love


To speed up development mode, view caches are only cleared when files in
the view paths have changed. Applications which have implemented custom
ActionView::Resolver subclasses may need to add their own cache clearing.
Copy link
Member

@kaspth kaspth Apr 1, 2019

I was wondering if we should expose ActionView::LookupContext::DetailsKey.clear as clear_cache on resolvers. iirc DetailsKey is private so users shouldn't know about that constant directly — and I don't know how else they'd clear the cache. What do you think?

Copy link
Member Author

@jhawthorn jhawthorn Apr 1, 2019

👍 I agree it needs a better (and not private) name. It's a little tricky because afaict having Resolver#clear_cache wouldn't currently work, when testing I got errors unless we cleared all the caches.

Maybe we should expose ActionView::CacheExpiry.clear_cache (ie. make it class-level and document it)

Copy link
Member

@kaspth kaspth Apr 1, 2019

Should the resolvers know about ActionView::CacheExpiry? Currently the cache expiry knows about the resolvers, so it seems off that we allow double "binding". Since it's just abstracting a reference to DetailsKey anyway, how about a class-level ActionView::Resolver.clear_caches?

Which we then also use in lieu of clear_cache in CacheExpiry.

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

3 participants