Bug 71499 – Only walk up the tree in search of MutationObservers if one has been added

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

Only walk up the tree in search of MutationObservers if one has been added

[Adam Klein]

Created attachment 113546 [details]
Patch

[Ojan Vafai]

Comment on attachment 113546 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113546&action=review

> Source/WebCore/ChangeLog:15
> +        This could be improved upon to keep a count of each type, as removing
> +        an observer currently has no effect on m_mutationObserverTypes.
> +        But that would require more space in Document.

So essentially, we do perf optimizations until you add any observer of a type. After that, if you remove the observer, we don't get back the perf optimization. As discussed in person, the reason we're not doing a count is more about code complexity than memory savings since you don't actually have that many documents in a page.

> Source/WebCore/dom/Node.cpp:2550
> +

Mind adding a FIXME to make the old mutation events update their relevant document bits here as well?

> Source/WebCore/dom/Node.cpp:2744
> +    document()->addMutationObserverTypes(options | WebKitMutationObserver::AllMutationTypes);

As discussed in person, we should only do this if it's a subtree observer.

> LayoutTests/fast/mutation/cross-document.html:6
> +<meta charset="utf-8">
> +<script src="../js/resources/js-test-pre.js"></script>
> +<title></title>

Please remove unnecessary bits. title and meta don't need to be here, right?

> LayoutTests/fast/mutation/cross-document.html:10
> +<p id=description></p>
> +<div id="console"></div>

description and console are no long required. js-test-pre will generate these elements for you.

> LayoutTests/fast/mutation/cross-document.html:47
> +    start();

Stylistic nit: I wouldn't have a start method at all. Just inline the contents of the start method here.

[Adam Klein]

Comment on attachment 113546 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113546&action=review

>> Source/WebCore/ChangeLog:15
>> +        But that would require more space in Document.
> 
> So essentially, we do perf optimizations until you add any observer of a type. After that, if you remove the observer, we don't get back the perf optimization. As discussed in person, the reason we're not doing a count is more about code complexity than memory savings since you don't actually have that many documents in a page.

Changed to talk about code complexity instead of space.

> Source/WebCore/dom/Document.h:1248
> +    MutationObserverOptions m_mutationObserverTypes;

Whoops, I forgot to initialize this.  Done in the new patch.

>> Source/WebCore/dom/Node.cpp:2550
>> +
> 
> Mind adding a FIXME to make the old mutation events update their relevant document bits here as well?

Done. As discussed, I don't think it's limited to mutation events, and my FIXME reflects that.

>> Source/WebCore/dom/Node.cpp:2744
>> +    document()->addMutationObserverTypes(options | WebKitMutationObserver::AllMutationTypes);
> 
> As discussed in person, we should only do this if it's a subtree observer.

Done, and renamed to include "Subtree" in the name.

>> LayoutTests/fast/mutation/cross-document.html:6
>> +<title></title>
> 
> Please remove unnecessary bits. title and meta don't need to be here, right?

Removed.

>> LayoutTests/fast/mutation/cross-document.html:10
>> +<div id="console"></div>
> 
> description and console are no long required. js-test-pre will generate these elements for you.

Yay, removed.

>> LayoutTests/fast/mutation/cross-document.html:47
>> +    start();
> 
> Stylistic nit: I wouldn't have a start method at all. Just inline the contents of the start method here.

I'd rather leave as-is for now to match the rest of the tests in this directory...

[Adam Klein]

Created attachment 113560 [details]
Patch for landing

[Adam Klein]

Holding off on submitting until Rafael lands his refactor.

[Ojan Vafai]

Comment on attachment 113560 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=113560&action=review

> LayoutTests/fast/mutation/cross-document.html:5
> +<head>
> +<script src="../js/resources/js-test-pre.js"></script>
> +</head>

Nit: you don't need the head, body or html elements either. But Ryosuke and I disagree about what the best style is here.

[Adam Klein]

Created attachment 113584 [details]
Bail out properly in node removal as well

[Early Warning System Bot]

Comment on attachment 113584 [details]
Bail out properly in node removal as well

Attachment 113584 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10285164

[WebKit Review Bot]

Comment on attachment 113584 [details]
Bail out properly in node removal as well

Attachment 113584 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10301019

[Adam Klein]

Created attachment 113592 [details]
Patch

[Ojan Vafai]

Comment on attachment 113584 [details]
Bail out properly in node removal as well

View in context: https://bugs.webkit.org/attachment.cgi?id=113584&action=review

> Source/WebCore/dom/Node.cpp:2751
> +        document()->addSubtreeMutationObserverTypes(options | WebKitMutationObserver::AllMutationTypes);

This should be &.

[Adam Klein]

Created attachment 113605 [details]
Patch

[Adam Klein]

(In reply to comment #11)
> (From update of attachment 113584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113584&action=review
> 
> > Source/WebCore/dom/Node.cpp:2751
> > +        document()->addSubtreeMutationObserverTypes(options | WebKitMutationObserver::AllMutationTypes);
> 
> This should be &.

Fixed

[Ojan Vafai]

BTW, I'd love to see some performance tests before doing more perf optimizing. Not required, but nice to have.

[Adam Klein]

(In reply to comment #14)
> BTW, I'd love to see some performance tests before doing more perf optimizing. Not required, but nice to have.

So would I.  For the record, this particular optimization was chosen after doing some tests with deeply-nested DOM trees and seeing that the cost of walking up the tree made appendChild() 30-60% slower than baseline even with no observers attached.

[Ojan Vafai]

(In reply to comment #15)
> (In reply to comment #14)
> > BTW, I'd love to see some performance tests before doing more perf optimizing. Not required, but nice to have.
> 
> So would I.  For the record, this particular optimization was chosen after doing some tests with deeply-nested DOM trees and seeing that the cost of walking up the tree made appendChild() 30-60% slower than baseline even with no observers attached.

Oh, I totally trust you tested the perf benefit. I'd just like to see the test checked in so that future people can run the test. You can check it in to Source/WebCore/manual-tests/perf (directory doesn't exist yet).

[Adam Klein]

Created attachment 113919 [details]
Merged with refactor

[Adam Klein]

Created attachment 113922 [details]
Slightly cleaner

[Ryosuke Niwa]

Comment on attachment 113922 [details]
Slightly cleaner

I think Ojan's r+ withstands. If not, then r=me.

[WebKit Review Bot]

Comment on attachment 113922 [details]
Slightly cleaner

Clearing flags on attachment: 113922

Committed r99593: <http://trac.webkit.org/changeset/99593>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.