1503036 - Track Changes - Consolidate similar recent events on the server before dispatching events to the client

Status: NEW

(DevTools :: Inspector: Changes, enhancement, P2)

Description

[Brad Werth [:bradwerth]]

There are a few methods of making changes that will produce a rapid series of track-changes events:
1) Typing. Each keystroke has the potential to send an event.
2) Dragging a control point or manipulating a slider will send multiple events.

The server has an opportunity to capture these similar events and coalesce them before passing them on to the client. Doing so will reduce the protocol traffic between server and client, hopefully avoiding performance penalties.

Comment 1

[Patrick Brosset <:pbro>]

Doing this will also allow us to consider many minute changes on one input (like a slider) as one 1 big change, which will become helpful when we implement undo/redo to avoid polluting the stack with hundreds of small changes.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1503036 Part 1: Add methods to ChangesActor to detect mergeable changes and to do the merge itself. r?rcaliman!

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1503036 Part 2: Make ChangesActor coalesce mergable changes and send them after a delay. r?rcaliman!

Depends on D10519

Comment 5

[Alexandre Poirot [:ochameau]]

I do not see any test in this patch queue.

It may be worth asserting that we really do aggregate some changes?
I was also wondering if that could be worth tracking overall performance of all non-default panels.
Today, DAMP only tracks the performance of the panels which are displayed by default (rules and layout?).
It may be worth adding specific tests to the changes and font panels and/or have a test script doing intense CSS mutations?

We do have a test script which does DOM mutations (with the default panels):
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/tests/inspector/mutations.js#18-60
But we don't have any which does CSS mutations.

Comment 6

[Razvan Caliman [:rcaliman]]

(In reply to Alexandre Poirot [:ochameau] from comment #5)

I do not see any test in this patch queue.

It may be worth asserting that we really do aggregate some changes?

Indeed, we don't have a test. We will have a test to for this when landing. This patch is still a work in progress. It suffered due to the changing nature of the changes structure (pun not intended). It has stabilized now.

This piece of code has the potential to impact the accuracy of aggregating changes. It will need guards to ensure important changes don't get dropped (but similar ones do get coalesced).

I was also wondering if that could be worth tracking overall performance of all non-default panels.

Good idea.

Today, DAMP only tracks the performance of the panels which are displayed by default (rules and layout?).

Correct.

It may be worth adding specific tests to the changes and font panels and/or have a test script doing intense CSS mutations?

The tricky part of the Changes panel is that it aggregates changes happening as a result of user interaction with other panels/tools like the Rules view, Fonts panel, Shape Path Editor, Color Picker, etc. Regular CSSOM mutations are not tracked (i.e. mutating the stylesheet with a script). This means spawning different tools and using them to generate changes so the Changes panel can aggregate them. This has the caveat that if other tools change performance metrics, they may taint the Changes panel test too.

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1503036 Part 3: Make ChangesActor consider the last committed change for mergeability.

This makes it harder to create a stack of changes that could have been merged
if only they were performed more quickly. This breaks the concept of a stable
undo stack on the server.

Depends on D12073

Comment 8

[Razvan Caliman [:rcaliman]]

See also: https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/devtools/client/inspector/compatibility/CompatibilityView.js#107

Comment 10

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12d278313149dadd6510152b5017eecf4f2c44f7

Comment 11

[Brad Werth [:bradwerth]]

Attachment: Bug 1503036 Part 3: Update test expectations to not wait for redundant events.

This patch updates 3 tests to update these expectations:

1. Changes no longer dispatch immediately.
2. Deletion of a disabled rule is no longer emitted as a change.
3. Because of issue 2, the order of some changes is different.

For safety, all 3 tests now request a longer timeout.

Depends on D12073

Comment 12

[Brad Werth [:bradwerth]]

I'm not working in this area anymore. Taking myself off the bug to make it clear that somebody else can pick it up.