Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2308413002:
Only update watches when directories change. (Closed)
DescriptionOnly update watches when directories changed.
Because updating all watches for a recursively watched directory is an
expensive operation (requires scanning the entire directory tree below
a recursive watch target) the code normally only performs this if a
directory is modified or some component of |target_| changes.
Because the target path is not present in |recursive_paths_by_watch_|,
the current implementation treats all inotify events in the target
directory as changes that require an expensive update to all recursive
watches, even if the event is only related to a non-directory file.
This patch addresses the issue by only forcing a global update if the
fired watch is both not in |recursive_paths_by_watch_| as well as not
the |target_| watch.
BUG=628223
TEST=base_unittests continues to pass
Manual testing confirms performance changes from O(n) to O(1) for
touching a file in a watched directory.
Committed: https://crrev.com/842221d3c80f2afd6fe3f6723b0b06d56fc6e541
Cr-Commit-Position: refs/heads/master@{#417872}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix logic in UpdateRecursiveWatches instead of adding new logic #Patch Set 3 : Remove unnecessary test changes #Patch Set 4 : formatting #Patch Set 5 : rebase #
Total comments: 4
Patch Set 6 : Addressed comments #Patch Set 7 : commit message updated #Patch Set 8 : Correctly set changed_dir #Messages
Total messages: 37 (19 generated)
dspaid@chromium.org changed reviewers: + nya@chromium.org
+nya, Here's an initial draft of one of the CLs to fix the performance issue with large Downloads DIR. I've done some manual testing and it looks good, but I plan to clean it up and add more tests tomorrow. This one is slightly more risky as its modifying the current behavior of the common FilePathWatcher, but as far as I can tell the new code should result in the same behavior as the old code but with O(1) time instead of O(n) time. With a larger refactor I could probably take this a step further and only update watches for subdirectory trees modified instead of scanning through all of them...
https://codereview.chromium.org/2308413002/diff/1/base/files/file_path_watche... File base/files/file_path_watcher_linux.cc (right): https://codereview.chromium.org/2308413002/diff/1/base/files/file_path_watche... base/files/file_path_watcher_linux.cc:537: if (!is_dir) Hmm, is this check insufficient?
https://codereview.chromium.org/2308413002/diff/1/base/files/file_path_watche... File base/files/file_path_watcher_linux.cc (right): https://codereview.chromium.org/2308413002/diff/1/base/files/file_path_watche... base/files/file_path_watcher_linux.cc:537: if (!is_dir) On 2016/09/05 10:48:09, Shuhei Takahashi wrote: > Hmm, is this check insufficient? Good catch. It should be, but observations suggest it isn't. I'll see if I can track down why, as this would certainly be more elegant.
On 2016/09/05 12:57:23, dspaid wrote: > https://codereview.chromium.org/2308413002/diff/1/base/files/file_path_watche... > File base/files/file_path_watcher_linux.cc (right): > > https://codereview.chromium.org/2308413002/diff/1/base/files/file_path_watche... > base/files/file_path_watcher_linux.cc:537: if (!is_dir) > On 2016/09/05 10:48:09, Shuhei Takahashi wrote: > > Hmm, is this check insufficient? > > Good catch. It should be, but observations suggest it isn't. I'll see if I can > track down why, as this would certainly be more elegant. Ok, after many iterations I think I've got it figured out. Turns out that because we don't add the root directory into the recursive_paths_by_watch_ map a change for a non-dir file would cause the global rebuild logic to happen. The solution is to do the is_dir check first (but also make sure we're not looking at an artificial call with an invalid watch).
The CQ bit was checked by dspaid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Only update watches when directories change. BUG=628223 ========== to ========== The current implementation causes the entire directory tree to be re-scanned every time a non-directory file in the root of the tree is modified. For large directories this can block the file thread for multiple seconds and cause very long queue times for events. This patch fixes the logic so that changes to non-directory files do not trigger global watch updates. BUG=628223 TEST=base_unittests continues to pass Manual testing confirms performance changes from O(n) to O(1) for touching a file in a watched directory. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2308413002/diff/80001/base/files/file_path_wa... File base/files/file_path_watcher_linux.cc (right): https://codereview.chromium.org/2308413002/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_linux.cc:515: bool is_dir) { Could you insert DCHECK(HasValidWatchVector()); here? https://codereview.chromium.org/2308413002/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_linux.cc:530: if (!ContainsKey(recursive_paths_by_watch_, fired_watch)) { Hmm, reordering the checks is fragile and I'm afraid future changes may cause regression unexpectedly. I prefer proper fix here: if (!ContainsKey(recursive_paths_by_watch_, fired_watch) && fired_watch != watches_.back().watch)
https://codereview.chromium.org/2308413002/diff/80001/base/files/file_path_wa... File base/files/file_path_watcher_linux.cc (right): https://codereview.chromium.org/2308413002/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_linux.cc:515: bool is_dir) { On 2016/09/08 05:10:52, Shuhei Takahashi wrote: > Could you insert DCHECK(HasValidWatchVector()); here? Done. https://codereview.chromium.org/2308413002/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_linux.cc:530: if (!ContainsKey(recursive_paths_by_watch_, fired_watch)) { On 2016/09/08 05:10:52, Shuhei Takahashi wrote: > Hmm, reordering the checks is fragile and I'm afraid future changes may cause > regression unexpectedly. I prefer proper fix here: > > if (!ContainsKey(recursive_paths_by_watch_, fired_watch) && > fired_watch != watches_.back().watch) Done.
lgtm, thanks!
dspaid@chromium.org changed reviewers: + dcheng@chromium.org
LGTM but please format the CL description to follow the usual conventions, with a short one-line description, followed by two newlines, and then a detailed description of the change, wrapped at ~72 chars: Only update watches when directories changed The current implementation... Two other thoughts about the description: 1) "non-directory file in the root of the tree is modified" --> do you mean a file in the target directory of the watcher? When the word 'root' is used here, it sounds like the filesystem root. 2) PLease add a clearer explanation of the issue to the CL description, something like: The watcher implementation is careful to only recursively updates watches for the target directory when there's a directory change underneath the target directory, because this is an expensive operation that requires walking the entire target directory. However, the logic for skipping the recursive updates didn't account for the fact that the actual target path is not in |recursive_paths_by_watch_|. Thus, a change to a file in the target directory would trigger the expensive recursive watch update.
Description was changed from ========== The current implementation causes the entire directory tree to be re-scanned every time a non-directory file in the root of the tree is modified. For large directories this can block the file thread for multiple seconds and cause very long queue times for events. This patch fixes the logic so that changes to non-directory files do not trigger global watch updates. BUG=628223 TEST=base_unittests continues to pass Manual testing confirms performance changes from O(n) to O(1) for touching a file in a watched directory. ========== to ========== Only update watches when directories changed. Because updating all watches for a recursively watched directory is an expensive operation (requires scanning the entire directory tree below a recursive watch target) the code normally only performs this if a directory is modified or some component of |target_| changes. Because the target path is not present in |recursive_paths_by_watch_|, the current implementation treats all inotify events in the target directory as changes that require an expensive update to all recursive watches, even if the event is only related to a non-directory file. This patch addresses the issue by only forcing a global update if the fired watch is both not in |recursive_paths_by_watch_| as well as not the |target_| watch. BUG=628223 TEST=base_unittests continues to pass Manual testing confirms performance changes from O(n) to O(1) for touching a file in a watched directory. ==========
On 2016/09/09 06:10:57, dcheng wrote: > LGTM but please format the CL description to follow the usual conventions, with > a short one-line description, followed by two newlines, and then a detailed > description of the change, wrapped at ~72 chars: > > Only update watches when directories changed > > The current implementation... > > Two other thoughts about the description: > 1) "non-directory file in the root of the tree is modified" --> do you mean a > file in the target directory of the watcher? When the word 'root' is used here, > it sounds like the filesystem root. > > 2) PLease add a clearer explanation of the issue to the CL description, > something like: > > The watcher implementation is careful to only recursively updates watches for > the target directory when there's a directory change underneath the target > directory, because this is an expensive operation that requires walking the > entire target directory. However, the logic for skipping the recursive updates > didn't account for the fact that the actual target path is not in > |recursive_paths_by_watch_|. Thus, a change to a file in the target directory > would trigger the expensive recursive watch update. Updated
LGTM, thanks!
The CQ bit was checked by dspaid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nya@chromium.org Link to the patchset: https://codereview.chromium.org/2308413002/#ps120001 (title: "commit message updated")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by dspaid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL Tests caught a minor regression.
lgtm
The CQ bit was checked by dspaid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nya@chromium.org Link to the patchset: https://codereview.chromium.org/2308413002/#ps140001 (title: "Correctly set changed_dir")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Only update watches when directories changed. Because updating all watches for a recursively watched directory is an expensive operation (requires scanning the entire directory tree below a recursive watch target) the code normally only performs this if a directory is modified or some component of |target_| changes. Because the target path is not present in |recursive_paths_by_watch_|, the current implementation treats all inotify events in the target directory as changes that require an expensive update to all recursive watches, even if the event is only related to a non-directory file. This patch addresses the issue by only forcing a global update if the fired watch is both not in |recursive_paths_by_watch_| as well as not the |target_| watch. BUG=628223 TEST=base_unittests continues to pass Manual testing confirms performance changes from O(n) to O(1) for touching a file in a watched directory. ========== to ========== Only update watches when directories changed. Because updating all watches for a recursively watched directory is an expensive operation (requires scanning the entire directory tree below a recursive watch target) the code normally only performs this if a directory is modified or some component of |target_| changes. Because the target path is not present in |recursive_paths_by_watch_|, the current implementation treats all inotify events in the target directory as changes that require an expensive update to all recursive watches, even if the event is only related to a non-directory file. This patch addresses the issue by only forcing a global update if the fired watch is both not in |recursive_paths_by_watch_| as well as not the |target_| watch. BUG=628223 TEST=base_unittests continues to pass Manual testing confirms performance changes from O(n) to O(1) for touching a file in a watched directory. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Only update watches when directories changed. Because updating all watches for a recursively watched directory is an expensive operation (requires scanning the entire directory tree below a recursive watch target) the code normally only performs this if a directory is modified or some component of |target_| changes. Because the target path is not present in |recursive_paths_by_watch_|, the current implementation treats all inotify events in the target directory as changes that require an expensive update to all recursive watches, even if the event is only related to a non-directory file. This patch addresses the issue by only forcing a global update if the fired watch is both not in |recursive_paths_by_watch_| as well as not the |target_| watch. BUG=628223 TEST=base_unittests continues to pass Manual testing confirms performance changes from O(n) to O(1) for touching a file in a watched directory. ========== to ========== Only update watches when directories changed. Because updating all watches for a recursively watched directory is an expensive operation (requires scanning the entire directory tree below a recursive watch target) the code normally only performs this if a directory is modified or some component of |target_| changes. Because the target path is not present in |recursive_paths_by_watch_|, the current implementation treats all inotify events in the target directory as changes that require an expensive update to all recursive watches, even if the event is only related to a non-directory file. This patch addresses the issue by only forcing a global update if the fired watch is both not in |recursive_paths_by_watch_| as well as not the |target_| watch. BUG=628223 TEST=base_unittests continues to pass Manual testing confirms performance changes from O(n) to O(1) for touching a file in a watched directory. Committed: https://crrev.com/842221d3c80f2afd6fe3f6723b0b06d56fc6e541 Cr-Commit-Position: refs/heads/master@{#417872} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/842221d3c80f2afd6fe3f6723b0b06d56fc6e541 Cr-Commit-Position: refs/heads/master@{#417872} |
