1188640 - Add ChromeOnly MutationObserver.mergeAttributeRecords to speed up devtools

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

I decided to add the flag to MutationObserver and not to the MutationObserverInit dictionary, since the flag is not about what is being observed, but about what will be delivered to the callback.
And the implementation let's then one to set the flag at any point and all the record that will be delivered after that
get the same handling.

This changes the profile for Bug 1188526 quite a bit.
	
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa44c76c8e59

Not super important bug, but this was easy to do.
And if we figure out how to add more filtering to the MutationObserver, we could perhaps speed up other cases too in devtools.
(and add more flags then)


bgrins, just look at the end of the patch.

Comment 1

[Brian Grinstead [:bgrins]]

Comment on attachment 8640172 [details] [diff] [review]
patch

Review of attachment 8640172 [details] [diff] [review]:
-----------------------------------------------------------------

Devtools change looks good.  I think we can get rid of the actor's duplication removal if the mutation observer handles it for us now.  I'll upload a patch that does that

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: remove-dedupe-inspector.patch

This should be able to be folded into the patch.  I tested locally with this bit removed on a page with duplicate mutations and on test_inspector-mutations-attr.html, and both worked correctly

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8640172 [details] [diff] [review]
patch

>+  bool MergeableAttributeRecord(nsDOMMutationRecord* aOldRecord,
>+                                nsDOMMutationRecord* aRecord);

Could use documentation.

>+  ok(true, "testAttributeRecordMerging1");

Should this just be an info()?  Same for the other tests.

r=me

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

well, info() didn't do what I wanted. It didn't show up in the UI when looking at the test results.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Also, this makes the testcase in bug 1188526 _much_ happier!

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

And MergeableAttributeRecord has documentation in .cpp. I guess I could move it to .h

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: merge + ?: fix, making compilers happier

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34e852d1bb9f

Comment 8

[Brian Grinstead [:bgrins]]

It'd be great if we could have similar functionality for characterData mutations also.  Not sure how much that would affect the particular testcase in bug 1188526, but we aren't even doing any pruning on the protocol end for that, so I'd imagine a case that had a lot of those mutations queued up would be very slow with the inspector opened:

data:text/html,<h2> </h2><script>setInterval(() => {for (var i = 0; i < 1000; i++) {document.querySelector("h2").childNodes[0].nodeValue = i;}}, 100)</script>

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

File bug 1189067 for that.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Filed...

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d84fd4ea8c1

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d84fd4ea8c1