Tweaks for improving file manager performance

Improving file manager js/css performance

This change attempts to reduce the load time of the file manager (especially on slow Alex devices) by trimming unnecessary work done in javascript and reducing layouts.

- Enable batch updating in cr.ui.Table (exactly how it's done in cr.ui.List).
- Add more 'on complete' callbacks to some of the FileManager infrastructure so we know when to stop batch UI updates.
- Use batch updates for some operations which profiling indicates causes non-trivial amounts of duplicated work.  In particular, in my testing this reduces the number of (sometimes expensive) List.redraw() calls on startup for the table from 6 down to 1, and for the roots list from 4 down to 2.

Measurements on alex are quite variable, but these changes result in about 70ms savings on startup (or about 17% of the time spent under 'v8.callChromeHiddenMethod' - i.e. JS callbacks through the extension system, which itself is about 1/3rd of total load time).

The majority of file manager load time is spent inside of v8, and there are many more opportunities like these to trim various code paths.  But it seems clear that major improvements are going to require more drastic approaches (eg. I'm experimenting with painting the initial UI after parsing/running a small fraction of the JS).

BUG=105181
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124674

[Rick Byers, 2012-02-28 18:16:59]

Robert, can you please review these file manager changes?

Erik, please review the small cr.ui.Table changes.

[rginda, 2012-02-28 18:38:35]

file_manager changes lgtm with one comment and a typo.

http://codereview.chromium.org/9379023/diff/2001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/9379023/diff/2001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:757:
self.rootsList_.endBatchUpdates();
updateRoots may benefit from a try/catch so that the start/end calls don't get
imbalanced when things start to go wrong.

http://codereview.chromium.org/9379023/diff/2001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1387: // Avoid a bunch
of intermedia list redraws while the data model is cleared
typo: intermediate

[Rick Byers, 2012-02-28 19:56:26]

Thanks Robert!

http://codereview.chromium.org/9379023/diff/2001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/9379023/diff/2001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:757:
self.rootsList_.endBatchUpdates();
On 2012/02/28 18:38:35, rginda wrote:
> updateRoots may benefit from a try/catch so that the start/end calls don't get
> imbalanced when things start to go wrong.

It doesn't look to me like there's much logic that happens synchronously in
updateRoots that is likely to fail.  Obviously bugs can lead to broken UI, but
is that something we want to add complexity to protect against?  Eg. unexpected
exceptions during all the file manager initialization code currently leave the
file manager as a white page (opacity isn't set).  From inspection, it looks
like the code is good about preserving the callback chain in the face of I/O
errors.  Are there other types of errors you're thinking about here that we
should be prepared for and try to recover from?  I'm happy to add some
protection around places that may be error-prone, I just don't see any obvious
places based on my limited understanding of the code...

http://codereview.chromium.org/9379023/diff/2001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1387: // Avoid a bunch
of intermedia list redraws while the data model is cleared
On 2012/02/28 18:38:35, rginda wrote:
> typo: intermediate

Done.

[arv (Not doing code reviews), 2012-02-28 20:26:32]

LGTM with nits

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/directory_model.js:533: * @param
{function} opt_loadedCallback Invoked when the entire directory has
{Function=}

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/directory_model.js:534: * been loaded.
indent

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:757:
self.rootsList_.endBatchUpdates();
this.directoryModel_.updateRoots(function() {
  self.rootsList_.endBatchUpdates();
});

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1390: var onLoaded =
function() {
or

var onLoaded = this.table_.endBatchUpdates.bind(this.table_);

[Rick Byers, 2012-02-28 20:35:00]

Thanks Erik.

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/directory_model.js:533: * @param
{function} opt_loadedCallback Invoked when the entire directory has
On 2012/02/28 20:26:32, arv wrote:
> {Function=}

Done.

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/directory_model.js:534: * been loaded.
On 2012/02/28 20:26:32, arv wrote:
> indent

Done.

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:757:
self.rootsList_.endBatchUpdates();
On 2012/02/28 20:26:32, arv wrote:
> this.directoryModel_.updateRoots(function() {
>   self.rootsList_.endBatchUpdates();
> });

Done.

http://codereview.chromium.org/9379023/diff/9001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1390: var onLoaded =
function() {
On 2012/02/28 20:26:32, arv wrote:
> or
> 
> var onLoaded = this.table_.endBatchUpdates.bind(this.table_);

Done.

[commit-bot: I haz the power, 2012-02-29 14:41:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/9379023/10003

[commit-bot: I haz the power, 2012-02-29 14:41:40]

Can't apply patch for file
chrome/browser/resources/file_manager/js/directory_model.js.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/file_manager/js/directory_model.js
Hunk #1 FAILED at 525.
Hunk #2 FAILED at 538.
Hunk #3 FAILED at 547.
Hunk #4 FAILED at 568.
Hunk #5 succeeded at 613 (offset 7 lines).
Hunk #6 succeeded at 761 with fuzz 1 (offset 21 lines).
Hunk #7 succeeded at 769 (offset 21 lines).
4 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/resources/file_manager/js/directory_model.js.rej

[rginda, 2012-02-29 19:32:36]

On 2012/02/28 19:56:26, Rick Byers wrote:
> Thanks Robert!
> 
>
http://codereview.chromium.org/9379023/diff/2001/chrome/browser/resources/fil...
> File chrome/browser/resources/file_manager/js/file_manager.js (right):
> 
>
http://codereview.chromium.org/9379023/diff/2001/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/js/file_manager.js:757:
> self.rootsList_.endBatchUpdates();
> On 2012/02/28 18:38:35, rginda wrote:
> > updateRoots may benefit from a try/catch so that the start/end calls don't
get
> > imbalanced when things start to go wrong.
> 
> It doesn't look to me like there's much logic that happens synchronously in
> updateRoots that is likely to fail.  Obviously bugs can lead to broken UI, but
> is that something we want to add complexity to protect against?  Eg.
unexpected
> exceptions during all the file manager initialization code currently leave the
> file manager as a white page (opacity isn't set).  From inspection, it looks
> like the code is good about preserving the callback chain in the face of I/O
> errors.  Are there other types of errors you're thinking about here that we
> should be prepared for and try to recover from?  I'm happy to add some
> protection around places that may be error-prone, I just don't see any obvious
> places based on my limited understanding of the code...

I just brought it up because it's something that concerns me when I see a
open/close type of pattern where the close happens in a callback.  I think
you're right though, this code seems very likely to call the loadedCallback in
all cases (as long as resolveCallback never fails.)

> 
>
http://codereview.chromium.org/9379023/diff/2001/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/js/file_manager.js:1387: // Avoid a
bunch
> of intermedia list redraws while the data model is cleared
> On 2012/02/28 18:38:35, rginda wrote:
> > typo: intermediate
> 
> Done.

[Rick Byers, 2012-02-29 20:37:11]

+kaznacheev

Vladislav, I merged this with your related change that went in this morning. 
The only non-trivial part to the merge was that we both added a new callback to
setupPath but it looks like our usage is compatible so I just folded them
together.  I don't think there's any need for you to review this too, but I'll
hold off putting it in the CQ until tomorrow morning (GMT-5) in case you have
any comments.

Thanks,
   Rick

[Vladislav Kaznacheev, 2012-02-29 21:04:48]

Hi Rick,

I am sorry my commit got in the way. You are doing wonderful things!

Though I am afraid that the merge is not quite done yet. See my comments.

  Vlad

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/directory_model.js:571: if
(opt_loadedCallback)
opt_loadedCallback has different semantic from the old opt_fileSelectCallback. I
expect this breaks some things in the file browser. Specifically,
opt_fileSelectCallback should be called only from this one place (when the file
is successfully selected).

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:1425: removeShade();
onLoaded() needs to be called from there as well.

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2369: changeDirectoryTo
= event.mountPath;
This is now an undeclared and unused variable. I doubt this is what you
intended.

[SeRya, 2012-03-01 09:59:47]

This code may reduce time of initial directory scan but potentially
significantly increase time when the users see first result.

Now 2 scan directories in 2 ways:
1) When directory changed it's loads in chunks.
2) When 'directory change' event comes it updates as whole.

This CL keeps initial load in chunks (each chunk causes a bunch of events
internally) but prevents visual update. When directory change it still loads in
chunks. It it what you really intended to do?

[Rick Byers, 2012-03-01 22:00:09]

On 2012/03/01 09:59:47, SeRya wrote:
> This code may reduce time of initial directory scan but potentially
> significantly increase time when the users see first result.
> 
> Now 2 scan directories in 2 ways:
> 1) When directory changed it's loads in chunks.
> 2) When 'directory change' event comes it updates as whole.
> 
> This CL keeps initial load in chunks (each chunk causes a bunch of events
> internally) but prevents visual update. When directory change it still loads
in
> chunks. It it what you really intended to do?

Thanks for the feedback Sergey.  So you're saying we are actively trying to
update the UI incrementally as results are available?  I agree that if this is
what we actually want, then my change will break that (it was precisely the
repeated updates, re-layouts, and paints I was trying to avoid to speed up
loading time).  However, in my tests it seems that DirectoryReader.ReadEntries
typically makes large chunks - eg. even with 10,000 files I get a single 'chunk'
with 10,000 entries (which we then go chew on in JS for >30 seconds on Alex
before displaying any of them - even before my change).  Also since the UI
always (or usually?) sorts the results - would we really want the results coming
in incrementally and moving around (resorting on every chunk)?  I just assumed
that the code dealing with chunks was just for convenience given the filesystem
API but that we didn't really intend to try to display incremental directory
loading.  So as far as I can tell, I don't think my CL is actually making this
any worse - do you agree?

If we do want to display incremental directory loading, then we probably need to
break up the chunks we get back from readEntries.  Eg., readEntries can return
10,000 entries relatively quickly, but (by default) we don't insert the chunk
into the list to be displayed until we've gotten the modification time for each
entry in the chunk.  Since this can take a long time, we'd need some mechanism
to update the list as we get modifications times back.

Let me know what you think we should do here.

[Rick Byers, 2012-03-02 00:19:45]

Thanks for the feedback Vlad - please take another look.

Regarding the incremental updates issue Sergey raised, I should add that there's
almost certainly some details in the code I don't understand yet (eg. I'm not
following on what the case is today where the update happens all at once - looks
like it's always 'chunked' to me).  Let me know what you guys think is best - we
could certainly relax some of the batching a bit.  In my testing, before my
change we'd do a List.redraw() of the grid list 6 times on initial load, and now
we do it only once.  If you guys feel it's important for a redraw to be
triggered after each DirectoryReader chunk then I should be able to at least
keep the number of redraws down to 2 instead of going back to 6.

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/directory_model.js:571: if
(opt_loadedCallback)
On 2012/02/29 21:04:48, Vladislav Kaznacheev wrote:
> opt_loadedCallback has different semantic from the old opt_fileSelectCallback.
I
> expect this breaks some things in the file browser. Specifically,
> opt_fileSelectCallback should be called only from this one place (when the
file
> is successfully selected).

Thanks.  So you want to handle the case where the path specifies an existing
leaf note.  Rather than have a special callback for that, it looks like we
already communicate (and make sure of) that information in the onResolve
callback.  How about I just keep track of whether a leaf was found in onResolve?
 Alternately I could pass the base/leaf names again in the onLoaded callback.

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:1425: removeShade();
On 2012/02/29 21:04:48, Vladislav Kaznacheev wrote:
> onLoaded() needs to be called from there as well.

I don't think so, we're not necessarily done yet at this point.  onFileSelected
will get invoked when we're done and we'll call onLoaded from there.  I'll
update the names to make this a little less confusing.

http://codereview.chromium.org/9379023/diff/16002/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2369: changeDirectoryTo
= event.mountPath;
On 2012/02/29 21:04:48, Vladislav Kaznacheev wrote:
> This is now an undeclared and unused variable. I doubt this is what you
> intended.

Yes thank you - I missed that case!  How's this instead?

http://codereview.chromium.org/9379023/diff/26009/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/9379023/diff/26009/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:1431: var foundLeaf =
true;
Here's where I handled the special 'onFileSelected' case with a combination of
the two callbacks.

[SeRya, 2012-03-02 12:10:08]

On 2012/03/01 22:00:09, Rick Byers wrote:
> On 2012/03/01 09:59:47, SeRya wrote:
> > This code may reduce time of initial directory scan but potentially
> > significantly increase time when the users see first result.
> > 
> > Now 2 scan directories in 2 ways:
> > 1) When directory changed it's loads in chunks.
> > 2) When 'directory change' event comes it updates as whole.
> > 
> > This CL keeps initial load in chunks (each chunk causes a bunch of events
> > internally) but prevents visual update. When directory change it still loads
> in
> > chunks. It it what you really intended to do?
> 
> Thanks for the feedback Sergey.  So you're saying we are actively trying to
> update the UI incrementally as results are available?  I agree that if this is
> what we actually want, then my change will break that (it was precisely the
> repeated updates, re-layouts, and paints I was trying to avoid to speed up
> loading time).  However, in my tests it seems that DirectoryReader.ReadEntries
> typically makes large chunks - eg. even with 10,000 files I get a single
'chunk'
> with 10,000 entries (which we then go chew on in JS for >30 seconds on Alex
> before displaying any of them - even before my change).  Also since the UI
> always (or usually?) sorts the results - would we really want the results
coming
> in incrementally and moving around (resorting on every chunk)?  I just assumed
> that the code dealing with chunks was just for convenience given the
filesystem
> API but that we didn't really intend to try to display incremental directory
> loading.  So as far as I can tell, I don't think my CL is actually making this
> any worse - do you agree?
> 
> If we do want to display incremental directory loading, then we probably need
to
> break up the chunks we get back from readEntries.  Eg., readEntries can return
> 10,000 entries relatively quickly, but (by default) we don't insert the chunk
> into the list to be displayed until we've gotten the modification time for
each
> entry in the chunk.  Since this can take a long time, we'd need some mechanism
> to update the list as we get modifications times back.
> 
> Let me know what you think we should do here.

There were pretty good reason why we mustn't rescan directory in chunks. For
scanning new directories I just left previously existing mechanism since what
change require more careful consideration and priority of that task was much
lower.

I suppose that intention of updating UI in chunks was the speed up UI feedback.
If this goal already broken your CL doesn't make things worse but there is a way
to achieve the same result even better by using cumulative update of data model
(like directory rescan does). Also I think you should file an issue describing
behavior with 10 000 files.

[Rick Byers, 2012-03-02 15:12:02]

On 2012/03/02 12:10:08, SeRya wrote:
> On 2012/03/01 22:00:09, Rick Byers wrote:
> > On 2012/03/01 09:59:47, SeRya wrote:
> > > This code may reduce time of initial directory scan but potentially
> > > significantly increase time when the users see first result.
> > > 
> > > Now 2 scan directories in 2 ways:
> > > 1) When directory changed it's loads in chunks.
> > > 2) When 'directory change' event comes it updates as whole.
> > > 
> > > This CL keeps initial load in chunks (each chunk causes a bunch of events
> > > internally) but prevents visual update. When directory change it still
loads
> > in
> > > chunks. It it what you really intended to do?
> > 
> > Thanks for the feedback Sergey.  So you're saying we are actively trying to
> > update the UI incrementally as results are available?  I agree that if this
is
> > what we actually want, then my change will break that (it was precisely the
> > repeated updates, re-layouts, and paints I was trying to avoid to speed up
> > loading time).  However, in my tests it seems that
DirectoryReader.ReadEntries
> > typically makes large chunks - eg. even with 10,000 files I get a single
> 'chunk'
> > with 10,000 entries (which we then go chew on in JS for >30 seconds on Alex
> > before displaying any of them - even before my change).  Also since the UI
> > always (or usually?) sorts the results - would we really want the results
> coming
> > in incrementally and moving around (resorting on every chunk)?  I just
assumed
> > that the code dealing with chunks was just for convenience given the
> filesystem
> > API but that we didn't really intend to try to display incremental directory
> > loading.  So as far as I can tell, I don't think my CL is actually making
this
> > any worse - do you agree?
> > 
> > If we do want to display incremental directory loading, then we probably
need
> to
> > break up the chunks we get back from readEntries.  Eg., readEntries can
return
> > 10,000 entries relatively quickly, but (by default) we don't insert the
chunk
> > into the list to be displayed until we've gotten the modification time for
> each
> > entry in the chunk.  Since this can take a long time, we'd need some
mechanism
> > to update the list as we get modifications times back.
> > 
> > Let me know what you think we should do here.
> 
> There were pretty good reason why we mustn't rescan directory in chunks. For
> scanning new directories I just left previously existing mechanism since what
> change require more careful consideration and priority of that task was much
> lower.
> 
> I suppose that intention of updating UI in chunks was the speed up UI
feedback.
> If this goal already broken your CL doesn't make things worse but there is a
way
> to achieve the same result even better by using cumulative update of data
model
> (like directory rescan does). Also I think you should file an issue describing
> behavior with 10 000 files.

Thanks Sergey.  I filed
http://code.google.com/p/chromium-os/issues/detail?id=27203 to track this.

[Vladislav Kaznacheev, 2012-03-02 15:31:08]

lgtm

[commit-bot: I haz the power, 2012-03-02 15:48:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/9379023/26009

[commit-bot: I haz the power, 2012-03-02 17:20:26]

Change committed as 124674

[zel, 2012-03-03 00:31:58]

On 2012/03/02 17:20:26, I haz the power (commit-bot) wrote:
> Change committed as 124674

Reverted - caused regression
http://code.google.com/p/chromium-os/issues/detail?id=27241