Add ChromeOnly MutationObserver.mergeAttributeRecords to speed up devtools

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

36 Branch
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
Created attachment 8640172 [details] [diff] [review]
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.
Attachment #8640172 - Flags: review?(bzbarsky)
Attachment #8640172 - Flags: review?(bgrinstead)
See Also: → bug 1145914
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
Attachment #8640172 - Flags: review?(bgrinstead) → review+
Created attachment 8640194 [details] [diff] [review]
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 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
Attachment #8640172 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

3 years ago
well, info() didn't do what I wanted. It didn't show up in the UI when looking at the test results.
Also, this makes the testcase in bug 1188526 _much_ happier!
(Assignee)

Comment 6

3 years ago
And MergeableAttributeRecord has documentation in .cpp. I guess I could move it to .h
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>
(Assignee)

Comment 9

3 years ago
File bug 1189067 for that.
(Assignee)

Comment 10

3 years ago
Filed...
https://hg.mozilla.org/mozilla-central/rev/6d84fd4ea8c1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

3 years ago
Depends on: 1192270
Depends on: 1209342
You need to log in before you can comment on or make changes to this bug.