RESOLVED FIXED 97352
Simplify and optimize ChildListMutationScope
https://bugs.webkit.org/show_bug.cgi?id=97352
Summary Simplify and optimize ChildListMutationScope
Adam Klein
Reported 2012-09-21 11:48:19 PDT
Simplify and optimize ChildListMutationScope
Attachments
Patch (16.69 KB, patch)
2012-09-21 12:02 PDT, Adam Klein
no flags
Patch (16.52 KB, patch)
2012-09-21 12:06 PDT, Adam Klein
no flags
Patch (17.01 KB, patch)
2012-09-21 14:09 PDT, Adam Klein
no flags
Patch (17.00 KB, patch)
2012-09-21 14:10 PDT, Adam Klein
no flags
Patch (16.98 KB, patch)
2012-09-21 15:40 PDT, Adam Klein
no flags
Alternate approach: RefCount MutationAccumulator (17.08 KB, patch)
2012-09-21 16:12 PDT, Adam Klein
no flags
Patch for landing (17.12 KB, patch)
2012-09-21 16:49 PDT, Adam Klein
no flags
Adam Klein
Comment 1 2012-09-21 12:02:45 PDT
Adam Klein
Comment 2 2012-09-21 12:06:26 PDT
Ryosuke Niwa
Comment 3 2012-09-21 12:39:19 PDT
Comment on attachment 165169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165169&action=review This looks right. > Source/WebCore/dom/ChildListMutationScope.cpp:70 > + if (!m_observers) > + return; I would move this check to ChildListMutationScope so that it can be inlined into ContainerNode member functions if I were you. > Source/WebCore/dom/ChildListMutationScope.cpp:115 > m_target, StaticNodeList::adopt(m_addedNodes), StaticNodeList::adopt(m_removedNodes), m_previousSibling.release(), m_nextSibling.release())); Nit: Wrong indentation (I know this is an existing code but...) > Source/WebCore/dom/ChildListMutationScope.cpp:149 > + delete this; I'm not a big fun of manually calling delete like this. > Source/WebCore/dom/ChildListMutationScope.h:96 > + typedef HashMap<Node*, MutationAccumulator*> AccumulatorMap; Can we use HashMap<Node*, OwnPtr<MutationAccumulator> > instead so as to avoid manual new/delete?
Adam Klein
Comment 4 2012-09-21 14:07:10 PDT
Comment on attachment 165169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165169&action=review Note that in my next patch I've pulled out MutationAccumulator so it's no longer an inner class. This is mostly just to make the number of colons-per-line less ridiculous in the .cpp file. I don't think there's much danger of someone trying to use this class, but I left a comment at the top saying not to anyway. >> Source/WebCore/dom/ChildListMutationScope.cpp:70 >> + return; > > I would move this check to ChildListMutationScope so that it can be inlined into ContainerNode member functions if I were you. Yeah, I guess that could be worth it. I had it that way initially but moved it to simplify the .h file, but I guess there's not much advantage to doing it that way. Fixed (and replaced with an ASSERT). >> Source/WebCore/dom/ChildListMutationScope.cpp:115 >> m_target, StaticNodeList::adopt(m_addedNodes), StaticNodeList::adopt(m_removedNodes), m_previousSibling.release(), m_nextSibling.release())); > > Nit: Wrong indentation (I know this is an existing code but...) Split into 4 statements for clarity. >> Source/WebCore/dom/ChildListMutationScope.cpp:149 >> + delete this; > > I'm not a big fun of manually calling delete like this. I did this so I could keep the destructor private. Otherwise I'd want to comment the destructor to require that it not be called. Let me know which you prefer. >> Source/WebCore/dom/ChildListMutationScope.h:96 >> + typedef HashMap<Node*, MutationAccumulator*> AccumulatorMap; > > Can we use HashMap<Node*, OwnPtr<MutationAccumulator> > instead so as to avoid manual new/delete? Was using that before, but wanted to make the destructor private. See above.
Adam Klein
Comment 5 2012-09-21 14:09:02 PDT
Adam Klein
Comment 6 2012-09-21 14:10:10 PDT
Adam Klein
Comment 7 2012-09-21 15:40:15 PDT
Ryosuke Niwa
Comment 8 2012-09-21 16:01:18 PDT
(In reply to comment #4) > >> Source/WebCore/dom/ChildListMutationScope.cpp:149 > >> + delete this; > > > > I'm not a big fun of manually calling delete like this. > > I did this so I could keep the destructor private. Otherwise I'd want to comment the destructor to require that it not be called. Let me know which you prefer. What's the advantage of making the destructor private?
Adam Klein
Comment 9 2012-09-21 16:12:10 PDT
Created attachment 165220 [details] Alternate approach: RefCount MutationAccumulator
Adam Klein
Comment 10 2012-09-21 16:13:27 PDT
(In reply to comment #8) > (In reply to comment #4) > > >> Source/WebCore/dom/ChildListMutationScope.cpp:149 > > >> + delete this; > > > > > > I'm not a big fun of manually calling delete like this. > > > > I did this so I could keep the destructor private. Otherwise I'd want to comment the destructor to require that it not be called. Let me know which you prefer. > > What's the advantage of making the destructor private? Just trying to avoid mis-use. Take a look at the latest patch, where I ditch m_scopingLevel entirely since it's effectively a refcount, and use RefPtrs in the scopes to handle all this automatically.
Ryosuke Niwa
Comment 11 2012-09-21 16:16:21 PDT
Comment on attachment 165213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review > Source/WebCore/dom/ChildListMutationScope.h:53 > +class MutationAccumulator { I would have named this ChildListMutationAccumulator now that it's not an inner class. > Source/WebCore/dom/ChildListMutationScope.h:103 > + if (m_accumulator && m_accumulator->hasObservers()) Maybe we need some clarification as to when m_accumulator->hasObservers() && !m_accumulator->hasObservers() is true.
Ryosuke Niwa
Comment 12 2012-09-21 16:17:39 PDT
Comment on attachment 165220 [details] Alternate approach: RefCount MutationAccumulator OMG, this code looks sooo much prettier!
Adam Klein
Comment 13 2012-09-21 16:31:17 PDT
Comment on attachment 165213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review >> Source/WebCore/dom/ChildListMutationScope.h:53 >> +class MutationAccumulator { > > I would have named this ChildListMutationAccumulator now that it's not an inner class. Well, half the reason I moved it out was to shorten the name, so I'm not sure it's worth it. Will leave as-is until it conflicts with something else. It's still "private API" from the point of view of ChildListMutationScope, so it's only a worry as far as the one-definition rule goes. >> Source/WebCore/dom/ChildListMutationScope.h:103 >> + if (m_accumulator && m_accumulator->hasObservers()) > > Maybe we need some clarification as to when m_accumulator->hasObservers() && !m_accumulator->hasObservers() is true. Not sure what you mean. Is hasObservers() not a clear enough name? Or are you wondering if there's further simplification I can do here?
Ryosuke Niwa
Comment 14 2012-09-21 16:34:24 PDT
(In reply to comment #13) > (From update of attachment 165213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review > > >> Source/WebCore/dom/ChildListMutationScope.h:53 > >> +class MutationAccumulator { > > > > I would have named this ChildListMutationAccumulator now that it's not an inner class. > > Well, half the reason I moved it out was to shorten the name, so I'm not sure it's worth it. Will leave as-is until it conflicts with something else. It's still "private API" from the point of view of ChildListMutationScope, so it's only a worry as far as the one-definition rule goes. Autocompletion on IDEs will suffer... :( > >> Source/WebCore/dom/ChildListMutationScope.h:103 > >> + if (m_accumulator && m_accumulator->hasObservers()) > > > > Maybe we need some clarification as to when m_accumulator->hasObservers() && !m_accumulator->hasObservers() is true. > > Not sure what you mean. Is hasObservers() not a clear enough name? Or are you wondering if there's further simplification I can do here? What's not clear is why we would not have observers here given that we had already checked that we had interested observers when we created m_accumulator.
Adam Klein
Comment 15 2012-09-21 16:39:06 PDT
Comment on attachment 165213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review >>>> Source/WebCore/dom/ChildListMutationScope.h:53 >>>> +class MutationAccumulator { >>> >>> I would have named this ChildListMutationAccumulator now that it's not an inner class. >> >> Well, half the reason I moved it out was to shorten the name, so I'm not sure it's worth it. Will leave as-is until it conflicts with something else. It's still "private API" from the point of view of ChildListMutationScope, so it's only a worry as far as the one-definition rule goes. > > Autocompletion on IDEs will suffer... :( Hmm, ok. I'll do some followups to move stuff around/rename, but I'd like to be done with this patch for now before I accidentally break it. >>>> Source/WebCore/dom/ChildListMutationScope.h:103 >>>> + if (m_accumulator && m_accumulator->hasObservers()) >>> >>> Maybe we need some clarification as to when m_accumulator->hasObservers() && !m_accumulator->hasObservers() is true. >> >> Not sure what you mean. Is hasObservers() not a clear enough name? Or are you wondering if there's further simplification I can do here? > > What's not clear is why we would not have observers here given that we had already checked that we had interested observers when we created m_accumulator. So the document-level flag only tells you if there are observers _somewhere_ in the document. hasObservers() tells you whether there are actually observers in this particular subtree. That's why we create the accumulator even if there aren't any observers in the subtree: it allows us to cache the tree-walking. Not sure how naming would help here: the fact that hasMutationObserversOfType() is on Document, and that it isn't given a context node, should be a hint that it's not getting the complete story.
Adam Klein
Comment 16 2012-09-21 16:49:19 PDT
Created attachment 165226 [details] Patch for landing
Adam Klein
Comment 17 2012-09-21 16:49:43 PDT
(In reply to comment #15) > (From update of attachment 165213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165213&action=review > > >>>> Source/WebCore/dom/ChildListMutationScope.h:53 > >>>> +class MutationAccumulator { > >>> > >>> I would have named this ChildListMutationAccumulator now that it's not an inner class. > >> > >> Well, half the reason I moved it out was to shorten the name, so I'm not sure it's worth it. Will leave as-is until it conflicts with something else. It's still "private API" from the point of view of ChildListMutationScope, so it's only a worry as far as the one-definition rule goes. > > > > Autocompletion on IDEs will suffer... :( > > Hmm, ok. I'll do some followups to move stuff around/rename, but I'd like to be done with this patch for now before I accidentally break it. Did this rename for now, since I had to fix up the ChangeLog anyway.
WebKit Review Bot
Comment 18 2012-09-21 17:30:27 PDT
Comment on attachment 165226 [details] Patch for landing Clearing flags on attachment: 165226 Committed r129280: <http://trac.webkit.org/changeset/129280>
WebKit Review Bot
Comment 19 2012-09-21 17:30:31 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.