Summary: | Only walk up the tree in search of MutationObservers if one has been added | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Klein <adamk> | ||||||||||||||||
Component: | New Bugs | Assignee: | Adam Klein <adamk> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | darin, dglazkov, mjs, ojan, rafaelw, rniwa, sam, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 68729 | ||||||||||||||||||
Attachments: |
|
Description
Adam Klein
2011-11-03 13:06:18 PDT
Created attachment 113546 [details]
Patch
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 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... Created attachment 113560 [details]
Patch for landing
Holding off on submitting until Rafael lands his refactor. 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. Created attachment 113584 [details]
Bail out properly in node removal as well
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 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 Created attachment 113592 [details]
Patch
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 &. Created attachment 113605 [details]
Patch
(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 BTW, I'd love to see some performance tests before doing more perf optimizing. Not required, but nice to have. (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. (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). Created attachment 113919 [details]
Merged with refactor
Created attachment 113922 [details]
Slightly cleaner
Comment on attachment 113922 [details]
Slightly cleaner
I think Ojan's r+ withstands. If not, then r=me.
Comment on attachment 113922 [details] Slightly cleaner Clearing flags on attachment: 113922 Committed r99593: <http://trac.webkit.org/changeset/99593> All reviewed patches have been landed. Closing bug. |