Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
298823005:
Files.app: Run two metadata fetch jobs congruently (Closed)
|
Created:
6 years, 7 months ago by yoshiki Modified:
6 years, 6 months ago Reviewers:
mtomasz CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFiles.app: Run two metadata fetch jobs congruently
On fetching metadata, after sending a request, JS is just waiting results and nothing is running in JS side.
With this patch, the next job can be run in such an idle time.
BUG=367123
TEST=filelist can be shown correctly
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273185
Patch Set 1 : wip #
Total comments: 5
Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : Tunes parameters (50/2 -> 25/4) #Patch Set 4 : Remove debug output #Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:652: this.prefetchMetadataQueue_.run(function(callbackOuter) { Naming is confusing. How about: this.prefetchMetadataQueue_ -> this.processNewEntriesQueue_ metadataFetchQueue -> prefetchMetadataQueue? https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:657: var metadataFetchQueue = new AsyncUtil.ConcurrentQueue(2); How much does this optimization improve performance? https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:658: metadataFetchQueue.setFinishCallback(function() { This should be passed to AsyncUtil.ConcurrentQueue constructor. https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:669: this.prefetchMetadata(chunk, function() { How about checking if cancelled also before prefetching metadata?
> https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... > ui/file_manager/file_manager/foreground/js/directory_contents.js:657: var > metadataFetchQueue = new AsyncUtil.ConcurrentQueue(2); > How much does this optimization improve performance? Roughly 1.2x improvement (scan-start to scan-completed) On z620: 600ms -> 500ms On daisy 4000ms -> 3200ms
On 2014/05/23 08:24:19, yoshiki wrote: > > > https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... > > ui/file_manager/file_manager/foreground/js/directory_contents.js:657: var > > metadataFetchQueue = new AsyncUtil.ConcurrentQueue(2); > > How much does this optimization improve performance? > > Roughly 1.2x improvement (scan-start to scan-completed) > On z620: 600ms -> 500ms > On daisy 4000ms -> 3200ms Nice. Did you try with more than 2?
On 2014/05/23 08:34:38, mtomasz wrote: > On 2014/05/23 08:24:19, yoshiki wrote: > > > > > > https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... > > > ui/file_manager/file_manager/foreground/js/directory_contents.js:657: var > > > metadataFetchQueue = new AsyncUtil.ConcurrentQueue(2); > > > How much does this optimization improve performance? > > > > Roughly 1.2x improvement (scan-start to scan-completed) > > On z620: 600ms -> 500ms > > On daisy 4000ms -> 3200ms > > Nice. Did you try with more than 2? On z620, 2-4 are almost same result. Not checked on real device. I think 2 is better for memory consumption.
@mtomasz: PTAL. https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/298823005/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:652: this.prefetchMetadataQueue_.run(function(callbackOuter) { On 2014/05/21 13:15:47, mtomasz wrote: > Naming is confusing. How about: > this.prefetchMetadataQueue_ -> this.processNewEntriesQueue_ > metadataFetchQueue -> prefetchMetadataQueue? Thanks for guidance! Done.
https://codereview.chromium.org/298823005/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/298823005/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:656: var MAX_CHUNK_SIZE = 50; I just took a look at metadata_cache.js, and it seems that all of the entries passed to MetadataCache::get() are processed in "parallel". So it seems that it is 50 requests running at once. Introducing a ConcurrentQueue(2) should basically have the same effect as setting MAX_CHUNK_SIZE = 100, without any other change. I may be wrong here. Please check if changing MAX_CHUNK_SIZE = 100 isn't the same as introducing and using ConcurrentQueue(2). If so, then we may not need to use it, and sorry for not catching it earlier. But I may be wrong, please do a micro benchmark. Thanks.
https://codereview.chromium.org/298823005/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/298823005/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:656: var MAX_CHUNK_SIZE = 50; On 2014/05/27 15:31:26, mtomasz wrote: > I just took a look at metadata_cache.js, and it seems that all of the entries > passed to MetadataCache::get() are processed in "parallel". So it seems that it > is 50 requests running at once. Introducing a ConcurrentQueue(2) should > basically have the same effect as setting MAX_CHUNK_SIZE = 100, without any > other change. > > I may be wrong here. Please check if changing = 100 isn't the > same as introducing and using ConcurrentQueue(2). If so, then we may not need to > use it, and sorry for not catching it earlier. > > But I may be wrong, please do a micro benchmark. Thanks. Situation are diffirent between drive and non-drive volumes. But this patch will improve the performance for both types of volume. As for drive metadata, the API is not called in parallel. The request is combined and the API is called once. As for other volume, the current problem is this.prefetchMetadataQueue_ waits the slowest query of every 50 requests and this patch will ease the blocking by the slowest queue. The results of the benchmarks show that. * Drive (~2200 files) With this patch: ave:793ms, mean:782ms, max:825ms, min:775ms (n=9) MAX_CHUNK_SIZE=100: ave:855ms, mean:848ms, max:939ms, min:814ms (n=9) Without modification: ave:993ms, mean:1059ms, max:1128ms, min:843ms (n=9) * Downloads (~2400 files) With this patch: ave:2661ms, mean:2624ms, max:2828ms, min:2571ms (n=9) MAX_CHUNK_SIZE=100: ave:2873ms, mean:2767ms, max:3021ms, min:2757ms (n=9) Without modification: ave:2922ms, mean:2819ms, max:3470ms, min:2667ms (n=9)
On 2014/05/28 03:30:15, yoshiki wrote: > https://codereview.chromium.org/298823005/diff/40001/ui/file_manager/file_man... > File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): > > https://codereview.chromium.org/298823005/diff/40001/ui/file_manager/file_man... > ui/file_manager/file_manager/foreground/js/directory_contents.js:656: var > MAX_CHUNK_SIZE = 50; > On 2014/05/27 15:31:26, mtomasz wrote: > > I just took a look at metadata_cache.js, and it seems that all of the entries > > passed to MetadataCache::get() are processed in "parallel". So it seems that > it > > is 50 requests running at once. Introducing a ConcurrentQueue(2) should > > basically have the same effect as setting MAX_CHUNK_SIZE = 100, without any > > other change. > > > > I may be wrong here. Please check if changing = 100 isn't the > > same as introducing and using ConcurrentQueue(2). If so, then we may not need > to > > use it, and sorry for not catching it earlier. > > > > But I may be wrong, please do a micro benchmark. Thanks. > > Situation are diffirent between drive and non-drive volumes. But this patch will > improve the performance for both types of volume. > > As for drive metadata, the API is not called in parallel. The request is > combined and the API is called once. > > As for other volume, the current problem is this.prefetchMetadataQueue_ waits > the slowest query of every 50 requests and this patch will ease the blocking by > the slowest queue. > > > The results of the benchmarks show that. > > * Drive (~2200 files) > With this patch: ave:793ms, mean:782ms, max:825ms, min:775ms (n=9) > MAX_CHUNK_SIZE=100: ave:855ms, mean:848ms, max:939ms, min:814ms (n=9) > Without modification: ave:993ms, mean:1059ms, max:1128ms, min:843ms (n=9) > > * Downloads (~2400 files) > With this patch: ave:2661ms, mean:2624ms, max:2828ms, min:2571ms (n=9) > MAX_CHUNK_SIZE=100: ave:2873ms, mean:2767ms, max:3021ms, min:2757ms (n=9) > Without modification: ave:2922ms, mean:2819ms, max:3470ms, min:2667ms (n=9) Interesting. Thanks for the detailed benchmark. I'm curious if having: MAX_CHUNK_SIZE=25 and ConcurrentQueue(4) would make it even better, then. Do you think it is possible? The code itself LGTM.
On 2014/05/28 04:00:21, mtomasz wrote: > On 2014/05/28 03:30:15, yoshiki wrote: > > > https://codereview.chromium.org/298823005/diff/40001/ui/file_manager/file_man... > > File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): > > > > > https://codereview.chromium.org/298823005/diff/40001/ui/file_manager/file_man... > > ui/file_manager/file_manager/foreground/js/directory_contents.js:656: var > > MAX_CHUNK_SIZE = 50; > > On 2014/05/27 15:31:26, mtomasz wrote: > > > I just took a look at metadata_cache.js, and it seems that all of the > entries > > > passed to MetadataCache::get() are processed in "parallel". So it seems that > > it > > > is 50 requests running at once. Introducing a ConcurrentQueue(2) should > > > basically have the same effect as setting MAX_CHUNK_SIZE = 100, without any > > > other change. > > > > > > I may be wrong here. Please check if changing = 100 isn't the > > > same as introducing and using ConcurrentQueue(2). If so, then we may not > need > > to > > > use it, and sorry for not catching it earlier. > > > > > > But I may be wrong, please do a micro benchmark. Thanks. > > > > Situation are diffirent between drive and non-drive volumes. But this patch > will > > improve the performance for both types of volume. > > > > As for drive metadata, the API is not called in parallel. The request is > > combined and the API is called once. > > > > As for other volume, the current problem is this.prefetchMetadataQueue_ waits > > the slowest query of every 50 requests and this patch will ease the blocking > by > > the slowest queue. > > > > > > The results of the benchmarks show that. > > > > * Drive (~2200 files) > > With this patch: ave:793ms, mean:782ms, max:825ms, min:775ms (n=9) > > MAX_CHUNK_SIZE=100: ave:855ms, mean:848ms, max:939ms, min:814ms (n=9) > > Without modification: ave:993ms, mean:1059ms, max:1128ms, min:843ms (n=9) > > > > * Downloads (~2400 files) > > With this patch: ave:2661ms, mean:2624ms, max:2828ms, min:2571ms (n=9) > > MAX_CHUNK_SIZE=100: ave:2873ms, mean:2767ms, max:3021ms, min:2757ms (n=9) > > Without modification: ave:2922ms, mean:2819ms, max:3470ms, min:2667ms (n=9) > > Interesting. Thanks for the detailed benchmark. I'm curious if having: > MAX_CHUNK_SIZE=25 and ConcurrentQueue(4) would make it even better, then. > Do you think it is possible? > > The code itself LGTM. As for Drive, 25/4 is faster than 50/2. On Downloads, 25/4 is a bit slower but I think it's accidental error. We may have more room to tune parameters but I'll go with 25/4 as for now. Thanks. Benchmark (25/4): - Drive: 718, 699, 812, 678 - Downloads: 2761, 2748. 2921, 2651
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/298823005/80001
Message was sent while issue was closed.
Change committed as 273185 |
