- Revision
- 129280
- Author
- ad...@chromium.org
- Date
- 2012-09-21 17:30:48 -0700 (Fri, 21 Sep 2012)
Log Message
Simplify and optimize ChildListMutationScope
https://bugs.webkit.org/show_bug.cgi?id=97352
Reviewed by Ryosuke Niwa.
ChildListMutationScope is one of the most complicated bits of
MutationObserver implementation. This patch aims to simplify it for
clarity and improve its performance (mostly by just doing less).
The big change is to remove the MutationAccumulatorRouter class,
replacing it with lifetime-management logic in ChildListMutationAccumulator
ChildListMutationScope is expected to call getOrCreate() in
its constructor, and each scope holds a RefPtr to the accumulator.
When the last scope holding such a RefPtr is destroyed,
ChildListMutationAccumulator's destructor enqueues the accumulated record.
This greatly reduces the number of lines of code, and condenses
two HashMaps into one. It also reduces hash lookups, which now
occur only on scope creation and when the refcount for a given
accumulator reaches 0 (previously, each childAdded and willRemoveChild
call could result in two hash lookups each).
There are some minor changes as well: the ChildListMutationAccumulator::clear()
method is gone, as it was doing more work than necessary;
DEFINE_STATIC_LOCAL is now used instead of hand-rolled static-management
code; ChildListMutationAccumulator::m_lastAdded is no longer a RefPtr, since it
always points at a Node that's already being ref'd by the accumulator.
Also various minor syntactic cleanups.
No new tests, no change in behavior.
* dom/ChildListMutationScope.cpp:
(WebCore::accumulatorMap): Reduced two maps to one, and manage its lifetime with DEFINE_STATIC_LOCAL.
(WebCore::ChildListMutationAccumulator::ChildListMutationAccumulator): Remove unnecessary call to clear() (which itself has been removed).
(WebCore::ChildListMutationAccumulator::~ChildListMutationAccumulator): Enqueue record if not empty at destruction, and have the accumulator
remove itself from the map.
(WebCore::ChildListMutationAccumulator::getOrCreate): Replaces half of MutationAccumulatorRouter's job.
(WebCore::ChildListMutationAccumulator::childAdded): Minor RefPtr usage improvements.
(WebCore::ChildListMutationAccumulator::isRemovedNodeInOrder): Simplify RefPtr syntax.
(WebCore::ChildListMutationAccumulator::willRemoveChild): Minor RefPtr usage improvements.
(WebCore::ChildListMutationAccumulator::enqueueMutationRecord): Replace call to clear() with clearing m_lastAdded,
since it's the only bit not cleared by the MutationRecord creation call. Also remove
isEmpty check and replace with asserts now that it's a private method.
(WebCore::ChildListMutationAccumulator::isEmpty): Added more assertions about emptiness.
* dom/ChildListMutationScope.h:
(WebCore):
(ChildListMutationAccumulator): Extract the inner class to make everything easier to read.
(WebCore::ChildListMutationScope::ChildListMutationScope): Store m_accumulator rather than m_target.
(WebCore::ChildListMutationScope::~ChildListMutationScope): ditto
(WebCore::ChildListMutationScope::childAdded): ditto
(WebCore::ChildListMutationScope::willRemoveChild): ditto
(ChildListMutationScope):
* html/HTMLElement.cpp: Remove unused ChildListMutationScope.h #include.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (129279 => 129280)
--- trunk/Source/WebCore/ChangeLog 2012-09-22 00:16:45 UTC (rev 129279)
+++ trunk/Source/WebCore/ChangeLog 2012-09-22 00:30:48 UTC (rev 129280)
@@ -1,3 +1,59 @@
+2012-09-21 Adam Klein <ad...@chromium.org>
+
+ Simplify and optimize ChildListMutationScope
+ https://bugs.webkit.org/show_bug.cgi?id=97352
+
+ Reviewed by Ryosuke Niwa.
+
+ ChildListMutationScope is one of the most complicated bits of
+ MutationObserver implementation. This patch aims to simplify it for
+ clarity and improve its performance (mostly by just doing less).
+
+ The big change is to remove the MutationAccumulatorRouter class,
+ replacing it with lifetime-management logic in ChildListMutationAccumulator
+ ChildListMutationScope is expected to call getOrCreate() in
+ its constructor, and each scope holds a RefPtr to the accumulator.
+ When the last scope holding such a RefPtr is destroyed,
+ ChildListMutationAccumulator's destructor enqueues the accumulated record.
+
+ This greatly reduces the number of lines of code, and condenses
+ two HashMaps into one. It also reduces hash lookups, which now
+ occur only on scope creation and when the refcount for a given
+ accumulator reaches 0 (previously, each childAdded and willRemoveChild
+ call could result in two hash lookups each).
+
+ There are some minor changes as well: the ChildListMutationAccumulator::clear()
+ method is gone, as it was doing more work than necessary;
+ DEFINE_STATIC_LOCAL is now used instead of hand-rolled static-management
+ code; ChildListMutationAccumulator::m_lastAdded is no longer a RefPtr, since it
+ always points at a Node that's already being ref'd by the accumulator.
+ Also various minor syntactic cleanups.
+
+ No new tests, no change in behavior.
+
+ * dom/ChildListMutationScope.cpp:
+ (WebCore::accumulatorMap): Reduced two maps to one, and manage its lifetime with DEFINE_STATIC_LOCAL.
+ (WebCore::ChildListMutationAccumulator::ChildListMutationAccumulator): Remove unnecessary call to clear() (which itself has been removed).
+ (WebCore::ChildListMutationAccumulator::~ChildListMutationAccumulator): Enqueue record if not empty at destruction, and have the accumulator
+ remove itself from the map.
+ (WebCore::ChildListMutationAccumulator::getOrCreate): Replaces half of MutationAccumulatorRouter's job.
+ (WebCore::ChildListMutationAccumulator::childAdded): Minor RefPtr usage improvements.
+ (WebCore::ChildListMutationAccumulator::isRemovedNodeInOrder): Simplify RefPtr syntax.
+ (WebCore::ChildListMutationAccumulator::willRemoveChild): Minor RefPtr usage improvements.
+ (WebCore::ChildListMutationAccumulator::enqueueMutationRecord): Replace call to clear() with clearing m_lastAdded,
+ since it's the only bit not cleared by the MutationRecord creation call. Also remove
+ isEmpty check and replace with asserts now that it's a private method.
+ (WebCore::ChildListMutationAccumulator::isEmpty): Added more assertions about emptiness.
+ * dom/ChildListMutationScope.h:
+ (WebCore):
+ (ChildListMutationAccumulator): Extract the inner class to make everything easier to read.
+ (WebCore::ChildListMutationScope::ChildListMutationScope): Store m_accumulator rather than m_target.
+ (WebCore::ChildListMutationScope::~ChildListMutationScope): ditto
+ (WebCore::ChildListMutationScope::childAdded): ditto
+ (WebCore::ChildListMutationScope::willRemoveChild): ditto
+ (ChildListMutationScope):
+ * html/HTMLElement.cpp: Remove unused ChildListMutationScope.h #include.
+
2012-09-21 Chris Rogers <crog...@google.com>
BiquadFilterNode must take audio-rate parameter changes into account
Modified: trunk/Source/WebCore/dom/ChildListMutationScope.cpp (129279 => 129280)
--- trunk/Source/WebCore/dom/ChildListMutationScope.cpp 2012-09-22 00:16:45 UTC (rev 129279)
+++ trunk/Source/WebCore/dom/ChildListMutationScope.cpp 2012-09-22 00:30:48 UTC (rev 129280)
@@ -42,60 +42,54 @@
#include "StaticNodeList.h"
#include <wtf/HashMap.h>
#include <wtf/OwnPtr.h>
+#include <wtf/StdLibExtras.h>
namespace WebCore {
-// MutationAccumulator expects that all removals from a parent take place in order
-// and precede any additions. If this is violated (i.e. because of code changes elsewhere
-// in WebCore) it will likely result in both (a) ASSERTions failing, and (b) mutation records
-// being enqueued for delivery before the outer-most scope closes.
-class ChildListMutationScope::MutationAccumulator {
- WTF_MAKE_NONCOPYABLE(MutationAccumulator);
-public:
- MutationAccumulator(PassRefPtr<Node>, PassOwnPtr<MutationObserverInterestGroup> observers);
- ~MutationAccumulator();
+typedef HashMap<Node*, ChildListMutationAccumulator*> AccumulatorMap;
+static AccumulatorMap& accumulatorMap()
+{
+ DEFINE_STATIC_LOCAL(AccumulatorMap, map, ());
+ return map;
+}
- void childAdded(PassRefPtr<Node>);
- void willRemoveChild(PassRefPtr<Node>);
- void enqueueMutationRecord();
-
-private:
- void clear();
- bool isEmpty();
- bool isAddedNodeInOrder(Node*);
- bool isRemovedNodeInOrder(Node*);
- RefPtr<Node> m_target;
-
- Vector<RefPtr<Node> > m_removedNodes;
- Vector<RefPtr<Node> > m_addedNodes;
- RefPtr<Node> m_previousSibling;
- RefPtr<Node> m_nextSibling;
- RefPtr<Node> m_lastAdded;
-
- OwnPtr<MutationObserverInterestGroup> m_observers;
-};
-
-ChildListMutationScope::MutationAccumulator::MutationAccumulator(PassRefPtr<Node> target, PassOwnPtr<MutationObserverInterestGroup> observers)
+ChildListMutationAccumulator::ChildListMutationAccumulator(PassRefPtr<Node> target, PassOwnPtr<MutationObserverInterestGroup> observers)
: m_target(target)
+ , m_lastAdded(0)
, m_observers(observers)
{
- clear();
}
-ChildListMutationScope::MutationAccumulator::~MutationAccumulator()
+ChildListMutationAccumulator::~ChildListMutationAccumulator()
{
- ASSERT(isEmpty());
+ if (!isEmpty())
+ enqueueMutationRecord();
+ accumulatorMap().remove(m_target.get());
}
-inline bool ChildListMutationScope::MutationAccumulator::isAddedNodeInOrder(Node* child)
+PassRefPtr<ChildListMutationAccumulator> ChildListMutationAccumulator::getOrCreate(Node* target)
{
+ AccumulatorMap::AddResult result = accumulatorMap().add(target, 0);
+ RefPtr<ChildListMutationAccumulator> accumulator;
+ if (!result.isNewEntry)
+ accumulator = result.iterator->second;
+ else {
+ accumulator = adoptRef(new ChildListMutationAccumulator(target, MutationObserverInterestGroup::createForChildListMutation(target)));
+ result.iterator->second = accumulator.get();
+ }
+ return accumulator.release();
+}
+
+inline bool ChildListMutationAccumulator::isAddedNodeInOrder(Node* child)
+{
return isEmpty() || (m_lastAdded == child->previousSibling() && m_nextSibling == child->nextSibling());
}
-void ChildListMutationScope::MutationAccumulator::childAdded(PassRefPtr<Node> prpChild)
+void ChildListMutationAccumulator::childAdded(PassRefPtr<Node> prpChild)
{
RefPtr<Node> child = prpChild;
+ ASSERT(hasObservers());
ASSERT(isAddedNodeInOrder(child.get()));
if (!isAddedNodeInOrder(child.get()))
enqueueMutationRecord();
@@ -105,21 +99,21 @@
m_nextSibling = child->nextSibling();
}
- m_addedNodes.append(child);
- m_lastAdded = child;
+ m_lastAdded = child.get();
+ m_addedNodes.append(child.release());
}
-inline bool ChildListMutationScope::MutationAccumulator::isRemovedNodeInOrder(Node* child)
+inline bool ChildListMutationAccumulator::isRemovedNodeInOrder(Node* child)
{
- return isEmpty() || m_nextSibling.get() == child;
+ return isEmpty() || m_nextSibling == child;
}
-void ChildListMutationScope::MutationAccumulator::willRemoveChild(PassRefPtr<Node> prpChild)
+void ChildListMutationAccumulator::willRemoveChild(PassRefPtr<Node> prpChild)
{
RefPtr<Node> child = prpChild;
+ ASSERT(hasObservers());
ASSERT(m_addedNodes.isEmpty() && isRemovedNodeInOrder(child.get()));
-
if (!m_addedNodes.isEmpty() || !isRemovedNodeInOrder(child.get()))
enqueueMutationRecord();
@@ -130,116 +124,35 @@
} else
m_nextSibling = child->nextSibling();
- m_removedNodes.append(child);
+ m_removedNodes.append(child.release());
}
-void ChildListMutationScope::MutationAccumulator::enqueueMutationRecord()
+void ChildListMutationAccumulator::enqueueMutationRecord()
{
- if (isEmpty()) {
- clear();
- return;
- }
+ ASSERT(hasObservers());
+ ASSERT(!isEmpty());
- m_observers->enqueueMutationRecord(MutationRecord::createChildList(
- m_target, StaticNodeList::adopt(m_addedNodes), StaticNodeList::adopt(m_removedNodes), m_previousSibling.release(), m_nextSibling.release()));
- clear();
-}
-
-void ChildListMutationScope::MutationAccumulator::clear()
-{
- if (!m_removedNodes.isEmpty())
- m_removedNodes.clear();
- if (!m_addedNodes.isEmpty())
- m_addedNodes.clear();
- m_previousSibling = 0;
- m_nextSibling = 0;
+ RefPtr<NodeList> addedNodes = StaticNodeList::adopt(m_addedNodes);
+ RefPtr<NodeList> removedNodes = StaticNodeList::adopt(m_removedNodes);
+ RefPtr<MutationRecord> record = MutationRecord::createChildList(m_target, addedNodes.release(), removedNodes.release(), m_previousSibling.release(), m_nextSibling.release());
+ m_observers->enqueueMutationRecord(record.release());
m_lastAdded = 0;
+ ASSERT(isEmpty());
}
-bool ChildListMutationScope::MutationAccumulator::isEmpty()
+bool ChildListMutationAccumulator::isEmpty()
{
- return m_removedNodes.isEmpty() && m_addedNodes.isEmpty();
-}
-
-ChildListMutationScope::MutationAccumulationRouter* ChildListMutationScope::MutationAccumulationRouter::s_instance = 0;
-
-ChildListMutationScope::MutationAccumulationRouter::MutationAccumulationRouter()
-{
-}
-
-ChildListMutationScope::MutationAccumulationRouter::~MutationAccumulationRouter()
-{
- ASSERT(m_scopingLevels.isEmpty());
- ASSERT(m_accumulations.isEmpty());
-}
-
-void ChildListMutationScope::MutationAccumulationRouter::initialize()
-{
- ASSERT(!s_instance);
- s_instance = adoptPtr(new ChildListMutationScope::MutationAccumulationRouter).leakPtr();
-}
-
-
-ChildListMutationScope::MutationAccumulationRouter* ChildListMutationScope::MutationAccumulationRouter::instance()
-{
- if (!s_instance)
- initialize();
-
- return s_instance;
-}
-
-void ChildListMutationScope::MutationAccumulationRouter::childAdded(Node* target, Node* child)
-{
- HashMap<Node*, OwnPtr<ChildListMutationScope::MutationAccumulator> >::iterator iter = m_accumulations.find(target);
- ASSERT(iter != m_accumulations.end());
- if (iter == m_accumulations.end() || !iter->second)
- return;
-
- iter->second->childAdded(child);
-}
-
-void ChildListMutationScope::MutationAccumulationRouter::willRemoveChild(Node* target, Node* child)
-{
- HashMap<Node*, OwnPtr<ChildListMutationScope::MutationAccumulator> >::iterator iter = m_accumulations.find(target);
- ASSERT(iter != m_accumulations.end());
- if (iter == m_accumulations.end() || !iter->second)
- return;
-
- iter->second->willRemoveChild(child);
-}
-
-void ChildListMutationScope::MutationAccumulationRouter::incrementScopingLevel(Node* target)
-{
- ScopingLevelMap::AddResult result = m_scopingLevels.add(target, 1);
- if (!result.isNewEntry) {
- ++(result.iterator->second);
- return;
- }
-
- if (OwnPtr<MutationObserverInterestGroup> observers = MutationObserverInterestGroup::createForChildListMutation(target))
- m_accumulations.set(target, adoptPtr(new ChildListMutationScope::MutationAccumulator(target, observers.release())));
+ bool result = m_removedNodes.isEmpty() && m_addedNodes.isEmpty();
#ifndef NDEBUG
- else
- m_accumulations.set(target, nullptr);
+ if (result) {
+ ASSERT(!m_previousSibling);
+ ASSERT(!m_nextSibling);
+ ASSERT(!m_lastAdded);
+ }
#endif
+ return result;
}
-void ChildListMutationScope::MutationAccumulationRouter::decrementScopingLevel(Node* target)
-{
- ScopingLevelMap::iterator iter = m_scopingLevels.find(target);
- ASSERT(iter != m_scopingLevels.end());
-
- --(iter->second);
- if (iter->second > 0)
- return;
-
- m_scopingLevels.remove(iter);
-
- OwnPtr<MutationAccumulator> record(m_accumulations.take(target));
- if (record)
- record->enqueueMutationRecord();
-}
-
} // namespace WebCore
#endif // ENABLE(MUTATION_OBSERVERS)
Modified: trunk/Source/WebCore/dom/ChildListMutationScope.h (129279 => 129280)
--- trunk/Source/WebCore/dom/ChildListMutationScope.h 2012-09-22 00:16:45 UTC (rev 129279)
+++ trunk/Source/WebCore/dom/ChildListMutationScope.h 2012-09-22 00:30:48 UTC (rev 129280)
@@ -39,65 +39,70 @@
#include <wtf/HashMap.h>
#include <wtf/Noncopyable.h>
#include <wtf/OwnPtr.h>
+#include <wtf/RefCounted.h>
namespace WebCore {
+class MutationObserverInterestGroup;
+
+// ChildListMutationAccumulator is not meant to be used directly; ChildListMutationScope is the public interface.
+//
+// ChildListMutationAccumulator expects that all removals from a parent take place in order
+// and precede any additions. If this is violated (i.e. because of code changes elsewhere
+// in WebCore) it will likely result in both (a) ASSERTions failing, and (b) mutation records
+// being enqueued for delivery before the outer-most scope closes.
+class ChildListMutationAccumulator : public RefCounted<ChildListMutationAccumulator> {
+public:
+ static PassRefPtr<ChildListMutationAccumulator> getOrCreate(Node*);
+ ~ChildListMutationAccumulator();
+
+ void childAdded(PassRefPtr<Node>);
+ void willRemoveChild(PassRefPtr<Node>);
+
+ bool hasObservers() const { return m_observers; }
+
+private:
+ ChildListMutationAccumulator(PassRefPtr<Node>, PassOwnPtr<MutationObserverInterestGroup>);
+
+ void enqueueMutationRecord();
+ bool isEmpty();
+ bool isAddedNodeInOrder(Node*);
+ bool isRemovedNodeInOrder(Node*);
+
+ RefPtr<Node> m_target;
+
+ Vector<RefPtr<Node> > m_removedNodes;
+ Vector<RefPtr<Node> > m_addedNodes;
+ RefPtr<Node> m_previousSibling;
+ RefPtr<Node> m_nextSibling;
+ Node* m_lastAdded;
+
+ OwnPtr<MutationObserverInterestGroup> m_observers;
+};
+
class ChildListMutationScope {
WTF_MAKE_NONCOPYABLE(ChildListMutationScope);
public:
explicit ChildListMutationScope(Node* target)
- : m_target(target->document()->hasMutationObserversOfType(MutationObserver::ChildList) ? target : 0)
{
- if (m_target)
- MutationAccumulationRouter::instance()->incrementScopingLevel(m_target);
+ if (target->document()->hasMutationObserversOfType(MutationObserver::ChildList))
+ m_accumulator = ChildListMutationAccumulator::getOrCreate(target);
}
- ~ChildListMutationScope()
- {
- if (m_target)
- MutationAccumulationRouter::instance()->decrementScopingLevel(m_target);
- }
-
void childAdded(Node* child)
{
- if (m_target)
- MutationAccumulationRouter::instance()->childAdded(m_target, child);
+ if (m_accumulator && m_accumulator->hasObservers())
+ m_accumulator->childAdded(child);
}
void willRemoveChild(Node* child)
{
- if (m_target)
- MutationAccumulationRouter::instance()->willRemoveChild(m_target, child);
+ if (m_accumulator && m_accumulator->hasObservers())
+ m_accumulator->willRemoveChild(child);
}
private:
- class MutationAccumulator;
-
- class MutationAccumulationRouter {
- WTF_MAKE_NONCOPYABLE(MutationAccumulationRouter);
- public:
- ~MutationAccumulationRouter();
-
- static MutationAccumulationRouter* instance();
-
- void incrementScopingLevel(Node*);
- void decrementScopingLevel(Node*);
-
- void childAdded(Node* target, Node* child);
- void willRemoveChild(Node* target, Node* child);
-
- private:
- MutationAccumulationRouter();
- static void initialize();
-
- typedef HashMap<Node*, unsigned> ScopingLevelMap;
- ScopingLevelMap m_scopingLevels;
- HashMap<Node*, OwnPtr<MutationAccumulator> > m_accumulations;
-
- static MutationAccumulationRouter* s_instance;
- };
-
- Node* m_target;
+ RefPtr<ChildListMutationAccumulator> m_accumulator;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/html/HTMLElement.cpp (129279 => 129280)
--- trunk/Source/WebCore/html/HTMLElement.cpp 2012-09-22 00:16:45 UTC (rev 129279)
+++ trunk/Source/WebCore/html/HTMLElement.cpp 2012-09-22 00:30:48 UTC (rev 129280)
@@ -30,7 +30,6 @@
#include "CSSPropertyNames.h"
#include "CSSValueKeywords.h"
#include "CSSValuePool.h"
-#include "ChildListMutationScope.h"
#include "DOMSettableTokenList.h"
#include "DocumentFragment.h"
#include "Event.h"
