Closed Bug 1188640 Opened 5 years ago Closed 5 years ago

Add ChromeOnly MutationObserver.mergeAttributeRecords to speed up devtools

Categories

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

36 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(3 files)

Attached patch patchSplinter Review
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: → 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+
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+
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!
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>
File bug 1189067 for that.
Filed...
https://hg.mozilla.org/mozilla-central/rev/6d84fd4ea8c1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1192270
Depends on: 1209342
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.