Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2310833002:
Don't block FILE thread for media scanning (Closed)
|
Created:
4 years, 3 months ago by dspaid Modified:
4 years, 3 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't block FILE thread for media scanning
Only perform media scanning if a scan has not been performed since the file update event.
Building up the timestamp map is also moved off the FILE thread and on to the blocking pool.
BUG=628223
TEST=create a large (~10k) set of files under /home/chronos/user/Downloads
Enable ARC++
create a profiler snapshot at chrome://profiler
create one more new file under Downloads
Create another snapshot in chrome://profiler
Verify that there are no long-running tasks on the FILE thread, and that DelayBuildTimestampMap is only called once.
Committed: https://crrev.com/d8b3cb93177c7223bc67bce7b823083a380b19c2
Cr-Commit-Position: refs/heads/master@{#418778}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Move logic back to FILE thread #
Total comments: 1
Patch Set 3 : Move only TimestampMap creation logic to blocking pool #
Total comments: 19
Patch Set 4 : Addressed comments #
Total comments: 2
Patch Set 5 : Addressed comments #Patch Set 6 : Add a delayed callback before building the timestamp map #Patch Set 7 : comments #
Total comments: 8
Patch Set 8 : addressed comments #
Total comments: 13
Patch Set 9 : Addressed Comments #Patch Set 10 : Addressed Comments #
Total comments: 5
Patch Set 11 : Update comment #Patch Set 12 : Revert change from FilePath::Append #Patch Set 13 : Use AppendRelativePath (again) #Messages
Total messages: 47 (21 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.
Thanks Dan, here are some quick comments. https://codereview.chromium.org/2310833002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:159: weak_ptr_factory_.GetWeakPtr(), base::Time::Now())); Weak pointers can be dereferenced only on FILE thread, but this callback would cause it to be dereferenced on the blocking pool task runner. I guess DCHECK will fail. See: https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?l=49 https://codereview.chromium.org/2310833002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:164: if (notify_time < last_update_time_) { Please add a comment about the purpose of this check and how it works, this is a bit tricky :) https://codereview.chromium.org/2310833002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:176: callback_.Run(changed_paths); |callback_| should be run on FILE thread.
https://codereview.chromium.org/2310833002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:159: weak_ptr_factory_.GetWeakPtr(), base::Time::Now())); On 2016/09/05 09:28:23, Shuhei Takahashi wrote: > Weak pointers can be dereferenced only on FILE thread, but this callback would > cause it to be dereferenced on the blocking pool task runner. I guess DCHECK > will fail. > > See: https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?l=49 Thanks for the reminder. I couldn't really find a thread safe way to get this working so I've moved it back to the FILE thread. However the rest of the optimizations should still keep it from getting too bad. https://codereview.chromium.org/2310833002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:164: if (notify_time < last_update_time_) { On 2016/09/05 09:28:23, Shuhei Takahashi wrote: > Please add a comment about the purpose of this check and how it works, this is a > bit tricky :) Done. https://codereview.chromium.org/2310833002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:176: callback_.Run(changed_paths); On 2016/09/05 09:28:23, Shuhei Takahashi wrote: > |callback_| should be run on FILE thread. Looks like this was fixed in a recent CL by hashimoto@
Description was changed from ========== Don't block FILE thread for media scanning BUG=628223 ========== to ========== Only perform media scanning if a scan has not been performed since the file update event. BUG=628223 ==========
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Drive-by https://codereview.chromium.org/2310833002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:165: TimestampMap current_timestamp_map = BuildTimestampMap(); For the record. As we chatted offline, because the heavy operation still blocks FILE thread, can we move the heaviest part of this function into WorkerPool, with keeping timestamp check in FILE thread? Something like... OnFilePathChanged() { if (is_build_timestamp_map_running) { // Suspend the task. last_on_file_path_changed_time = Time::Now(); return; } PostTaskAndReply(..., Bind(HandleFilePathChanged, ...), Bind(OnBuildTimestampMap, GetWeakPtr()); } HandleFilePathChanged(...) { Time update_time = Time::Now(); ... heavy operation ... return pair(update_time, timestamp_map); } OnBuildTimestampMap(update_time, timestamp_map) { last_timestamp_map_ = timestamp_map; if (last_on_file_path_changed_ > update_time) { // During the map building, another change had come. Re-run. PostTask(...); } }
On 2016/09/07 01:44:32, hidehiko wrote: > Drive-by > > https://codereview.chromium.org/2310833002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): > > https://codereview.chromium.org/2310833002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:165: TimestampMap > current_timestamp_map = BuildTimestampMap(); > For the record. > > As we chatted offline, because the heavy operation still blocks FILE thread, can > we move the heaviest part of this function into WorkerPool, with keeping > timestamp check in FILE thread? Something like... > > OnFilePathChanged() { > if (is_build_timestamp_map_running) { > // Suspend the task. > last_on_file_path_changed_time = Time::Now(); > return; > } > PostTaskAndReply(..., > Bind(HandleFilePathChanged, ...), > Bind(OnBuildTimestampMap, GetWeakPtr()); > } > > HandleFilePathChanged(...) { > Time update_time = Time::Now(); > ... heavy operation ... > return pair(update_time, timestamp_map); > } > > OnBuildTimestampMap(update_time, timestamp_map) { > last_timestamp_map_ = timestamp_map; > if (last_on_file_path_changed_ > update_time) { > // During the map building, another change had come. Re-run. > PostTask(...); > } > } Ok, I think the newest revision roughly does what we want. Manual testing indicates that it works as expected.
Description was changed from ========== Only perform media scanning if a scan has not been performed since the file update event. BUG=628223 ========== to ========== Only perform media scanning if a scan has not been performed since the file update event. Building up the timestamp map is also moved off the FILE thread and on to the blocking pool. BUG=628223 ==========
https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:100: return std::pair<base::Time, TimestampMap>(update_time, FYI, std::make_pair is useful to avoid explicit template parameters: http://www.cplusplus.com/reference/utility/make_pair/ https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:101: current_timestamp_map); IIUC you need std::move() here to avoid copy. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:120: // Wrapper around HandleFilePathChanged to pass the notification timestamp. This line is outdated. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:126: void HandleFilePathChanged(base::Time notify_time); This function is no longer defined. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:136: base::Time last_update_time_; How about using "bool pending_task_" instead? Though the logic will change slightly, efficiency is almost same, and the code will get easier to read. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:201: TimestampMap current_timestamp_map = timestamp_and_map.second; Could you insert DCHECK_CURRENTLY_ON? https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:201: TimestampMap current_timestamp_map = timestamp_and_map.second; Also, could you insert DCHECK(outstanding_task_)? https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:221: outstanding_task_ = false; We must keep |outstanding_task_| true when there was a pending task and PostTaskAndReplyWithResult() was called.
https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:100: return std::pair<base::Time, TimestampMap>(update_time, On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > FYI, std::make_pair is useful to avoid explicit template parameters: > http://www.cplusplus.com/reference/utility/make_pair/ Done. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:101: current_timestamp_map); On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > IIUC you need std::move() here to avoid copy. Done. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:120: // Wrapper around HandleFilePathChanged to pass the notification timestamp. On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > This line is outdated. Done. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:126: void HandleFilePathChanged(base::Time notify_time); On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > This function is no longer defined. Done. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:136: base::Time last_update_time_; On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > How about using "bool pending_task_" instead? Though the logic will change > slightly, efficiency is almost same, and the code will get easier to read. The small timing difference actually has a reasonable performance gain since we receive up to 3 events per action almost simultaneously. With the timestamp implementation these should be batched up into a single call in most cases, whereas the boolean implementation would guarantee two full scans per action. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:201: TimestampMap current_timestamp_map = timestamp_and_map.second; On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > Could you insert DCHECK_CURRENTLY_ON? Done. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:201: TimestampMap current_timestamp_map = timestamp_and_map.second; On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > Also, could you insert DCHECK(outstanding_task_)? Done. https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:221: outstanding_task_ = false; On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > We must keep |outstanding_task_| true when there was a pending task and > PostTaskAndReplyWithResult() was called. Done.
https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:136: base::Time last_update_time_; On 2016/09/08 03:59:56, dspaid wrote: > On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > > How about using "bool pending_task_" instead? Though the logic will change > > slightly, efficiency is almost same, and the code will get easier to read. > > The small timing difference actually has a reasonable performance gain since we > receive up to 3 events per action almost simultaneously. With the timestamp > implementation these should be batched up into a single call in most cases, > whereas the boolean implementation would guarantee two full scans per action. I see. But then I think it's a bit optimistic to assume such three events occur before the callback is invoked. How about inserting some delay (1000ms for example) before the callback? https://codereview.chromium.org/2310833002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:98: base::Time update_time = base::Time::Now(); |last_update_time_| and |update_time| sounds a bit confusing, they are actually different kind of timestamps. How about |last_notify_time_| and |snapshot_time|, for example?
https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:136: base::Time last_update_time_; On 2016/09/08 04:25:48, Shuhei Takahashi wrote: > On 2016/09/08 03:59:56, dspaid wrote: > > On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > > > How about using "bool pending_task_" instead? Though the logic will change > > > slightly, efficiency is almost same, and the code will get easier to read. > > > > The small timing difference actually has a reasonable performance gain since > we > > receive up to 3 events per action almost simultaneously. With the timestamp > > implementation these should be batched up into a single call in most cases, > > whereas the boolean implementation would guarantee two full scans per action. > > I see. But then I think it's a bit optimistic to assume such three events occur > before the callback is invoked. How about inserting some delay (1000ms for > example) before the callback? Experimentally I'm seeing a reasonable success rate (~68% of operations with 3 events are being deduped into a single update). I was looking for something like PostDelayedTask to introduce delay, but there doesn't seem to be anything to use with PostTaskAndReplyWithResult semantics. I'm generally skeptical of solutions that involve directly calling sleep, but am not sure what the common practice in chromium is. https://codereview.chromium.org/2310833002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:98: base::Time update_time = base::Time::Now(); On 2016/09/08 04:25:48, Shuhei Takahashi wrote: > |last_update_time_| and |update_time| sounds a bit confusing, they are actually > different kind of timestamps. How about |last_notify_time_| and |snapshot_time|, > for example? Done.
https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:136: base::Time last_update_time_; On 2016/09/09 02:10:45, dspaid wrote: > On 2016/09/08 04:25:48, Shuhei Takahashi wrote: > > On 2016/09/08 03:59:56, dspaid wrote: > > > On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > > > > How about using "bool pending_task_" instead? Though the logic will change > > > > slightly, efficiency is almost same, and the code will get easier to read. > > > > > > The small timing difference actually has a reasonable performance gain since > > we > > > receive up to 3 events per action almost simultaneously. With the timestamp > > > implementation these should be batched up into a single call in most cases, > > > whereas the boolean implementation would guarantee two full scans per > action. > > > > I see. But then I think it's a bit optimistic to assume such three events > occur > > before the callback is invoked. How about inserting some delay (1000ms for > > example) before the callback? > > Experimentally I'm seeing a reasonable success rate (~68% of operations with 3 > events are being deduped into a single update). I was looking for something > like PostDelayedTask to introduce delay, but there doesn't seem to be anything > to use with PostTaskAndReplyWithResult semantics. I'm generally skeptical of > solutions that involve directly calling sleep, but am not sure what the common > practice in chromium is. It is best if we have PostDelayedTaskAndReplyWithResult, but it apparently does not exist. Maybe you can define an intermediate callback function and calling PostDelayedTask and PostTaskAndReplyWithResult in order?
On 2016/09/09 03:18:51, Shuhei Takahashi wrote: > https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): > > https://codereview.chromium.org/2310833002/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:136: base::Time > last_update_time_; > On 2016/09/09 02:10:45, dspaid wrote: > > On 2016/09/08 04:25:48, Shuhei Takahashi wrote: > > > On 2016/09/08 03:59:56, dspaid wrote: > > > > On 2016/09/08 03:43:48, Shuhei Takahashi wrote: > > > > > How about using "bool pending_task_" instead? Though the logic will > change > > > > > slightly, efficiency is almost same, and the code will get easier to > read. > > > > > > > > The small timing difference actually has a reasonable performance gain > since > > > we > > > > receive up to 3 events per action almost simultaneously. With the > timestamp > > > > implementation these should be batched up into a single call in most > cases, > > > > whereas the boolean implementation would guarantee two full scans per > > action. > > > > > > I see. But then I think it's a bit optimistic to assume such three events > > occur > > > before the callback is invoked. How about inserting some delay (1000ms for > > > example) before the callback? > > > > Experimentally I'm seeing a reasonable success rate (~68% of operations with 3 > > events are being deduped into a single update). I was looking for something > > like PostDelayedTask to introduce delay, but there doesn't seem to be anything > > to use with PostTaskAndReplyWithResult semantics. I'm generally skeptical of > > solutions that involve directly calling sleep, but am not sure what the common > > practice in chromium is. > > It is best if we have PostDelayedTaskAndReplyWithResult, but it apparently does > not exist. Maybe you can define an intermediate callback function and calling > PostDelayedTask and PostTaskAndReplyWithResult in order? Done.
lgtm with nits. Thanks! https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:37: base::TimeDelta::FromMilliseconds(5); Hmm, 5ms sounds too short when threads are busy. Why don't you delay more, 1 second for example? We don't need to scan files instantly. https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:140: base::Time last_notify_time_; Oh, BTW, base::Time is not guaranteed to be monotonically increasing (the clock may be adjusted). base::TimeTicks would be more appropriate. https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:207: TimestampMap current_timestamp_map = timestamp_and_map.second; Nit: Could you also destructure |snapshot_time|? Then the condition below (last_notify_time_ > snapshot_time) will get slightly easier to read. https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:231: void ArcDownloadsWatcherService::DownloadsWatcher::DelayBuildTimestampMap() { nit: Could you move this function to before OnBuildTimestampMap() to align with call order?
https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:37: base::TimeDelta::FromMilliseconds(5); On 2016/09/09 06:32:40, Shuhei Takahashi wrote: > Hmm, 5ms sounds too short when threads are busy. Why don't you delay more, 1 > second for example? We don't need to scan files instantly. Experimental testing showed 5ms covered all inotify events in 100 tests, but as you say we don't need to scan files immediately. Changed to 1s. https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:140: base::Time last_notify_time_; On 2016/09/09 06:32:41, Shuhei Takahashi wrote: > Oh, BTW, base::Time is not guaranteed to be monotonically increasing (the clock > may be adjusted). base::TimeTicks would be more appropriate. Done. https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:207: TimestampMap current_timestamp_map = timestamp_and_map.second; On 2016/09/09 06:32:41, Shuhei Takahashi wrote: > Nit: Could you also destructure |snapshot_time|? Then the condition below > (last_notify_time_ > snapshot_time) will get slightly easier to read. Done. https://codereview.chromium.org/2310833002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:231: void ArcDownloadsWatcherService::DownloadsWatcher::DelayBuildTimestampMap() { On 2016/09/09 06:32:40, Shuhei Takahashi wrote: > nit: Could you move this function to before OnBuildTimestampMap() to align with > call order? Done.
Oops, I missed several points... https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:217: DCHECK_CURRENTLY_ON(BrowserThread::FILE); This should be not true, it's on blocking pool. https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:220: TimestampMap current_timestamp_map = timestamp_and_map.second; Could you use std::move() here?
LGTM with minor comments. https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:39: base::TimeDelta::FromMilliseconds(1000); Optional: 1s sounds a bit longer delay...? Anyway, could you comment the heuristics how to decide 1s? https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:93: .Append( Maybe, FilePath::AppendRelativePath() is what you want? https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:103: base::TimeTicks snapshot_time = base::TimeTicks::Now(); Could you comment that, - BuildTimestampMap take a bit longer time. - snapshot_time must be taken *before* building map. https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:178: outstanding_task_ = false; Clarification: what is this for? Here, outstandig_task_ should be false always? If this is for just-in-case purpose, DCHECK sounds better I think? https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:217: DCHECK_CURRENTLY_ON(BrowserThread::FILE); On 2016/09/12 04:29:13, Shuhei Takahashi wrote: > This should be not true, it's on blocking pool. This should be on FILE thread, IIUC.
https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:217: DCHECK_CURRENTLY_ON(BrowserThread::FILE); On 2016/09/12 05:44:10, hidehiko wrote: > On 2016/09/12 04:29:13, Shuhei Takahashi wrote: > > This should be not true, it's on blocking pool. > > This should be on FILE thread, IIUC. Oops, I misread the code. Please don't mind.
https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:39: base::TimeDelta::FromMilliseconds(1000); On 2016/09/12 05:44:10, hidehiko wrote: > Optional: 1s sounds a bit longer delay...? > Anyway, could you comment the heuristics how to decide 1s? Completely arbitrary. Testing shows that we can actually get away with something as short as 5ms for just deduping individual file events, but nya@ suggested a longer delay since scanning media files is a very low-priority task (and 1s would let us dedupe multi-file events like unzipping/copying). https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:93: .Append( On 2016/09/12 05:44:10, hidehiko wrote: > Maybe, FilePath::AppendRelativePath() is what you want? Done. https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:103: base::TimeTicks snapshot_time = base::TimeTicks::Now(); On 2016/09/12 05:44:10, hidehiko wrote: > Could you comment that, > - BuildTimestampMap take a bit longer time. > - snapshot_time must be taken *before* building map. Done. https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:178: outstanding_task_ = false; On 2016/09/12 05:44:10, hidehiko wrote: > Clarification: what is this for? Here, outstandig_task_ should be false always? > If this is for just-in-case purpose, DCHECK sounds better I think? Done. https://codereview.chromium.org/2310833002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:220: TimestampMap current_timestamp_map = timestamp_and_map.second; On 2016/09/12 04:29:13, Shuhei Takahashi wrote: > Could you use std::move() here? Done.
Sorry for taking long time for the review. Could you run trybot when you upload a new PS? https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:94: cros_path.value().substr(downloads_dir.value().length() + 1)); Something went wrong here, I think? https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:103: // The TimestampMap may include chnages form after snapshot_time. nit: changes
Sorry for taking long time for the review. Could you run trybot when you upload a new PS?
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 perform media scanning if a scan has not been performed since the file update event. Building up the timestamp map is also moved off the FILE thread and on to the blocking pool. BUG=628223 ========== to ========== Don't block FILE thread for media scanning Only perform media scanning if a scan has not been performed since the file update event. Building up the timestamp map is also moved off the FILE thread and on to the blocking pool. BUG=628223 TEST=create a large (~10k) set of files under /home/chronos/user/Downloads Enable ARC++ create a profiler snapshot at chrome://profiler create one more new file under Downloads Create another snapshot in chrome://profiler Verify that there are no long-running tasks on the FILE thread, and that DelayBuildTimestampMap is only called once. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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.
https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:94: cros_path.value().substr(downloads_dir.value().length() + 1)); On 2016/09/13 07:33:56, hidehiko wrote: > Something went wrong here, I think? Not sure what you're thinking of, but the previous commet about switching to AppendRelativePath seems to be incorrect. I've reverted to the original code. https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:103: // The TimestampMap may include chnages form after snapshot_time. On 2016/09/13 07:33:56, hidehiko wrote: > nit: changes Done.
https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:94: cros_path.value().substr(downloads_dir.value().length() + 1)); On 2016/09/15 01:58:12, dspaid wrote: > On 2016/09/13 07:33:56, hidehiko wrote: > > Something went wrong here, I think? > > Not sure what you're thinking of, but the previous commet about switching to > AppendRelativePath seems to be incorrect. I've reverted to the original code. Sorry for poor explanation. I meant; FilePath android_path(kAndroidDownloadDir); downloads_dir.AppendRelativePath(cros_path, &android_path); is what you want? cf) https://cs.chromium.org/chromium/src/base/files/file_path.h?q=AppendRelativeP...
On 2016/09/15 02:26:18, hidehiko wrote: > https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): > > https://codereview.chromium.org/2310833002/diff/180001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:94: > cros_path.value().substr(downloads_dir.value().length() + 1)); > On 2016/09/15 01:58:12, dspaid wrote: > > On 2016/09/13 07:33:56, hidehiko wrote: > > > Something went wrong here, I think? > > > > Not sure what you're thinking of, but the previous commet about switching to > > AppendRelativePath seems to be incorrect. I've reverted to the original code. > > Sorry for poor explanation. > I meant; > > FilePath android_path(kAndroidDownloadDir); > downloads_dir.AppendRelativePath(cros_path, &android_path); > > is what you want? > cf) > https://cs.chromium.org/chromium/src/base/files/file_path.h?q=AppendRelativeP... Done. Sorry, that was all just moved from elsewhere in the file, so I hadn't looked at it too closely.
lgtm
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.
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/2310833002/#ps240001 (title: "Use AppendRelativePath (again)")
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 ========== Don't block FILE thread for media scanning Only perform media scanning if a scan has not been performed since the file update event. Building up the timestamp map is also moved off the FILE thread and on to the blocking pool. BUG=628223 TEST=create a large (~10k) set of files under /home/chronos/user/Downloads Enable ARC++ create a profiler snapshot at chrome://profiler create one more new file under Downloads Create another snapshot in chrome://profiler Verify that there are no long-running tasks on the FILE thread, and that DelayBuildTimestampMap is only called once. ========== to ========== Don't block FILE thread for media scanning Only perform media scanning if a scan has not been performed since the file update event. Building up the timestamp map is also moved off the FILE thread and on to the blocking pool. BUG=628223 TEST=create a large (~10k) set of files under /home/chronos/user/Downloads Enable ARC++ create a profiler snapshot at chrome://profiler create one more new file under Downloads Create another snapshot in chrome://profiler Verify that there are no long-running tasks on the FILE thread, and that DelayBuildTimestampMap is only called once. ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Don't block FILE thread for media scanning Only perform media scanning if a scan has not been performed since the file update event. Building up the timestamp map is also moved off the FILE thread and on to the blocking pool. BUG=628223 TEST=create a large (~10k) set of files under /home/chronos/user/Downloads Enable ARC++ create a profiler snapshot at chrome://profiler create one more new file under Downloads Create another snapshot in chrome://profiler Verify that there are no long-running tasks on the FILE thread, and that DelayBuildTimestampMap is only called once. ========== to ========== Don't block FILE thread for media scanning Only perform media scanning if a scan has not been performed since the file update event. Building up the timestamp map is also moved off the FILE thread and on to the blocking pool. BUG=628223 TEST=create a large (~10k) set of files under /home/chronos/user/Downloads Enable ARC++ create a profiler snapshot at chrome://profiler create one more new file under Downloads Create another snapshot in chrome://profiler Verify that there are no long-running tasks on the FILE thread, and that DelayBuildTimestampMap is only called once. Committed: https://crrev.com/d8b3cb93177c7223bc67bce7b823083a380b19c2 Cr-Commit-Position: refs/heads/master@{#418778} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/d8b3cb93177c7223bc67bce7b823083a380b19c2 Cr-Commit-Position: refs/heads/master@{#418778} |
