Bug 71499 - Only walk up the tree in search of MutationObservers if one has been added
Summary: Only walk up the tree in search of MutationObservers if one has been added
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2011-11-03 13:06 PDT by Adam Klein
Modified: 2011-11-08 10:30 PST (History)
8 users (show)

See Also:


Attachments
Patch (8.98 KB, patch)
2011-11-03 13:18 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (11.43 KB, patch)
2011-11-03 14:33 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Bail out properly in node removal as well (12.14 KB, patch)
2011-11-03 17:03 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (12.18 KB, patch)
2011-11-03 17:39 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (12.18 KB, patch)
2011-11-03 18:36 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Merged with refactor (14.26 KB, patch)
2011-11-07 13:41 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Slightly cleaner (14.22 KB, patch)
2011-11-07 13:46 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-11-03 13:06:18 PDT
Only walk up the tree in search of MutationObservers if one has been added
Comment 1 Adam Klein 2011-11-03 13:18:10 PDT
Created attachment 113546 [details]
Patch
Comment 2 Ojan Vafai 2011-11-03 14:14:35 PDT
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.
Comment 3 Adam Klein 2011-11-03 14:33:18 PDT
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...
Comment 4 Adam Klein 2011-11-03 14:33:55 PDT
Created attachment 113560 [details]
Patch for landing
Comment 5 Adam Klein 2011-11-03 14:49:30 PDT
Holding off on submitting until Rafael lands his refactor.
Comment 6 Ojan Vafai 2011-11-03 15:12:55 PDT
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.
Comment 7 Adam Klein 2011-11-03 17:03:06 PDT
Created attachment 113584 [details]
Bail out properly in node removal as well
Comment 8 Early Warning System Bot 2011-11-03 17:25:28 PDT
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
Comment 9 WebKit Review Bot 2011-11-03 17:36:11 PDT
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
Comment 10 Adam Klein 2011-11-03 17:39:17 PDT
Created attachment 113592 [details]
Patch
Comment 11 Ojan Vafai 2011-11-03 18:33:32 PDT
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 &.
Comment 12 Adam Klein 2011-11-03 18:36:11 PDT
Created attachment 113605 [details]
Patch
Comment 13 Adam Klein 2011-11-03 18:36:32 PDT
(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
Comment 14 Ojan Vafai 2011-11-03 18:43:56 PDT
BTW, I'd love to see some performance tests before doing more perf optimizing. Not required, but nice to have.
Comment 15 Adam Klein 2011-11-03 18:48:30 PDT
(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.
Comment 16 Ojan Vafai 2011-11-03 18:50:34 PDT
(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).
Comment 17 Adam Klein 2011-11-07 13:41:11 PST
Created attachment 113919 [details]
Merged with refactor
Comment 18 Adam Klein 2011-11-07 13:46:21 PST
Created attachment 113922 [details]
Slightly cleaner
Comment 19 Ryosuke Niwa 2011-11-07 22:06:20 PST
Comment on attachment 113922 [details]
Slightly cleaner

I think Ojan's r+ withstands. If not, then r=me.
Comment 20 WebKit Review Bot 2011-11-08 10:30:10 PST
Comment on attachment 113922 [details]
Slightly cleaner

Clearing flags on attachment: 113922

Committed r99593: <http://trac.webkit.org/changeset/99593>
Comment 21 WebKit Review Bot 2011-11-08 10:30:17 PST
All reviewed patches have been landed.  Closing bug.