Improve view performance in development and reinstate template reloading in production

Sorry, meant Josh Goebel, not Rick

David, the default is now to not to recompile, i.e. cache_template_loading = true in the default production environment file.

Also it doesn't check every templates mtime - at least it shouldn't do, if it does let me know. Only templates that are going to be rendered should have the mtime checked.

Andrew, I have implemented a very similar functionality in rails-dev-boost I assume you've looked at it while making the patch.

I went through the diff. It is quite well written, but I do have couple of points: 1. You are not properly tracking mtimes and the template (once modified) is thus considered stale forever and is being recompiled every request after that. 2. Once you fix that, you will discover that you have to start tracking methods that are compiled on ActionView::Base::CompiledTemplates and manually undef all of them on stale templates (the plugin does that). 3. I would also suggest you introduce something similar to @disk_cache (as it is in the plugin). I am pretty sure you are running Dir.glob on pretty much every find_template request. 4. It probably is not thread safe (neither is my implementation in the plugin), is something that is not thread safe going to be accepted into core?

On the other hand Rails 2.3 performance in the development mode is appalling and developing a 70 controller app becomes really painful. I also think we should be paying a little bit more attention to how slow the development mode is, after all every Rails developer in the world uses dev. mode to build his apps. Isn't not-waisting developer's time one of the core ideas behind Rails?

I'm +1 on the patch (after some fixes of course).

-thedarkone

PS: Andrew, have you tried running your app with rails-dev-boost?

Damn the formatting :(.

And in case anyone cares, rails-dev-boost is an improved (Rails 2.2 & 2.3 compatible) alternative to the Josh Goebel's rails_dev_mode_performance, so if anyone is having really bad time with development mode, give it a try :).

Attached is a new patch which addresses some of the issues mentioned.

I've handled the threadsafe issue for the moment by forcing to cached templates only. The mtime is now tracked and it now works properly when partials have been changed.

I'm not removing the methods because I'm sure I read somewhere that it leaks memory, plus I'm assuming that it wouldn't be threadsafe. The Dir.glob in find_template is only for finding possible templates within the base_path so only ever matches a few files and is a one time hit when a newly added template is requested.

I did look at rails-dev-boost and did try running it with my app but there were some conflicts with the dependencies modifications. I did have a look before writing this patch but didn't use it anyway since I wanted to touch the minimum of code in this patch so that it would stand a good chance of getting into 2.3, so I'm sure it's not optimal.

I would hope for 3.0 that as part of the public API the template picking and caching would be pluggable so I fully expect this to be chucked at some point.

+1 for including this in 2.3 -- and bonus points for fixing 2 other tickets I am watching closely!

This patch completely resolved #1922 for me. +1 for including in 2.3.

Joshua, for what it's worth, if 2.3 goes gold without a fix like this, it's going to be neigh-unusable for me. Adding an additional 2 seconds per page reload is going to destroy my development productivity, and I'd much rather develop against a faster 2.2.2 than a slower, more featureful 2.3. The slower view performance is a definite regression from 2.2.2.

@Chris woah, don't worry, its going get in. Just waiting on Yehuda's comments.

Woot. Very glad to hear that. :)

Yes there is.

My engine is running fine without cache_classes turned on but in production environment i get an error during server boot.

/vendor/rails/railties/lib/initializer.rb:374:in load_view_paths': undefined local variable or methodview_path' for #Rails::Initializer:0x1a59334 (NameError)

Oh shit, it already got commited? The patch doesn't cover some non obvious edge cases that will painful to debug.

Here what I was going to write, before refreshing the page:

Andrew, I'd suggest you rewrite the mtime checking thing without using memoize at all, it does have some overhead and you aren't really using it to memoize anything, but that's just my opinion.

Now onto the important stuff: I don't think remove_method leaks memory on methods defined through evaled def abc string, plus as you said, you don't care about thread safety of reloading templates. However all that is irrelevant, you still need to track the compiled methods and recompile/remove them manually, I don't think there is a simple way round it :).

  index.html.erb:
  <%= render 'test' %>
  <%= render 'test', :a => 'a' %>
  
  _test.html.erb:
  a

Change _test.html.erb and see what you get back.

As for Dir.glob - unless all your templates have locale extensions your implementation does invoke Dir.glob in every find_template call.

I'm sorry if I sound like a major PITA here, but the stuff the plugin does and you patch doesn't is there for a reason.

I attempted to port my patch from the plugin. I'm 95% certain that it works (the core is the same, I just had to wire it up with the framework differently), but it does have shitload of tests failing. I'm not really familiar with actionpack test scaffolding and therefore quite lost. If anyone is up for the task of fixing them... help is appreciated :).

Add-reloadable-templates-HEAD2.patch should be against pre-patch.

Add-reloadable-templates-HEAD.patch should be against current master.

Since I do have some expertise at implementing reloadable templates, I'll try to go through any other patch that gets posted here :).

Wow, I do suck at making diffs :(.

Attaching new patch against current master/HEAD.

I don't know about that "ActionView::ReloadableTemplate.register_new_request_callback!(ActionMailer::Base)" stuff.

We don't want that kind of initializer code in the dispatcher nor do we want to be referencing ActionMailer when someone could decide to not include it. AC should not reference AM.

Okay, here's a patch that addresses the edge case of the same template being called with different local assigns. I've also tweaked it slightly so that when perform_caching is disabled then templates are always reloaded from the disk. This approximates to the old behaviour.

Rather than remove methods I'm tracking the timestamp of when the source was compiled and the timestamp of when a method is defined. If the timestamp of compilation is later than the definition then it gets redefined. Removing methods was causing some of the tests to fail.

I've removed the load_framework_views code again and tweaked the initializer test to call initialize_framework_views instead. There were the two methods before because we had Path and EagerPath but now they've been merged we don't need load_view_paths.

I've also testing with some of my own engine code and it seems to work fine.

Both ActionPack and Railties tests pass.

Current rails edge is broken due to a typo in initializer.rb.

This patch fixes this.

Gaspard, just a note that the patch that I posted this morning removes those lines anyway. Josh put them back in because of an Initializer test which called load_view_paths.

The initial purpose of load_view_paths was to eager load templates but since EagerPath has been folded into Path then there's no need for it as initialize_framework_views does the job now and doesn't stamp all over engine view paths.

Haven't looked at all the comments (i'm in a hurry right now) but commented on the commit an issue I'm having when using external template handlers: http://github.com/rails/rails/co...

LacKac, does this latest patch help?

Andrew, yes, it seems to work. Thanks.

I still have problems with this. Now it seems that it is overcaching in development (or maybe I'm not getting how this should work). First I thought it's JRuby or Haml, but I could reproduce in a fresh bare rails app run on MRI.

$ /path/to/edge/rails/bin/rails test
$ cd test
$ ln -s /path/to/edge/rails vendor/rails
$ script/generate scaffold Post title:string content:text
$ rake db:migrate
$ script/server

Now visit http://localhost:3000/posts/new and then change the title from "New post" to "Brand new post" in app/views/posts/new.html.erb. Reload in the browser and you will still see the original title. Only by restarting the server did I get the new text.

New version of my patch 0001-Add-reloadable-templates.patch: kicked out objectionable stuff from Dispatcher, reinstated _pick_partial_template cache that was removed by Andrews' patch and fixed failing tests.

Is there a chance that my version of the patch could be accepted? It does some stuff differently, mainly by creating ReloadableTemplate and ReloadablePath classes as I think that makes it much more clear what each part of the code does. I am also pretty certain it is faster and I don't want to be patching this stuff again in rails-dev-boost once 2.3 is out.

Changed whitespacing in ReloadableTemplate to be consistent with Rails' style.

Please comment on thedarkone's patch. Andrew what do you think?

Josh, this last patch seems to solve my problems. Haml templates work and the views are not overcached.

+1 for the darkone patch: 0001-Add-reloadable-templates.patch

Current edge requires a server restart to recompile the views in development mode (but they recompile when changed in production mode). The darkone patch fixes this problem.

Looks like we have a threadsafety issue: http://rails.lighthouseapp.com/p...

Here's a patch to fix the reloading when perform_caching is false. Not sure why it wasn't picked up by the test though.

I'm going over the latest patch from thedarkone will let you have my comments in a bit

This patch fixes both the reloading and threadsafe issues

I've had a look over thedarkone's patch and I do like the way it's implemented and the main reason I didn't do something similar was I wanted to try touch as little as possible so that it would be accepted for 2.3.

However, at the moment the way the testing is done is that virtually all of the tests are on EagerPath view paths rather than ReloadablePath view paths - only test_template_changes_are_reflected_without_cached_template_loading is actually tested using ReloadablePath. This is because the patch sets cache_template_loading to true in abstract_unit.

Since my changes reduce the Path implementations down to just a single one I feel that my changes have undergone more testing so before I can endorse thedarkone's patch I'd like to see him fix the attached test failures when running with cached_template_loading = false.

Thanks for the feedback Andrew :).

First of all, I am pretty sure neither of our patches are thread safe, both of them are also leaking memory (because of how ActionView works) and should be used in production only if reloadable templates are an absolutely necessity.

I changed tests to be run with EagerPaths, because that is how your tests run pretty much. To see what I mean, put something like this in abstract_unit.rb to truly simulate development mode:

ActionView::Template::Path.class_eval do
  def load!
    @paths = {}
  end
end

And watch things blow up.

Majority of tests that were failing with my patch were of this kind: assert_equal ActionView::Template::EagerPath, view_paths.first.class

There was also some funky stuff with relative paths like this ./.././././abc not being properly handled, I've fixed that.

I've also droped all the Dispatcher.to_prepare callback magic as it wouldn't notice prepend_view_paths calls on a class level.

I've kept cache_template_view set to true in abstract_unit.rb for faster running tests, but all tests are now passing for ReloadableTemplates as well.

0002-Add-reloadable-templates.patch is the new patch.

One more comment, right now the default value for cached_template_loading is false, I would argue that the default value should be true and that cache_classes = false would always flip it to false.

cached_template_loading = false by default: pros: template caching is fully configurable in enviroment files cons: existing apps would have to manually set cached_template_loading = true in production.rb on upgrade, I'm sure this will cause some problems :)

cached_template_loading = true by default: pros: upgrading to 2.3 requires no changes to production.rb cons: cache_classes = false will always flip it to false and you won't be able to have development mode with reloading classes, but cached templates (that's a retarded combination anyway)

Screwed by Lighthouse formatting yet again :(.

0003-Add-reloadable-templates.patch: restored dual (reloadable vs. cached) tests in test/template/render_test.rb that were removed by Andrew's patch.

These kinds of things are such a pain since there are so many edge cases and they all have to be tested manually.

1. Make sure templates reload in development mode
2. Make sure templates don't reload in production mode unless cache_template_loading is set to true
3. Test performance and memory leakage
4. Threadsafety

0003-Add-reloadable-templates.patch

railties fail

1) Failure: test_engine_controllers_should_have_their_view_path_set_when_loaded(TestPluginLoader)

[test/plugin_loader_test.rb:133:in `test_engine_controllers_should_have_their_view_path_set_when_loaded'
 mocha (0.9.5) lib/mocha/test_case_adapter.rb:69:in `__send__'
 mocha (0.9.5) lib/mocha/test_case_adapter.rb:69:in `run']:

<["./test/fixtures/plugins/engines/engine/app/views"]> expected but was <["test/fixtures/plugins/engines/engine/app/views"]>.

I've talked with Andrew and his only remaining objection to my patch is that it was rescueing from any errors while removing compiled methods like this:

def undef_my_compiled_methods!
 @compiled_methods.each {|comp_method|
ActionView::Base::CompiledTemplates.send(:remove_method, comp_method)
rescue nil}
 @compiled_methods.clear
end

That was necessary because compiled_templates_test.rb was kinda blunt about removing methods from CompiledTemplates. I rewrote the test and removed the rescue clause.

There is however some potential for breakage if a plugin tampers with CompiledTemplates without notifying ReloadableTemplate about its intentions. Andrew suggests adding a tracking mechanism to CompiledTemplates that would try to handle some cases. I'm arguing that a plugin author should handle any edge cases himself.

I'm posting from a new account, because Lighthouse is being a bitch and always errors out on me (since yesterday), if I try to access the ticket as thedarkone.

Josh, there should not be any problems with the plugin not working in dev or prod mode. It also should be very fast.

The memory leak I was talking about is about 2 issues:

1. leaks through symbols, due to methods compiled on CompiledTemplates (un-fixable due to how template compilation works),

2. leaks through deleted templates that are never accessed by a new/changed templates, i.e. if a controller is deleted and its templates are never accessed again, they still will remain in memory because everything is written to be super-lazy.

Reloadable templates are not thread safe and ActionController::Base.allow_concurency = true always disables template reloading.

0004-Add-reloadable-templates.patch applied.

Please test.

Maybe this is the wrong place for a newbie question but I tried apply this patch to edge rails in /vendor to fix the missing template error (#1712) but I get lots of errors like:

error: actionmailer/test/abstract_unit.rb: does not exist in index error: actionmailer/test/mail_service_test.rb: does not exist in index error: actionpack/lib/action_controller/rescue.rb: does not exist in index

after running "git am < 0004-Add-reloadable-templates.patch"

Hey AQ, I'm not familiar with #1712, but 0004-Add-reloadable-templates.patch has already been applied and is now a part of edge Rails: http://github.com/rails/rails/co...

Hey everyone, I'm still getting the MissingTemplate error using latest version of edge rails and latest version of Haml. Do I need to configure something in environment.rb? Where can I find the line of code that renders the template through Haml? Thanks

Just found a slight edge case.

If you alter the view paths in development mode using a string then when the class gets reloaded and the view paths are reprocessed blowing away the existing templates. The templates are recreated but since they don't know about the compiled methods then they aren't deleted and therefore aren't recompiled.

In production the classes are cached so the view paths are reprocessed so any template modifications are reloaded automatically.

Ouch, that's one ugly edge case :(.

0001-Fix-an-edge-case-where-we-might-encounter-some-rando.patch gives up on manually tracking compiled methods and just greps public_instance_methods for any suspects.

Can you please open a new ticket for related issues. I'm getting lost in this long thread ;)

New ticket: #2012

Don't you guys think you're introducing a bit too much new stuff here? 2.3 is supposed to be in a release candidate state.

We're trying to port an existing app that extends ActionView to 2.3 so we can start using 2.3 as soon as it hits a stable release. But ActionView moves so fast that this seems virtually impossible.

An api freeze for ActionView would help a lot with that.

Please don't get me wrong. I don't want to rain on anybody's work or party. And I love you guys for all the awesome work you're putting in here. :D

Sven, yeah it hasn't turned out so well. But most people considered the pervious development mode slowness and unacceptable bug.

This stuff + rack changes is really making this a rough RC process. Lucky for me, I'm in charge of both these areas :)

Yeah, and you're doing some damn awesome work in both of them :)

Sven, is it related to your adva_cms and how it allows for some templates to be dynamically changed in production?

The ReloadableTemplates patch, as it is right now, really goes out of its way to avoid touching anything related to how fully cached templates work in production. Almost nothing should have been changed between RC1 and master about how ActionView's default production environment works.

I didn't dig that deep, but couldn't you be using core's ReloadableTemplates in production by simply setting config.action_view.cache_template_loading = false?

Hey thedarkone,

thanks for the suggestions! Yeah, this is about our theme support in adva_cms.

I'm checking out your code right now. Your implementation actually looks better than what we've previously been trying, better encapsulated etc. So, of course this is pretty cool to have in core.

Of course I'd rather not want to turn off cache_template_loading for the whole app. We add a theme load path on a per controller instance/request basis. I'm thinking of just adding a ReloadablePath here. Reading your code I figure that even might work without any further modification. Unfortunately I can't really test things right now with existing tests because Webrat seems to be broken on edge, too. :/

Anyway. Thanks again!

Sven, I thought you might have problems - one of the things we left out was overriding remove_method in CompiledTemplates to track other plugins, etc. that tried to remove compiled template methods. I found your app via a github search but yours was the only one so we left it out.

I've been using this code for a few days now on my ecommerce app and its been stable so hopefully we've eliminated most of the regressions.

My app also uses per-request view paths and the way I handle it is call process_view_paths on each themes view path on startup and store them in a hash keyed by theme name. That way I don't need to directly deal with ReloadablePath or EagerPath - the cache_template_loading config will deal with it. The only issue is that I need to restart the app to see any new themes so if you want to support something like uploading of new themes you'd want to process the view path after uploading and unpacking the theme.

Also the performance enhancement in this patch is huge - previously it was taking about three seconds to render a page in my app as the views paths were constantly walked and now it's down to half a second. Even better is that with template reloading restored to production mode I can create and edit themes for my app in production mode so the productivity boost is even greater.

It's been a bit of rocky path over the past couple weeks but hopefully you'll agree it's been worth it.

(from [906aebceedb95d8caa6db6314bc90f605bdfaf2b]) Bring abstract_controller up to date with rails/master

Resolved all the conflicts since 2.3.0 -> HEAD. Following is a list of commits that could not be applied cleanly or are obviated with the abstract_controller refactor. They all need to be revisited to ensure that fixes made in 2.3 do not reappear in 3.0:

2259ecf368e6a6715966f69216e3ee86bf1a82a7 AR not available * This will be reimplemented with ActionORM or equivalent

06182ea02e92afad579998aa80144588e8865ac3 implicitly rendering a js response should not use the default layout [#1844 state:resolved] * This will be handled generically

893e9eb99504705419ad6edac14d00e71cef5f12 Improve view rendering performance in development mode and reinstate template recompiling in production [#1909 state:resolved] * We will need to reimplement rails-dev-boost on top of the refactor;

the changes here are very implementation specific and cannot be
cleanly applied. The following commits are implicated:

  199e750d46c04970b5e7684998d09405648ecbd4
  3942cb406e1d5db0ac00e03153809cc8dc4cc4db
  f8ea9f85d4f1e3e6f3b5d895bef6b013aa4b0690
  e3b166aab37ddc2fbab030b146eb61713b91bf55
  ae9f258e03c9fd5088da12c1c6cd216cc89a01f7
  44423126c6f6133a1d9cf1d0832b527e8711d40f

0cb020b4d6d838025859bd60fb8151c8e21b8e84 workaround for picking layouts based on wrong view_paths [#1974 state:resolved] * The specifics of this commit no longer apply. Since it is a two-line

commit, we will reimplement this change.

8c5cc66a831aadb159f3daaffa4208064c30af0e make action_controller/layouts pick templates from the current instance's view_paths instead of the class view_paths [#1974 state:resolved] * This does not apply at all. It should be trivial to apply the feature

to the reimplemented ActionController::Base.

87e8b162463f13bd50d27398f020769460a770e3 fix HTML fallback for explicit templates [#2052 state:resolved] * There were a number of patches related to this that simply compounded

each other. Basically none of them apply cleanly, and the underlying
issue needs to be revisited. After discussing the underlying problem
with Koz, we will defer these fixes for further discussion.

http://github.com/rails/rails/co...