Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
113293007:
[Files.app] Change sort timing during metadata fetch (Closed)
|
Created:
7 years ago by yoshiki Modified:
6 years, 11 months ago CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Files.app] Change sort timing during metadata fetch
Previously, sort was not started until all the metadata is fetched. When fetching took a time, the user saw unsorted list for a while. With this patch, sort is happened just after every updates. User can see sorted list every time.
BUG=327479
TEST=manually tested
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243816
Patch Set 1 : . #
Total comments: 4
Patch Set 2 : addressed comments #
Total comments: 2
Patch Set 3 : Adressed the comment #
Total comments: 10
Patch Set 4 : addressed comments #
Total comments: 2
Patch Set 5 : addressed comment #Patch Set 6 : rebase #
Total comments: 2
Patch Set 7 : . #Messages
Total messages: 18 (0 generated)
@hirono: could you take a look? Thanks.
https://codereview.chromium.org/113293007/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/113293007/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:623: this.fileList_.updateIndex(i); How about the following? var status = this.list_.sortStatus; this.list_.sort(status.field, status.direction); https://codereview.chromium.org/113293007/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js (right): https://codereview.chromium.org/113293007/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:183: return desiredNumber; nit: How about just returning the expression.
@hirono, PTAL. Thanks. https://codereview.chromium.org/113293007/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/113293007/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:623: this.fileList_.updateIndex(i); On 2013/12/17 11:35:15, hirono wrote: > How about the following? > > var status = this.list_.sortStatus; > this.list_.sort(status.field, status.direction); Done. https://codereview.chromium.org/113293007/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js (right): https://codereview.chromium.org/113293007/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:183: return desiredNumber; On 2013/12/17 11:35:15, hirono wrote: > nit: How about just returning the expression. Done.
https://codereview.chromium.org/113293007/diff/40001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/113293007/diff/40001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:614: this.prefetchMetadataQueue_.run(function(i, chunk, callback) { The i variable no longer needed.
@hirono: PTAL. Thanks. https://codereview.chromium.org/113293007/diff/40001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/113293007/diff/40001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:614: this.prefetchMetadataQueue_.run(function(i, chunk, callback) { On 2013/12/18 04:09:38, hirono wrote: > The i variable no longer needed. Done.
lgtm. Thanks!
https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:623: // items. Currenyly we have a method this.fileList_.updateIndex() to typo: Currently. https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:630: this.list_.sort(status.field, status.direction); Please check if it is not a performance killer for large number of files. https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:630: this.list_.sort(status.field, status.direction); In 'scan-completed' handler in file_manager.js, the entire metadata cache is cleared and reloaded. Shall we remove it, since we have the entire metadata during updates? https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js (right): https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:167: MetadataCache.prototype.setCurrentContentNumber = function(number) { Contents is uncountable. Also, Cache shouldn't know about number of items being shown. How about setCacheSize()? https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:181: return this.currentContentNumber_ * 2 + MetadataCache.EVICTION_NUMBER / 2; Eviction policy got tricky here. Why this formula? Why do you divide EVICTION_NUMBER by 2? Can you please add a comment?
@mtomasz: PTAL. https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:623: // items. Currenyly we have a method this.fileList_.updateIndex() to On 2013/12/24 00:22:10, mtomasz wrote: > typo: Currently. Done. https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:630: this.list_.sort(status.field, status.direction); On 2013/12/24 00:22:10, mtomasz wrote: > In 'scan-completed' handler in file_manager.js, the entire metadata cache is > cleared and reloaded. Shall we remove it, since we have the entire metadata > during updates? I agree. Done. https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/directory_contents.js:630: this.list_.sort(status.field, status.direction); On 2013/12/24 00:22:10, mtomasz wrote: > Please check if it is not a performance killer for large number of files. This increases the number of executed sort and the total time of scan may be increased. But the sort happens just after the content is updated, so the user feels less frustrated. https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... File chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js (right): https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:167: MetadataCache.prototype.setCurrentContentNumber = function(number) { On 2013/12/24 00:22:10, mtomasz wrote: > Contents is uncountable. Also, Cache shouldn't know about number of items being > shown. How about setCacheSize()? Thanks! Done. https://codereview.chromium.org/113293007/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:181: return this.currentContentNumber_ * 2 + MetadataCache.EVICTION_NUMBER / 2; On 2013/12/24 00:22:10, mtomasz wrote: > Eviction policy got tricky here. Why this formula? Why do you divide > EVICTION_NUMBER by 2? Can you please add a comment? Made the rule and the code simpler.
Thanks! Looks great! LgTm https://codereview.chromium.org/113293007/diff/410001/chrome/browser/resource... File chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js (right): https://codereview.chromium.org/113293007/diff/410001/chrome/browser/resource... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:164: * Sets the size of cache. The acutual cache size may be larger than the given nit: actual
Thanks! Going to commit. https://codereview.chromium.org/113293007/diff/410001/chrome/browser/resource... File chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js (right): https://codereview.chromium.org/113293007/diff/410001/chrome/browser/resource... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:164: * Sets the size of cache. The acutual cache size may be larger than the given On 2014/01/08 08:11:02, mtomasz wrote: > nit: actual Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/113293007/480001
Failed to apply patch for
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
Hunk #1 succeeded at 76 (offset -1 lines).
Hunk #2 succeeded at 105 (offset -1 lines).
Hunk #3 succeeded at 160 with fuzz 1 (offset -1 lines).
Hunk #4 FAILED at 480.
Hunk #5 succeeded at 536 (offset 8 lines).
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js.rej
Patch:
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
Index:
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
diff --git
a/chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
b/chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
index
75dabec132323901386012c85aa231ec7e35a55c..af69851f155a878eb644aa749c864d9a2fc7cbe2
100644
---
a/chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
+++
b/chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
@@ -77,6 +77,8 @@ function MetadataCache() {
this.batchCount_ = 0;
this.totalCount_ = 0;
+ this.currentCacheSize_ = 0;
+
/**
* Time of first get query of the current batch. Items updated later than
this
* will not be evicted.
@@ -104,9 +106,9 @@ MetadataCache.CHILDREN = 1;
MetadataCache.DESCENDANTS = 2;
/**
- * Minimum number of items in cache to start eviction.
+ * Margin of the cache size. This amount of caches may be kept in addition.
*/
-MetadataCache.EVICTION_NUMBER = 1000;
+MetadataCache.EVICTION_THRESHOLD_MARGIN = 500;
/**
* @return {MetadataCache!} The cache with all providers.
@@ -159,6 +161,28 @@ MetadataCache.prototype.isInitialized = function() {
};
/**
+ * Sets the size of cache. The actual cache size may be larger than the given
+ * value.
+ * @param {number} size The cache size to be set.
+ */
+MetadataCache.prototype.setCacheSize = function(size) {
+ this.currentCacheSize_ = size;
+
+ if (this.totalCount_ > this.currentEvictionThreshold_())
+ this.evict_();
+};
+
+/**
+ * Returns the current threshold to evict caches. When the number of caches
+ * exceeds this, the cache should be evicted.
+ * @return {number} Threshold to evict caches.
+ * @private
+ */
+MetadataCache.prototype.currentEvictionThreshold_ = function() {
+ return this.currentCacheSize_ * 2 + MetadataCache.EVICTION_THRESHOLD_MARGIN;
+};
+
+/**
* Fetches the metadata, puts it in the cache, and passes to callback.
* If required metadata is already in the cache, does not fetch it again.
* @param {string|Entry|Array.<string|Entry>} items The list of entries or
@@ -456,7 +480,7 @@ MetadataCache.prototype.startBatchUpdates = function() {
MetadataCache.prototype.endBatchUpdates = function() {
this.batchCount_--;
if (this.batchCount_ != 0) return;
- if (this.totalCount_ > MetadataCache.EVICTION_NUMBER)
+ if (this.totalCount_ > this.currentEvictionThreshold_())
this.evict_();
for (var index = 0; index < this.observers_.length; index++) {
var observer = this.observers_[index];
@@ -504,7 +528,7 @@ MetadataCache.prototype.evict_ = function() {
var toRemove = [];
// We leave only a half of items, so we will not call evict_ soon again.
- var desiredCount = Math.round(MetadataCache.EVICTION_NUMBER / 2);
+ var desiredCount = this.currentEvictionThreshold();
var removeCount = this.totalCount_ - desiredCount;
for (var url in this.cache_) {
if (this.cache_.hasOwnProperty(url) &&
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/113293007/520001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/113293007/790001
Message was sent while issue was closed.
Change committed as 243816
Message was sent while issue was closed.
https://codereview.chromium.org/113293007/diff/790001/chrome/browser/resource... File chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js (right): https://codereview.chromium.org/113293007/diff/790001/chrome/browser/resource... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:539: var desiredCount = this.currentEvictionThreshold(); Error in response to fileBrowserPrivate.searchDriveMetadata: TypeError: Object #<MetadataCache> has no method 'currentEvictionThreshold' currentEvictionThreshold -> currentEvictionThreshold_. Sorry for not catching earlier.
Message was sent while issue was closed.
https://codereview.chromium.org/113293007/diff/790001/chrome/browser/resource... File chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js (right): https://codereview.chromium.org/113293007/diff/790001/chrome/browser/resource... chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:539: var desiredCount = this.currentEvictionThreshold(); On 2014/01/10 00:57:59, mtomasz wrote: > Error in response to fileBrowserPrivate.searchDriveMetadata: TypeError: Object > #<MetadataCache> has no method 'currentEvictionThreshold' > > currentEvictionThreshold -> currentEvictionThreshold_. Sorry for not catching > earlier. Oops, sorry. I didn't catch it. I'll fix it soon. |
