Skip to content

Modified the updater function to cache the file list as directory traversal is super slow #7745

Closed
wants to merge 2 commits into from

10 participants

@simonjsmithuk

This now occurs after 5 seconds have elapsed, rather than on every call.

@simonjsmithuk simonjsmithuk Added caching of asset list so that asset folders are only researched…
… after 5 seconds have elapsed, rather than on every call.
10ee26b
@fxn
Ruby on Rails member
fxn commented Sep 24, 2012
@josevalim
Ruby on Rails member

I guess this is fine, but the proposed code could be refactored (the if clauses are the same) and moved to its own method.

@fxn
Ruby on Rails member
fxn commented Sep 24, 2012

Was wondering... couldn't we just skip this check for asset requests instead of adding a magic window of 5 seconds?

@tenderlove
Ruby on Rails member

Just curious, I wonder what the value of @glob typically is. We could possibly optimize the glob pattern as well.

@schneems
Ruby on Rails member

Can you add some inline comments explaining why you are implementing this behavior? It might be confusing to come across this code months/years from now and not understand the use-case. A line or two would be fine.

While this seeks to speed things up on windows it will be applied across all systems. Are there potential race conditions where one might be able to add a file and refresh in under 5 seconds?

@simonjsmithuk

It will improve performance on all systems, Windows just happens to have the slowest filesystem so it is most obvious on Windows. In my original post I pointed out that I was concerned about the thread safety of this. In particular what happens in the case of multiple simultaneous writes to the @dir_cache? Is a synchronize block needed around this, or is the code already single threaded? I do not know the rails internals well enough to say for certain what the case is in all environments.

@glob contains the list of folders that can contain the assets. This is passed to Dir which parses the folder list into a complete list of files by searching the folders. Folders can and do contain wildcard folders too causing whole tree to be searched. This is the slow call. The actual checking of the timestamps once you have the file names is surprisingly fast.

@schneems
Ruby on Rails member
schneems commented Oct 4, 2012

Thanks for adding the comments @simonjsmithuk, what does everyone else think of this change?

@tenderlove tenderlove commented on the diff Nov 28, 2012
activesupport/lib/active_support/file_update_checker.rb
@@ -45,6 +45,9 @@ def initialize(files, dirs={}, &block)
@last_watched = watched
@last_update_at = updated_at(@last_watched)
+
+ @check_new_at = nil
@tenderlove
Ruby on Rails member
tenderlove added a note Nov 28, 2012

Can you change this nil to Time.now - 5?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove tenderlove commented on the diff Nov 28, 2012
activesupport/lib/active_support/file_update_checker.rb
@@ -93,15 +96,25 @@ def execute_if_updated
def watched
@watched || begin
all = @files.select { |f| File.exists?(f) }
- all.concat(Dir[@glob]) if @glob
- all
+
+ #Dir[@glob] can be very slow to generate its file list, since asset requests usually come in bursts i.e. several assets loaded as
+ #after a page is loaded this can cause loading to be very slow. Updating this file list only once every 5 seconds improves speed
+ #dramatically on Windows, where the problem is most obvious, and has no noticable effect on the flexibility to add/remove assets
+ #while in development and have the cache update.
+
+ if @check_new_at.nil? || Time.now > @check_new_at
@tenderlove
Ruby on Rails member
tenderlove added a note Nov 28, 2012

Then remove the nil? check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove tenderlove commented on the diff Nov 28, 2012
activesupport/lib/active_support/file_update_checker.rb
end
end
def updated_at(paths)
@updated_at || max_mtime(paths) || Time.at(0)
end
-
+
@tenderlove
Ruby on Rails member
tenderlove added a note Nov 28, 2012

Remove this whitespace diff please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove
Ruby on Rails member

I think it's fine. Can you make the adjustments I've commented, and then we can merge?

@fxn
Ruby on Rails member
fxn commented Nov 28, 2012

Not convinced by this proposal.

Since the beginning of Rails you edit your code and the change is reflected. This window introduces a little uncertainty that I doubt is worth the trouble.

The current approach improved the previous one in 3.2 and people loved it.

@fxn
Ruby on Rails member
fxn commented Nov 28, 2012

I wonder... the natural step forward here would be to support some kind of file system notification. Maybe optional, and maybe only in Unix.

What I envision is: if you have this installed, or you set this flag, something external and optional like that... the watcher will use such notification mechanism to know when things have changed.

@simonjsmithuk
@senny
Ruby on Rails member
senny commented Mar 17, 2013

@fxn @tenderlove how should we proceed on this one?

@ekampp
ekampp commented Mar 22, 2013

As I read it, it would take up to five seconds for my file structure changes to be reflected? Is that a correct interpretation?

@estsauver

@fxn I think it might be reasonable to implement a file system notification interface for windows. I think the problem is that the unix systems are able to use sys calls and it looks like the Dir.c file is making calls out to a win32.c wrappers, which I'm really having a hard time understanding what exactly is going on in that function. (My C isn't amazing.)

If we did decide to build a file system notification layer for Rails, it seems that there is at least an api for watching a windows directory and its subtree: http://msdn.microsoft.com/en-us/library/windows/desktop/aa364417(v=vs.85).aspx

@rafaelfranca
Ruby on Rails member

I'm closing this one mostly because #7745 (comment).

I believe adding a delay for the file updater check, even if small will make the overall development workflow worst in the most cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.