Skip to content

Commit

Permalink
Removed config.action_view.cache_template_loading, use config.cache_c…
Browse files Browse the repository at this point in the history
…lasses instead
  • Loading branch information
josh committed Jul 16, 2008
1 parent 5cc3ea6 commit 83e29b9
Show file tree
Hide file tree
Showing 8 changed files with 20 additions and 11 deletions.
2 changes: 2 additions & 0 deletions actionpack/CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
*Edge*

* Removed config.action_view.cache_template_loading, use config.cache_classes instead [Josh Peek]

* Get buffer for fragment cache from template's @output_buffer [Josh Peek]

* Set config.action_view.warn_cache_misses = true to receive a warning if you perform an action that results in an expensive disk operation that could be cached [Josh Peek]
Expand Down
7 changes: 4 additions & 3 deletions actionpack/lib/action_view/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,10 @@ class << self
delegate :erb_trim_mode=, :to => 'ActionView::TemplateHandlers::ERB'
end

# Specify whether templates should be cached. Otherwise the file we be read everytime it is accessed.
@@cache_template_loading = false
cattr_accessor :cache_template_loading
def self.cache_template_loading=(*args)
ActiveSupport::Deprecation.warn("config.action_view.cache_template_loading option has been deprecated and has no affect. " <<
"Please remove it from your config files.", caller)
end

def self.cache_template_extensions=(*args)
ActiveSupport::Deprecation.warn("config.action_view.cache_template_extensions option has been deprecated and has no affect. " <<
Expand Down
16 changes: 12 additions & 4 deletions actionpack/lib/action_view/paths.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ def self.type_cast(obj)
end

class Path #:nodoc:
def self.eager_load_templates!
@eager_load_templates = true
end

def self.eager_load_templates?
@eager_load_templates || false
end

attr_reader :path, :paths
delegate :to_s, :to_str, :inspect, :to => :path

Expand All @@ -37,6 +45,9 @@ def reload!
@paths = {}

templates_in_path do |template|
# Eager load memoized methods and freeze cached template
template.freeze if self.class.eager_load_templates?

@paths[template.path] = template
@paths[template.path_without_extension] ||= template
end
Expand All @@ -48,10 +59,7 @@ def reload!
def templates_in_path
(Dir.glob("#{@path}/**/*/**") | Dir.glob("#{@path}/**")).each do |file|
unless File.directory?(file)
template = Template.new(file.split("#{self}/").last, self)
# Eager load memoized methods and freeze cached template
template.freeze if Base.cache_template_loading
yield template
yield Template.new(file.split("#{self}/").last, self)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/renderable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def #{render_symbol}(local_assigns)
# if local_assigns has a new key, which isn't supported by the compiled code yet.
def recompile?(symbol)
meth = Base::CompiledTemplates.instance_method(template.method) rescue nil
!(meth && Base.cache_template_loading)
!(meth && frozen?)
end
end
end
1 change: 0 additions & 1 deletion actionpack/test/abstract_unit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
ActionController::Base.logger = nil
ActionController::Routing::Routes.reload rescue nil

ActionView::Base.cache_template_loading = true
FIXTURE_LOAD_PATH = File.join(File.dirname(__FILE__), 'fixtures')
ActionController::Base.view_paths = FIXTURE_LOAD_PATH

Expand Down
1 change: 0 additions & 1 deletion railties/environments/production.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
# Full error reports are disabled and caching is turned on
config.action_controller.consider_all_requests_local = false
config.action_controller.perform_caching = true
config.action_view.cache_template_loading = true

# Use a different cache store in production
# config.cache_store = :mem_cache_store
Expand Down
1 change: 1 addition & 0 deletions railties/lib/initializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,7 @@ def initialize_framework_logging
# paths have already been set, it is not changed, otherwise it is
# set to use Configuration#view_path.
def initialize_framework_views
ActionView::PathSet::Path.eager_load_templates! if configuration.cache_classes
ActionMailer::Base.template_root ||= configuration.view_path if configuration.frameworks.include?(:action_mailer)
ActionController::Base.view_paths = [configuration.view_path] if configuration.frameworks.include?(:action_controller) && ActionController::Base.view_paths.empty?
end
Expand Down
1 change: 0 additions & 1 deletion railties/lib/performance_test_help.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
require 'action_controller/performance_test'

ActionController::Base.perform_caching = true
ActionView::Base.cache_template_loading = true
ActiveSupport::Dependencies.mechanism = :require
Rails.logger.level = ActiveSupport::BufferedLogger::INFO

17 comments on commit 83e29b9

@acechase
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, as of this commit, is there no longer a way to have view code recompile without recompiling classes as well?

@jeremy
Copy link
Member

@jeremy jeremy commented on 83e29b9 Dec 12, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could turn off ActionView::PathSet::Path.eager_load_templates by hand.

This was so rarely used that it because configuration noise.

@eric1234
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m surprised at this also! I know Rails tries to encourage best practices so I don’t mind that the templates do not reload by default because best practices dictate you should only update production templates via a deployment system like Capistrano that will restart the application when done with the template update.

But not everybody lives in that ideal world. I work with a lot of people coming from the static HTML and PHP world where they are used to being able to make simple text changes on the production server and not need a “professional” to make every text change. They assume the risk associated with this style of development to save on costs. Restarting the application after every text change is out of their league also as they are not comfortable with the command line (they update the files via FTP manually). Obviously Rails should encourage people away from this style of development but I have always found it useful that this style of development was possible. I don’t even mind changing a config value to enable it.

This change (6 months old but not all of us live on the edge) now means that if I choose to allow this style of development I am also punished into having ALL of my classes reload on every request! Old versions of Rails where smart enough to check timestamps and only reload what needs to be reloaded. I consider this a step backwards.

Just my two cents.

@josh
Copy link
Contributor Author

@josh josh commented on 83e29b9 Dec 30, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eric I kind of want that functionality back too, but its a bit more difficult now

PID :)

@NZKoz
Copy link
Member

@NZKoz NZKoz commented on 83e29b9 Dec 30, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe he means PDI ;)

anyway, you can use passenger and just touch tmp/restart.txt in the meantime, it’s not some huge ordeal like restarting a monit-managed mongrel-farm

@dhh
Copy link
Member

@dhh dhh commented on 83e29b9 Dec 30, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d like this back as well. It’s very useful when restarting your app is expensive. Say when you’re dealing with 200 req/sec in prime time and your visitors would feel slowdown if everything was bounced.

@eric1234
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josh, I don’t understand why it would be so difficult to restore the functionality.

Seems to me that we could just add “config.action_view.cache_template_loading” back. If true (the default to encourage best practices) then updates to templates do not affect the running application. If false then it would still cache the compiled template (for performance) but prior to running it, a quick check of the template’s last modified time-stamp would be done. If the template is newer than the cached value in memory then invalidate the cache and re-compile the template. Otherwise just use the compiled template already in memory.

So developer have a choice of “super-fast but inflexible (and if you are using standard deployment techniques this inflexibility won’t get in your way)” or “fast (since only changed templates are re-processed) but flexible”.

@josh
Copy link
Contributor Author

@josh josh commented on 83e29b9 Dec 30, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eric You make it sound so easy :)

If you can make a patch, I’d be happy to apply it.

@csmuc
Copy link
Contributor

@csmuc csmuc commented on 83e29b9 Feb 3, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another vote for bringing this back. The need for a full restart is just too painful for small changes on the views and reduces the overall availability of the app. At least this is my experience.

@lifo
Copy link
Member

@lifo lifo commented on 83e29b9 Feb 3, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we all agree it should be back. So at this point, we probably need a patch and not only votes :)

@alloy
Copy link
Contributor

@alloy alloy commented on 83e29b9 Feb 3, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 lifo ;)

@thedarkone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this exact “timestamping-on-change-reloading-templates” implemented in my fork of rails-dev-boost (the plugin patches a few things to get production like speeds in dev mode).

It is implemented against Rails 2.2 APIs though… I’ll have a look how portable it is for the 2.3 branch.

@akia
Copy link

@akia akia commented on 83e29b9 Feb 4, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for bringing this back :)

@thedarkone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the plugin to be compatible with Rails 2.3.

The implementation is obviously not thread safe… after all I only care about the speedy development mode.

@pixeltrix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch for bringing it back:
http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/1909

Comments appreciated!

@turlockmike
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed? This really increases my development time. I have a programmer who only modifies erb code and this is super annoying to have requests that take 3-4 seconds. Please reconsider adding this back in 3.1.

@josevalim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is already in 3.1

Please sign in to comment.