Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

File watchers and caching #6198

Merged
merged 105 commits into from
Jan 8, 2014
Merged

File watchers and caching #6198

merged 105 commits into from
Jan 8, 2014

Conversation

iwehrman
Copy link
Contributor

@iwehrman iwehrman commented Dec 6, 2013

This review-only PR supersedes #6052. Description still to be written; the text of #6052 is copied below for now.

CC @gruehle @peterflynn @jasonsanjose


This is the first of what will become a two-part pull request. The first part adds file caching with consistency checks using the existing coarse cache-invalidation strategy (i.e., invalidate everything on focus). The second part will use file watchers to implement a more fine-grained cache-invalidation strategy.

Notes:

  1. This pull request adds consistent cache support for all FileSystemEntry objects. In particular, stats are cached for FileSystemEntry objects, and additionally directory contents and children stat lists for Directory objects; and file contents for File objects.
  2. Data is only cached for watched entries. For simplicity, this is checked only at cache update time, and not at cache read time. As a consequence, cached data is used whenever it is available. Cached data is not purged when an entry transitions from a watched to an unwatched state because, in this case, the entry should no longer be accessed anyway. However, an entry can safely go from an unwatched to watched state, and so the watching infrastructure is responsible for calling _setWatched on each entry so that it knows to begin caching its data.
  3. The only time consistency checks happen are during file writes. In particular, calls to stat and read will immediately return cached data if available. This makes cached reads very fast. (On my machine, searches for occurrences of a single character in the entire Brackets project complete in under a second after data is cached.) Consistency is checked using a hash of the file's contents, which is provided by the impl. The only time the consistency hash is updated is when the contents of the are known: i.e., after reads and writes.
  4. A consistency check can fail during writes if the impl determines that the hash of the file on disk is different from the hash in memory. In that case, the write will fail with a new error code FileSystemError.CONTENTS_MODIFIED. (This error should never occur if the file is created by the write.) If a write is attempted to an existing file that has never been read (i.e., a blind write), the consistency check will fail because there is no hash in memory. This is a case in which the consistency check may be too strict, and so it is possible for clients to override the check by setting blind: true in the options parameter to the write call. There were no instances of blind writes in Brackets that required this override.
  5. If we fail to invalidate cached contents that have changed on disk then the user will not be notified of the change until she attempts to save. Brackets will not allow the new data on disk to be overwritten accidentally in this case because of the consistency check, but the experience would nonetheless be frustrating and disconcerting to the user. Timely cache invalidation is required to avoid this scenario. Consequently, this pull request should not be merged until file watchers can be used to perform this invalidation.

TODO 1/2/2014

  • unit test pass
  • startup slugishness on linux/windows for brackets and brackets-shell repos
  • project tree unhandled exception (try compiling brackets-shell to produce lots of change events)
  • scrub review comments from @peterflynn

Ian Wehrman added 30 commits November 26, 2013 14:20
…d a reminder to refresh watched paths upon domain reload
…chers are online for more consistent filtering
…oots AND if they pass the watchedRoot's filter function
@iwehrman
Copy link
Contributor Author

iwehrman commented Jan 3, 2014

The recent watching and caching changes seem to have improved Windows startup performance, so I'm checking that task off.

@gruehle
Copy link
Member

gruehle commented Jan 3, 2014

@iwehrman - yep, performance on Windows is much better with the latest changes. 👍

@RaymondLim
Copy link
Contributor

I'm getting different set of unit test failures on Mac. 5 JSQuitEdit and 4 Live Development tests fail. Below is the first error JSQuitEdit failures.

should open a function with form: function functionName()
Error: Expected '[rewriteProject] promise rejected with: undefined' to be null.
at new jasmine.ExpectationResult (file://localhost/Users/rlim/brackets/brackets/test/thirdparty/jasmine-core/jasmine.js:102:32)
at null.toBe (file://localhost/Users/rlim/brackets/brackets/test/thirdparty/jasmine-core/jasmine.js:1194:29)
at Object. (file://localhost/Users/rlim/brackets/brackets/test/spec/SpecRunnerUtils.js:172:77)
at l (file://localhost/Users/rlim/brackets/brackets/src/thirdparty/jquery-2.0.1.min.js:4:24797)
at Object.c.fireWith (file://localhost/Users/rlim/brackets/brackets/src/thirdparty/jquery-2.0.1.min.js:4:25618)
at Object.i.(anonymous function) as reject
at Object. (file://localhost/Users/rlim/brackets/brackets/src/extensions/default/JavaScriptQuickEdit/unittests.js:59:20)
at l (file://localhost/Users/rlim/brackets/brackets/src/thirdparty/jquery-2.0.1.min.js:4:24797)
at Object.c.fireWith (file://localhost/Users/rlim/brackets/brackets/src/thirdparty/jquery-2.0.1.min.js:4:25618)
at Object.i.(anonymous function) as reject

jasonsanjose and others added 18 commits January 6, 2014 12:05
maintain selection in refreshFileTree
Share temp project for ProjectManager-test. Code cleanup.
…s when no added or removed files are present.
Fix selection update after fs change event
…of the project root, but when we do refresh the file tree, also clear the change queue (using a new PromiseQueue.removeAll method) and use a debounced refreshFileTree call
… watcher notifications and increase timeouts
@jasonsanjose
Copy link
Member

Reviewed and confirmed unit tests failures resolved.

jasonsanjose added a commit that referenced this pull request Jan 8, 2014
[REVIEW ONLY] File watchers and caching
@jasonsanjose jasonsanjose merged commit 5ded6ae into master Jan 8, 2014
@gruehle
Copy link
Member

gruehle commented Jan 8, 2014

@njx
Copy link
Contributor

njx commented Jan 8, 2014

image

@iwehrman
Copy link
Contributor Author

iwehrman commented Jan 8, 2014

"dance"

@njx njx deleted the iwehrman/file-watchers branch May 19, 2014 17:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants