Use digest cache in development. #20384

kaspth · 2015-05-30T21:15:50Z

Fixes #20361

Avoid computing the same fragment digest many times when looping over templates, and
cut down on logging noise.

The cache is cleared on every request, so there's no chance of stale templates being served.

kaspth · 2015-05-30T21:23:02Z

Ya, it's kinda late here and I think I missed that we should wrap around fragment_for to capture what's written to the cache and not just the fragment name. c/d @dhh?

I should add a test for the caching writing and reading. Possibly even one for the reduced log output.

For glory and all the XP!

dhh · 2015-06-01T13:06:13Z

I haven't actually dug into this part of the code in a long time. @rafaelfranca could you vet this?

rafaelfranca · 2015-06-01T14:52:56Z

Ya, it's kinda late here and I think I missed that we should wrap around fragment_for to capture what's written to the cache and not just the fragment name. c/d

c

rafaelfranca · 2015-06-01T14:55:07Z

actionview/lib/action_view/railtie.rb

@@ -42,6 +42,16 @@ class Railtie < Rails::Railtie # :nodoc:
      end
    end

+    initializer "action_view.fragment_name_cache" do |app|
+      ActiveSupport.on_load(:action_view) do
+        if Rails.env.development?


I think we should check consider_all_requests_local.

dhh · 2015-06-26T13:29:05Z

Hey @kaspth had any chance to consider this further?

kaspth · 2015-06-26T13:44:08Z

Sorry, I've totally sweat this out. I'll take a look this weekend 👍

Kasper

On 26. jun. 2015 15.29 +0200, David Heinemeier Hanssonnotifications@github.com, wrote:

Hey@kaspth(https://github.com/kaspth)had any chance to consider this further?

—
Reply to this email directly orview it on GitHub(#20384 (comment)).

dhh · 2015-06-26T14:05:13Z

Rocking. Thanks man!

On Fri, Jun 26, 2015 at 3:45 PM, Kasper Timm Hansen <
notifications@github.com> wrote:

Sorry, I've totally sweat this out. I'll take a look this weekend 👍

Kasper

On 26. jun. 2015 15.29 +0200, David Heinemeier Hansson<
notifications@github.com>, wrote:

Hey@kaspth(https://github.com/kaspth)had any chance to consider this
further?

—
Reply to this email directly orview it on GitHub(
#20384 (comment)).

—
Reply to this email directly or view it on GitHub
#20384 (comment).

kaspth · 2015-06-26T19:59:55Z

actionview/lib/action_view/helpers/cache_helper.rb

@@ -98,7 +98,7 @@ module CacheHelper
      #   <%# Template Dependency: todolists/todolist %>
      #   <%= render_sortable_todolists @project.todolists %>
      #
-      # The pattern used to match these is <tt>/# Template Dependency: (\S+)/</tt>,
+      # The pattern used to match these is /# Template Dependency: ([^ ]+)/,


Unintended change from a rebase, I'll fix it on the next push.

rafaelfranca · 2015-06-27T04:40:45Z

railties/test/application/per_request_fragment_cache_test.rb

+        get '/customers/1'
+      end
+
+      assert write_not_called


Since it is per request should not this be false since other request was made and the cache was cleaned?

Haha, doh. Thanks 😄

kaspth · 2015-06-28T18:51:11Z

@rafaelfranca I'm really stumped on getting these tests to work 😢

It's all coming up black magic, and I have no idea what's going on.

For some reason the initializers aren't run. However, if I added this in cache_helper.rb:

class ActionView::Base
  mattr_accessor(:per_request_fragment_cache) { ActiveSupport::Cache::MemoryStore.new }
end

They were run! Do you have any idea why? 😄

The next problem was that app.config.consider_all_requests_local was false even though I added it to the config. Can you spot if I've done it wrong?

rafaelfranca · 2015-07-02T00:13:41Z

Weird. I'll debug in my machine

kaspth · 2015-07-12T09:48:28Z

@dhh I finally got everything up and running here. I reread your original description of the problem and realized I'd been trying to solve the wrong thing 😅

Anyway, the cache now captures generated digests which I think is what you want.

Also I looked at ActionView::Digestor which seems to already have a cache. Can that fill your use case? 😄

dhh · 2015-07-12T19:21:44Z

Hey, hey. Yeah, all I wanted was that the cache we're already using in production for template digests to be used on a per-request basis in development. So the first lookup of a template within a request in development is computed, all subsequent lookups are not. 👍

Avoid computing the same fragment digest many times when looping over templates. The cache is cleared on every request so template changes are still picked up.

kaspth · 2015-07-18T20:05:59Z

@dhh Boom, got it 👍. Way easier once I figured out there was already a cache.

I'm closing and reopening for Travis to run.

dhh · 2015-07-20T11:15:44Z

Badabing! Awesome. Thanks!

Use digest cache in development.

kaspth added the actionview label May 30, 2015

kaspth self-assigned this May 30, 2015

kaspth added this to the 5.0.0 milestone May 30, 2015

rafaelfranca reviewed Jun 1, 2015
View reviewed changes

kaspth force-pushed the per-request-cache branch from 186dde3 to 9407b23 Compare June 26, 2015 19:58

kaspth reviewed Jun 26, 2015
View reviewed changes

kaspth force-pushed the per-request-cache branch 2 times, most recently from 07de57b to 810013d Compare June 26, 2015 20:04

rafaelfranca reviewed Jun 27, 2015
View reviewed changes

kaspth force-pushed the per-request-cache branch from 810013d to 1f626fd Compare June 28, 2015 18:35

kaspth force-pushed the per-request-cache branch from 1f626fd to 87c56e0 Compare July 12, 2015 09:42

kaspth added 2 commits July 18, 2015 21:54

Make digest cache work in development.

40f79da

Avoid computing the same fragment digest many times when looping over templates. The cache is cleared on every request so template changes are still picked up.

Remove unused persistent option.

7bd70ba

kaspth force-pushed the per-request-cache branch from 87c56e0 to 7bd70ba Compare July 18, 2015 19:54

kaspth closed this Jul 18, 2015

kaspth reopened this Jul 18, 2015

kaspth changed the title ~~Add per request fragment name cache.~~ Use digest cache in development. Jul 19, 2015

dhh added a commit that referenced this pull request Jul 20, 2015

Merge pull request #20384 from kaspth/per-request-cache

068865a

Use digest cache in development.

dhh merged commit 068865a into rails:master Jul 20, 2015

kaspth deleted the per-request-cache branch July 20, 2015 11:20

rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

This was referenced Dec 5, 2016

Don't clear digest cache in test environment #27271

Merged

Memory leak in development #27273

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use digest cache in development. #20384

Use digest cache in development. #20384

kaspth commented May 30, 2015

kaspth commented May 30, 2015

dhh commented Jun 1, 2015

rafaelfranca commented Jun 1, 2015

rafaelfranca Jun 1, 2015

dhh commented Jun 26, 2015

kaspth commented Jun 26, 2015

dhh commented Jun 26, 2015

kaspth Jun 26, 2015

rafaelfranca Jun 27, 2015

kaspth Jun 28, 2015

kaspth commented Jun 28, 2015

rafaelfranca commented Jul 2, 2015

kaspth commented Jul 12, 2015

dhh commented Jul 12, 2015

kaspth commented Jul 18, 2015

dhh commented Jul 20, 2015

Use digest cache in development. #20384

Use digest cache in development. #20384

Conversation

kaspth commented May 30, 2015

kaspth commented May 30, 2015

dhh commented Jun 1, 2015

rafaelfranca commented Jun 1, 2015

rafaelfranca Jun 1, 2015

Choose a reason for hiding this comment

dhh commented Jun 26, 2015

kaspth commented Jun 26, 2015

dhh commented Jun 26, 2015

kaspth Jun 26, 2015

Choose a reason for hiding this comment

rafaelfranca Jun 27, 2015

Choose a reason for hiding this comment

kaspth Jun 28, 2015

Choose a reason for hiding this comment

kaspth commented Jun 28, 2015

rafaelfranca commented Jul 2, 2015

kaspth commented Jul 12, 2015

dhh commented Jul 12, 2015

kaspth commented Jul 18, 2015

dhh commented Jul 20, 2015