Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

  Steve March 17th, 2010 @ 07:53 AM

  • Tag changed from “caching, content_for, feature_request” to “caching, content_for, feature_request, patch”

  At my company, a simple grep -R "content_for" app | wc -l shows we have 86 places where we use content_for. We would like to use action caching to improve our site performance while still retaining some dynamism, but unfortunately refactoring to remove our reliance on content_for is simply impractical.

  So without further ado I introduce to you... action caching AND fragment caching that both support content_for blocks!! Tryout my patch and let me know if you have any success.
  

  I added tests for every piece of functionality I added, and I ran the tests for the files I changed. Everything appears to be OK, but I couldn't get the full rails test suite passing even without my patch applied (rvm + ruby1.9.1 + pg + mysql = FAIL!). But like I said, these other tests seem less applicable. ActiveSupport had a bunch of warnings about i8ln and of cource ActiveRecord mysql + postgre wouldn't run. Everything else seemed to be OK.

• You flagged this item as spam.
  

  Steve March 17th, 2010 @ 07:59 AM

  Whoops that patch was for 3.0b1. I can probably make a similar one for 2.3.6 without much trouble, but... don't try to apply this to 2.3.6.

  Seems a little unfortunate that there is no obvious way to edit or remove my previous reply.

• You flagged this item as spam.
  

  Steve March 17th, 2010 @ 04:06 PM

  OK maybe I should have let my mind simmer for awhile before I submitted that patch. I just thought of a small problem with the way I implemented the fragment content_for caching, and a good way I could DRY up the code a little bit. Stay tuned for a new patch in the next couple days... I'll also take a look at implementing this for Rails 2.3.6.

• You flagged this item as spam.
  

  Steve March 18th, 2010 @ 09:35 AM

  Whew! OK I'm making progress. I had a nightmarish merge because I (foolishly) started from the 3.0b1 tag when I was originally working on this, instead of starting from master.

  I definitely made some major improvements over my previous attempt. It works great and tests pass, but I think I can do better. I'll sleep on it then post the new patch. No point in rushing it I guess, especially considering I appear to be the only one interested in this patch. ;)

  Thanks,
  Steve

• You flagged this item as spam.
  

  Greg Hazel March 18th, 2010 @ 03:57 PM

  I'm interested! :)

  Take your time, though.

• You flagged this item as spam.
  

  Steve March 23rd, 2010 @ 09:17 PM

  OK I have one failing test left. The problem I'm trying to fight through seems to be that in the testing environment, @controller.view_context always brings back a new object, so trying to assert content_for?(:foo) will of course not work, as it's transitory and will vanish the next time you call @controller.view_context

  I guess I'll look at moving my fragment_for test with content_for to the view tests, but right now it's looking fairly confounding. Plus trying to patch Rails is like coding for a moving target, so it's proved an incredible time-sink: write the tests, writing the code to make them pass, rebase, make the tests pass again, rebase, make the tests pass again.... etc. It's like I'm stuck in an infinite loop. Even if I get them to pass and rebase again and everything still looks good and post a patch really quick, it will probably be almost immediately impossible to apply cleanly.

  How do most people deal with this?

• You flagged this item as spam.
  

  Steve March 23rd, 2010 @ 10:53 PM

  • Title changed from “content_for and fragment caching” to “[PATCH]content_for and fragment caching”

  OK well maybe I was exaggerating a bit. Anyway time will tell. I finally got this working, albeit with a slightly shady test implementation (by memoizing view_context in the test controllers).

  So this probably not the best implementation that is possible, but it's the best implementation I could figure out in a first pass. Here is the basic strategy:

  Action Caching & Fragment Caching

  • Added a method CaptureHelper#all_content_for which returns a deep dup of @_content_for instance variable, with the :layout key deleted.
  • If caches_action is called with :layout => false, grab all_content_for, dump it with Marshal, and cache it separately (using a key prepended with "content_for/" instead of "views/").
  • If fragment_for is called, we have to determine what content_for was added in the block that is being cached. To do this, we grab all_content_for before we enter the block, then we grab all_content_for again after the block and diff the two hashes. For any keys that appear in the last hash and the diff_hash, we sub out whatever was in the earlier value with empty string, and we are left with exactly what was added to @_content_for in the block. We Marshal that and cache it separately.
  • Whenever read_fragment is called, we attempt to look up the cached content_for and "rehydrate" it. Ie, for each key in the Marshalled hash we pull out of the cache, we call content_for(key, value) in order to push that stuff back into the @_content_for variable.

  I only ran the ActionPack tests, since that's all I touched and I didn't want to waste my time getting the pg and system_timer gems installed & working properly on my dev box.

• You flagged this item as spam.
  

  Steve March 24th, 2010 @ 02:24 AM

  Also since I (foolishly) started my work on the 3.0b1 tag, it will probably be fairly easy to backport this enhancement to Rails 2.x. I'll work on that next since I don't see my company taking the time and effort to upgrade from 2.3.2 to 3 anytime soon.

  I had to start over from scratch with master as my starting point in order to get the patch working properly, so by now I know this area of the Rails code fairly well. ;)

• 

  Yehuda Katz (wycats) March 27th, 2010 @ 07:52 AM

  • State changed from “new” to “open”
  • Tag changed from “caching, content_for, feature_request, patch” to “caching, content_for, feature_request, patch, rails3”
  • Assigned user set to “Yehuda Katz (wycats)”
  • Milestone cleared.

• You flagged this item as spam.
  

  Steve March 29th, 2010 @ 06:41 PM

  OK I've backported this to 2.3.2. Should I post that patch here or keep it off to avoid confusion? Does anyone other than me want it for 2.3.2?

• 

  Yehuda Katz (wycats) March 29th, 2010 @ 07:25 PM

  @steve I reviewed the patch and would like to chat with you. Are you on IRC or GTalk?

• 

  Yehuda Katz (wycats) March 29th, 2010 @ 09:48 PM

  • State changed from “open” to “hold”

  I'm putting this ticket on hold while I work with Steve on improving this patch. It's definitely important and I hope to get it in before 3.0 final.

• You flagged this item as spam.
  

  Steve April 6th, 2010 @ 08:24 AM

  OK! I was foolish enough to tell Yehuda that I was "just about done" like four days ago. Then I promptly fell into a super amateurish rabbit-hole. Anyway I finally came out the other end with this new, improved, updated patch for your consideration!

  Caching content_for

  • Assumes that the cache_store can store arbitrary ruby objects (or at least hashes with defaults defined).
    • IF this is not true, it will be necessary for "string-only" cache_stores to be enhanced to transparently serialize and de-serialize objects when they are written and read.
  • For the "standard case" (ie, caching with content_for being unused), this will incur merely an extra if statement for action-caching and an extra block evaluation & new Hash being created and then destroyed in the case of fragment caching.
  • Fragment caching is much cleaner than my previous patch
    • content_for keeps an additional Hash while it is evaluating the block to be cached, which is then cached in addition to the content itself. The pointer to the additional Hash is assigned to nil at the end of the block execution.
  • Cache-store issues with possible falloff of related keys has been mitigated, relative to my previous patch.
    • Yehuda identified a potential problem with the fact that content_for was being stored separately from the content itself. This could cause the problem where the content itself still exists, so the associated block or action won't be re-executed, but the associated cached content_for could have been pushed out of the cache-store.
    • This has been solved by storing the content and the content_for stuff together in a hash.

  Other thoughts

  • It would be nice if the tests weren't so implementation specific, but rather tested the essence of the functionality. I tried to make them less brittle but failed.
  • To that end, re-writing all the tests in rspec would be (IMHO) a brilliant improvement and pave the way for better tests in the future and more flexibility in implementation. I understand that this would be an enormous undertaking, but I figure that with like 10 people in the community working on different pieces, we could knock it out in a reasonable amount of time. Maybe we can coordinate something at RailsConf...

• You flagged this item as spam.
  

  Steve April 22nd, 2010 @ 06:50 PM

  I don't think this is a high priority for anyone, but just in case people are wondering about the status of this -- here's an update:

  I made my tests pass by working around a bug (controller.view_context is a new instance of the view_context every time you call it, so I memoized it in my tests). It seems to me that a simple fix for this would be:
  

  # actionpack/lib/abstract_controller/rendering.rb: line 84
  def view_context
    @template ||= view_context_class.new(lookup_context, view_assigns, self)
  end
  

  A little bonus would be that if legacy Rails apps used the @template instance variable, this might still work for them (assuming that view_context had been called prior to them trying to use it, of course). I'm guessing that there's some reason this wouldn't be ideal, but I'm not sure what that reason is...

  I did a spot check of the cache stores implemented, and I found that they appear to support arbitrary Ruby objects (by utilizing Marshal), so that should be OK.

• You flagged this item as spam.
  

  James Hayton April 28th, 2010 @ 04:36 AM

  I am just stopping by to say thanks for working on this and that I really hope this makes it into rails 3. It's fairly important imho.

• You flagged this item as spam.
  

  varchar April 29th, 2010 @ 12:58 AM

  agreed, looking forward to this making it into rails

• You flagged this item as spam.
  

  Steve April 30th, 2010 @ 02:05 AM

  OK I cleaned up the patch a bit (removed vestige of old approach, squashed my commits, and removed the "def view_context" stuff from the caching_test.rb). The removal of the "def view_context" stuff means that my patch won't work (and accordingly, 2 tests fail), as it depends on view_context being the same object before, during and after rendering.

  To address that, I have also attached a patch that memoizes the view_context in a @template variable (as shown in my last post). Of course, memoizing the view_context causes 2 other tests to fail. Go figure.

• You flagged this item as spam.
  

  James Hayton August 24th, 2010 @ 05:12 PM

  • Importance changed from “” to “High”

  Any update on this? Is this not going to make it in Rails3?

• You flagged this item as spam.
  

  Steve August 31st, 2010 @ 11:29 PM

  @James Hayton -- I honestly haven't tried to apply my patch to the latest Rails3 code base. I'll give it a shot and resolve conflicts/fix issues that come up, then try to reconnect with Yehuda about it...

  Yehuda mentioned it may not make it into Rails 3 till a point release or something.

• You flagged this item as spam.
  

  Zach September 19th, 2010 @ 05:13 AM

  @Steve, have you tested this patch with the Rails3 release? Could you post the latest patch? I'd be happy to help out with this, but i'd rather not start from scratch. Thanks.

• You flagged this item as spam.
  

  Steve September 22nd, 2010 @ 08:38 AM

  @Zach: honestly, I haven't touched this stuff for months. My 9-5 job that pays the bills has been taking all my time & energy recently, and realistically, I don't see myself being able to make time for this till next week at the earliest.

  I'll try to get to it sooner, but it doesn't look good. FWIW, Yehuda said we can target 3.1 for this.

• You flagged this item as spam.
  

  James Hayton September 23rd, 2010 @ 08:12 PM

  @Steve: Thanks for the updates. Would love for this to happen. Thanks for all the work. Totally understand how things go.

• 

  Jeremy Kemper October 15th, 2010 @ 11:01 PM

  • Milestone set to “3.0.2”

  [bulk edit]

• You flagged this item as spam.
  

  Joost Schuur October 29th, 2010 @ 11:31 AM

  Allow me to add my encouragement on getting this in too, @Steve. Just ran into this problem myself and will work around it less elegantly for now.

• You flagged this item as spam.
  

  Steve October 30th, 2010 @ 07:13 AM

  Joost, James, Zach -- thanks. Your encouragement spurs me onward. :)

• You flagged this item as spam.
  

  Daniel Gaiottino November 5th, 2010 @ 01:53 PM

  What's the latest status for this? I've run into this problem making it impossible for me to cache certain items which I'd really like cached. The problem most of the time is that we use content_for to send javascript code to the bottom of the dom.

  Will this be included in 3.0.2? Is it testable/usable today?

• You flagged this item as spam.
  

  Ben Hutton November 5th, 2010 @ 05:15 PM

  +1... also eagerly awaiting this!

• 

  Rohit Arondekar November 17th, 2010 @ 01:33 AM

  Any updates here?

• You flagged this item as spam.
  

  Steve November 17th, 2010 @ 02:03 AM

  Actually, yes!

  I've been working on it all this past week. In short, I think I'm going to separate the fragment caching and the action caching support for content_for into two patches. The reason for this is that the fragment part is pretty much done and working, and while I was able to get action caching working with content_for as well, 149 tests in action_pack broke afterwards(!).

  @Daniel -- it's not testable nor usable yet. I'll post a patch soon that can cleanly apply to current-ish HEAD.

• You flagged this item as spam.
  

  Steve November 17th, 2010 @ 02:56 AM

  Hah! Just got them down to 10 failures. Patch may be forthcoming... :)

• You flagged this item as spam.
  

  Steve November 17th, 2010 @ 10:24 AM

  [Action pack] tests are green! Feast your eyes on the patch that should apply cleanly to current HEAD.

  It's a bit ugly, but it works. I have an idea for refactoring to something a little less disgusting, but for now... this is what I got. Feel free to check it out and give me your suggestions for making it prettier.

  • cache_content_for_v4.patch 16.3 KB

• You flagged this item as spam.
  

  Daniel Gaiottino January 5th, 2011 @ 05:26 PM

  What does it mean when it says that this story is on hold?

  Since the patch passes all the tests shouldn't it included in the next release of rails? Still waiting eagerly for this.

• You flagged this item as spam.
  

  Steve January 6th, 2011 @ 07:11 AM

  • Assigned user changed from “Yehuda Katz (wycats)” to “José Valim”

  @Daniel -- honestly I don't think this is patch is going to be accepted. Obviously, I'd love to see it accepted, as I've put numerous hours into getting it working, but I think it's a bit too big (touches too many files), and is probably a bit too complex for the rails team to feel comfortable accepting.

  That said, last I heard from Yehuda, José was supposed to review this patch, but I haven't heard anything about it since then (Nov 17th). So I guess I'll assign it to José and maybe he'll be able to help me get this ready for primetime (?).

  Cheers!

  -Steve

• You flagged this item as spam.
  

  Brad Langhorst January 21st, 2011 @ 04:45 PM

  Getting content_for to work with caches_action might get me to update to rails3 ...
  unless there's a backport somewhere.

• You flagged this item as spam.
  

  James Hayton February 25th, 2011 @ 06:49 PM

  Can someone give an update on this? Would love to know if it will make it to 3.1? Really really useful feature imho.

• You flagged this item as spam.
  

  Steve February 25th, 2011 @ 07:03 PM

  @James: I think that making action caching (when you're not caching the template) work with content_for might be a bit too heavy for the Rails team to accept. I'll pull out fragment caching and make it a separate patch since it's much smaller and more lightweight IMO. After that, if no rails core team member will review it and accept it or even reject it... I think that will be the end of this (as far as I'm concerned).

• You flagged this item as spam.
  

  James Hayton February 25th, 2011 @ 08:34 PM

  @Steve: I just wish they would at least review it and give an answer one way or the other. I can't believe that there aren't more people clamoring for this. I use action caching extensively with layout => false and this would really help me dry up my views.

• You flagged this item as spam.
  

  StackNG March 29th, 2011 @ 07:50 AM

  For those who need this function now:
  I just make a monkey patch for rails 3 based on Steve's great work, and leave the abstract_controller untouched.
  The patch is here: https://gist.github.com/891895, all you have to do is to put it in your config/initializers directory.

  I'm very curious why abstract_controller.view_context returns a new instance instead of a memorized one like @template, too.
  Could anyone point this out?

• You flagged this item as spam.
  

  opsb May 15th, 2011 @ 01:50 PM

  Thank you so much for this. We really wanted to start using action caching with memcache, it was a no go without this patch.