MD Bookmarks: Batch updates to the UI when processing deletes and moves

MD Bookmarks: Batch updates to the UI when processing API operations

In MD Bookmarks, each action processed from the bookmarks API listeners
would immediately notify the UI about changes that need to be made. In
cases where many notifications are triggered in a row (eg, deleting 100
bookmarks), this would cause many useless UI updates, slowing the page
down significantly.

This CL adds a 'batch mode' to the data store which suppresses UI
notifications while it is active. Enabling batch mode for API operations
(move, add, delete) makes these >10x faster with large numbers of nodes.

BUG=727177
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2946203002
Cr-Commit-Position: refs/heads/master@{#482838}
Committed: https://chromium.googlesource.com/chromium/src/+/f789a6236652537285fc13dac4cce609e1b94f55

[tsergeant, 2017-06-21 04:20:49]

Description was changed from

==========
MD Bookmarks: Batch updates to the UI when processing deletes and moves

In MD Bookmarks, each action processed from the bookmarks API listeners
would immediately notify the UI about changes that need to be made. In
cases where many notifications are triggered in a row (eg, deleting 100
bookmarks), this would cause many useless UI updates, slowing the page
down significantly.

This CL adds a 'batch mode' to the data store which suppresses UI
notifications while it is active. Enabling batch mode for drag-and-drop
and deletion makes these operations >10x faster with larger numbers of
nodes.

BUG=727177
==========

to

==========
MD Bookmarks: Batch updates to the UI when processing deletes and moves

In MD Bookmarks, each action processed from the bookmarks API listeners
would immediately notify the UI about changes that need to be made. In
cases where many notifications are triggered in a row (eg, deleting 100
bookmarks), this would cause many useless UI updates, slowing the page
down significantly.

This CL adds a 'batch mode' to the data store which suppresses UI
notifications while it is active. Enabling batch mode for drag-and-drop
and deletion makes these operations >10x faster with larger numbers of
nodes.

BUG=727177
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2017-06-21 04:21:08]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 04:21:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 05:38:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 05:39:00]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-06-21 05:57:02]

tsergeant@chromium.org changed reviewers:
+ calamity@chromium.org

[calamity, 2017-06-22 05:48:27]

https://codereview.chromium.org/2946203002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2946203002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/dnd_manager.js:342:
chrome.bookmarkManagerPrivate.drop(
This works within the BMM, but moving a folder in the bookmarks bar will still
cause the burst of move events. Can we start the batch update in onDragEnter and
end it in onDrop instead?

[tsergeant, 2017-06-23 00:20:20]

https://codereview.chromium.org/2946203002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2946203002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/dnd_manager.js:342:
chrome.bookmarkManagerPrivate.drop(
On 2017/06/22 05:48:27, calamity wrote:
> This works within the BMM, but moving a folder in the bookmarks bar will still
> cause the burst of move events. Can we start the batch update in onDragEnter
and
> end it in onDrop instead?

Hmmm, good idea, but I'm not sure that this is possible.

When I drag BMM -> bookmarks bar, the chrome.bookmarksManagerPrivate.onDrop
listener never fires. Instead, we just get onDragLeave and then the
onBookmarkMoved calls.

Even dragging BMM->BMM doesn't work well, since the
bookmarksManagerPrivate.onDrop listener fires before the onBookmarkMoved calls.

[calamity, 2017-06-26 03:17:25]

https://codereview.chromium.org/2946203002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2946203002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/dnd_manager.js:342:
chrome.bookmarkManagerPrivate.drop(
On 2017/06/23 00:20:20, tsergeant wrote:
> On 2017/06/22 05:48:27, calamity wrote:
> > This works within the BMM, but moving a folder in the bookmarks bar will
still
> > cause the burst of move events. Can we start the batch update in onDragEnter
> and
> > end it in onDrop instead?
> 
> Hmmm, good idea, but I'm not sure that this is possible.
> 
> When I drag BMM -> bookmarks bar, the chrome.bookmarksManagerPrivate.onDrop
> listener never fires. Instead, we just get onDragLeave and then the
> onBookmarkMoved calls.
> 
> Even dragging BMM->BMM doesn't work well, since the
> bookmarksManagerPrivate.onDrop listener fires before the onBookmarkMoved
calls.

Hmm. How about just batching in api_listener.js? Having a 100ms debouncer for
all events fired probably isn't unreasonable and would handle all EventStorms.

[tsergeant, 2017-06-27 07:34:39]

Patchset #2 (id:20001) has been deleted

[tsergeant, 2017-06-27 07:35:53]

https://codereview.chromium.org/2946203002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_bookmarks/dnd_manager.js (right):

https://codereview.chromium.org/2946203002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_bookmarks/dnd_manager.js:342:
chrome.bookmarkManagerPrivate.drop(
On 2017/06/26 03:17:24, calamity wrote:
> On 2017/06/23 00:20:20, tsergeant wrote:
> > On 2017/06/22 05:48:27, calamity wrote:
> > > This works within the BMM, but moving a folder in the bookmarks bar will
> still
> > > cause the burst of move events. Can we start the batch update in
onDragEnter
> > and
> > > end it in onDrop instead?
> > 
> > Hmmm, good idea, but I'm not sure that this is possible.
> > 
> > When I drag BMM -> bookmarks bar, the chrome.bookmarksManagerPrivate.onDrop
> > listener never fires. Instead, we just get onDragLeave and then the
> > onBookmarkMoved calls.
> > 
> > Even dragging BMM->BMM doesn't work well, since the
> > bookmarksManagerPrivate.onDrop listener fires before the onBookmarkMoved
> calls.
> 
> Hmm. How about just batching in api_listener.js? Having a 100ms debouncer for
> all events fired probably isn't unreasonable and would handle all EventStorms.

Done. As discussed, I setTimeout seems to work reliably on my machine. We'll
probably need to double check that it works okay on other machines as well after
this lands.

[tsergeant, 2017-06-27 07:38:00]

Description was changed from

==========
MD Bookmarks: Batch updates to the UI when processing deletes and moves

In MD Bookmarks, each action processed from the bookmarks API listeners
would immediately notify the UI about changes that need to be made. In
cases where many notifications are triggered in a row (eg, deleting 100
bookmarks), this would cause many useless UI updates, slowing the page
down significantly.

This CL adds a 'batch mode' to the data store which suppresses UI
notifications while it is active. Enabling batch mode for drag-and-drop
and deletion makes these operations >10x faster with larger numbers of
nodes.

BUG=727177
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Bookmarks: Batch updates to the UI when processing deletes and moves

In MD Bookmarks, each action processed from the bookmarks API listeners
would immediately notify the UI about changes that need to be made. In
cases where many notifications are triggered in a row (eg, deleting 100
bookmarks), this would cause many useless UI updates, slowing the page
down significantly.

This CL adds a 'batch mode' to the data store which suppresses UI
notifications while it is active. Enabling batch mode for API operations
(move, add, delete) makes these >10x faster with large numbers of nodes.

BUG=727177
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2017-06-27 07:38:19]

Description was changed from

==========
MD Bookmarks: Batch updates to the UI when processing deletes and moves

In MD Bookmarks, each action processed from the bookmarks API listeners
would immediately notify the UI about changes that need to be made. In
cases where many notifications are triggered in a row (eg, deleting 100
bookmarks), this would cause many useless UI updates, slowing the page
down significantly.

This CL adds a 'batch mode' to the data store which suppresses UI
notifications while it is active. Enabling batch mode for API operations
(move, add, delete) makes these >10x faster with large numbers of nodes.

BUG=727177
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Bookmarks: Batch updates to the UI when processing API operations

In MD Bookmarks, each action processed from the bookmarks API listeners
would immediately notify the UI about changes that need to be made. In
cases where many notifications are triggered in a row (eg, deleting 100
bookmarks), this would cause many useless UI updates, slowing the page
down significantly.

This CL adds a 'batch mode' to the data store which suppresses UI
notifications while it is active. Enabling batch mode for API operations
(move, add, delete) makes these >10x faster with large numbers of nodes.

BUG=727177
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2017-06-27 07:53:16]

Wew. Like it. Lgtm.

I'm mildly perturbed that this doesn't break any tests, but I guess
api_listener.js is outside the test surface.

[tsergeant, 2017-06-27 23:50:25]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-06-27 23:50:57]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 01:15:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498607425129040,
"parent_rev": "1be4c8f7053d29f3dbc0f7d21d341c04d9e63acc", "commit_rev":
"f789a6236652537285fc13dac4cce609e1b94f55"}

[commit-bot: I haz the power, 2017-06-28 01:15:38]

Description was changed from

==========
MD Bookmarks: Batch updates to the UI when processing API operations

In MD Bookmarks, each action processed from the bookmarks API listeners
would immediately notify the UI about changes that need to be made. In
cases where many notifications are triggered in a row (eg, deleting 100
bookmarks), this would cause many useless UI updates, slowing the page
down significantly.

This CL adds a 'batch mode' to the data store which suppresses UI
notifications while it is active. Enabling batch mode for API operations
(move, add, delete) makes these >10x faster with large numbers of nodes.

BUG=727177
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Bookmarks: Batch updates to the UI when processing API operations

In MD Bookmarks, each action processed from the bookmarks API listeners
would immediately notify the UI about changes that need to be made. In
cases where many notifications are triggered in a row (eg, deleting 100
bookmarks), this would cause many useless UI updates, slowing the page
down significantly.

This CL adds a 'batch mode' to the data store which suppresses UI
notifications while it is active. Enabling batch mode for API operations
(move, add, delete) makes these >10x faster with large numbers of nodes.

BUG=727177
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2946203002
Cr-Commit-Position: refs/heads/master@{#482838}
Committed:
https://chromium.googlesource.com/chromium/src/+/f789a6236652537285fc13dac4cc...
==========

[commit-bot: I haz the power, 2017-06-28 01:15:39]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f789a6236652537285fc13dac4cc...