config.reload_classes_only_on_change doesn't honor model mtimes #9496

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

tonyla commented Mar 1, 2013

Relating to Rails 3-2-stable

I believe the config.reload_classes_only_on_change option does not act as expected.

Code starting here:
https://github.com/rails/rails/blob/3-2-stable/railties/lib/rails/application/finisher.rb#L79

if config.reload_classes_only_on_change
  reloader = config.file_watcher.new(*watchable_args, &callback)
  self.reloaders << reloader
  # We need to set a to_prepare callback regardless of the reloader result, i.e.
  # models should be reloaded if any of the reloaders (i18n, routes) were updated.
  ActionDispatch::Reloader.to_prepare(:prepend => true){ reloader.execute }
else
  ActionDispatch::Reloader.to_cleanup(&callback)
end

Is supposed to only reload dependencies if the mtimes of files have changed. That is what the class ActiveSupport::FileUpdateChecker provides as on of the default reloaders.

But ActionDispatch::Reloader.to_prepare(:prepend => true){ reloader.execute } that line will reload all the dependencies regardless if mtimes.

def execute
https://github.com/rails/rails/blob/3-2-stable/activesupport/lib/active_support/file_update_checker.rb#L76

I believe that line should be

ActionDispatch::Reloader.to_prepare(:prepend => true){ reloader.execute_if_updated }

def execute_if_updated
https://github.com/rails/rails/blob/3-2-stable/activesupport/lib/active_support/file_update_checker.rb#L84

This way your application code is only reloaded if it has been updated.

Application constants being cleared on every request in development mode
ActiveSupport::FileUpdateChecker#execute clears all constants regardless
of mtimes this causes a huge perf hit in development mode which
ActiveSupport::FileUpdateChecker was supposed to fix.

Instead the more correct call here would be
ActiveSupport::FileUpdateChecker#execute_if_updated which only reloads
if the mtimes on files have changed.
Member

schneems commented Mar 3, 2013

@tonyla is it possible to test this functionality, maybe creating two files, touching one and making sure only the one touched is updated in the reloader? I agree with your logic, but want this functionality tested.

Does the reloader behave the same in master?

tonyla commented Mar 6, 2013

@schneems

The reloader functionality is tested in this file https://github.com/rails/rails/blob/3-2-stable/activesupport/test/file_update_checker_test.rb

Does that cover what you were thinking? Or are you meaning a test that tests the reloader in context? Meaning testing of actual file reloading inside a rails environment.

Master does behave the exact same way:
https://github.com/rails/rails/blob/master/railties/lib/rails/application/finisher.rb#L87

As an anecdote this reduces my dev page loads from ~4s to < 1s. The first page load still takes 4s but each subsequent page load is < 1s. Has made a huge different in development speed in the fairly large rails app I work with.

Contributor

stouset commented Mar 26, 2013

This seems like a pretty important thing to fix for Rails 4 given other improvements in getting the dev environment to load quickly. What else is necessary for this to get fixed? Tests? A patch?

tonyla commented Mar 26, 2013

I can create what ever is needed, I'm just unsure of how to get this actually "approved".

I've been using this on a forked version of rails and it speeds up development significantly.

https://github.com/pingg-corp/rails/tree/v3.2.12-patched

Contributor

stouset commented Mar 26, 2013

@tonyla is it possible to test this functionality, maybe creating two files, touching one and making sure only the one touched is updated in the reloader? I agree with your logic, but want this functionality tested.

You've obviously got a patch. I'd make a pull request referencing this issue with the relevant changes, plus tests.

tonyla commented Mar 27, 2013

I did some more digging and found out that the reloading behaviour was being caused by wavii/rails-dev-tweaks#9 (A gem that I use). I'll detail the explanation below just in case it helps someone in the future.

The code does behave as expected but it is very deceiving when reading it. That is because the line

ActionDispatch::Reloader.to_prepare(:prepend => true){ reloader.execute }

Looks like it will reload on every request. But reloaders are not actually run unless #reload_dependencies? is true.

    def reload_dependencies? #:nodoc:
      config.reload_classes_only_on_change != true || reloaders.map(&:updated?).any?
    end

As you can see it checks to see if any reloader #updated?. It will not reload the code on ever request.

@tonyla tonyla closed this Mar 27, 2013

Owner

rafaelfranca commented Mar 27, 2013

@josevalim could you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment