From 5fdc0fab22ce7efd32532ee989b223fa12f8171e Mon Sep 17 00:00:00 2001 From: Kenneth Russell Date: Fri, 1 Dec 2017 03:37:01 +0000 Subject: [PATCH] Revert "Use DoublyLinkedList instead of ListHashSet in DocumentState" This reverts commit 252e8a49c9383eceebe0938a1e876f0b4ab5aa8e. Reason for revert: Caused http://crbug.com/790739 Original change's description: > Use DoublyLinkedList instead of ListHashSet in DocumentState > > The only operations carried out on form_controls_ are insertions, removals > and iterating through the entire list. Insertion and removal can be done > faster with a DoublyLinkedList. > > Since the nodes for the DoublyLinkedList are Oilpan objects, this CL > introduces HeapDoublyLinkedList that uses Member for the head and tail > pointers, and traces the pointers. > > This improves the performance of HTMLInputElement::InsertedInto and > HTMLInputElement::RemovedFrom by ~15%. > > Bug: > Change-Id: I5b4cd20737e0276bece2430edfb7ec9609690f04 > Reviewed-on: https://chromium-review.googlesource.com/758877 > Reviewed-by: Kentaro Hara > Reviewed-by: Keishi Hattori > Reviewed-by: Jeremy Roman > Commit-Queue: Adithya Srinivasan > Cr-Commit-Position: refs/heads/master@{#517876} Bug: 790739 TBR=jbroman@chromium.org,haraken@chromium.org,keishi@chromium.org,adithyas@chromium.org,lfg@chromium.org Change-Id: I48ddedd7b356efa6b1f6f69c58e2022e9a0872f1 Reviewed-on: https://chromium-review.googlesource.com/802774 Commit-Queue: Kenneth Russell Reviewed-by: Kenneth Russell Reviewed-by: Kentaro Hara Cr-Commit-Position: refs/heads/master@{#520840} --- .../Source/core/html/forms/FormController.cpp | 14 ++-- .../Source/core/html/forms/FormController.h | 6 +- .../forms/HTMLFormControlElementWithState.cpp | 6 -- .../forms/HTMLFormControlElementWithState.h | 15 +--- .../Source/platform/heap/HeapAllocator.h | 18 ----- .../platform/heap/HeapTerminatedArray.h | 6 ++ .../WebKit/Source/platform/heap/HeapTest.cpp | 58 --------------- .../WebKit/Source/platform/heap/TraceTraits.h | 18 ----- .../Source/platform/wtf/DoublyLinkedList.h | 71 +++++++++---------- 9 files changed, 49 insertions(+), 163 deletions(-) diff --git a/third_party/WebKit/Source/core/html/forms/FormController.cpp b/third_party/WebKit/Source/core/html/forms/FormController.cpp index 535b3cee4e18c..3157e8d798fa9 100644 --- a/third_party/WebKit/Source/core/html/forms/FormController.cpp +++ b/third_party/WebKit/Source/core/html/forms/FormController.cpp @@ -410,14 +410,14 @@ void DocumentState::Trace(blink::Visitor* visitor) { } void DocumentState::AddControl(HTMLFormControlElementWithState* control) { - DCHECK(!control->Next() && !control->Prev()); - form_controls_.Append(control); + auto result = form_controls_.insert(control); + DCHECK(result.is_new_entry); } void DocumentState::RemoveControl(HTMLFormControlElementWithState* control) { - form_controls_.Remove(control); - control->SetPrev(nullptr); - control->SetNext(nullptr); + auto it = form_controls_.find(control); + CHECK(it != form_controls_.end()); + form_controls_.erase(it); } static String FormStateSignature() { @@ -433,8 +433,8 @@ Vector DocumentState::ToStateVector() { FormKeyGenerator* key_generator = FormKeyGenerator::Create(); std::unique_ptr state_map = WTF::WrapUnique(new SavedFormStateMap); - for (HTMLFormControlElementWithState* control = form_controls_.Head(); - control; control = control->Next()) { + for (const auto& form_control : form_controls_) { + HTMLFormControlElementWithState* control = form_control.Get(); DCHECK(control->isConnected()); if (!control->ShouldSaveAndRestoreFormControlState()) continue; diff --git a/third_party/WebKit/Source/core/html/forms/FormController.h b/third_party/WebKit/Source/core/html/forms/FormController.h index c21565d55c44b..7ec8019b08500 100644 --- a/third_party/WebKit/Source/core/html/forms/FormController.h +++ b/third_party/WebKit/Source/core/html/forms/FormController.h @@ -25,7 +25,6 @@ #include #include "platform/heap/Handle.h" -#include "platform/heap/HeapAllocator.h" #include "platform/wtf/Allocator.h" #include "platform/wtf/Forward.h" #include "platform/wtf/ListHashSet.h" @@ -92,8 +91,9 @@ class DocumentState final : public GarbageCollected { Vector ToStateVector(); private: - using FormElementList = HeapDoublyLinkedList; - FormElementList form_controls_; + using FormElementListHashSet = + HeapListHashSet, 64>; + FormElementListHashSet form_controls_; }; class FormController final : public GarbageCollectedFinalized { diff --git a/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.cpp b/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.cpp index 740ae5a9f592f..b8dd24fa9e078 100644 --- a/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.cpp +++ b/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.cpp @@ -87,10 +87,4 @@ bool HTMLFormControlElementWithState::IsFormControlElementWithState() const { return true; } -void HTMLFormControlElementWithState::Trace(Visitor* visitor) { - visitor->Trace(prev_); - visitor->Trace(next_); - HTMLFormControlElement::Trace(visitor); -} - } // namespace blink diff --git a/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.h b/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.h index 0a4db72c33714..31320d66b3c6a 100644 --- a/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.h +++ b/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.h @@ -27,15 +27,13 @@ #include "core/CoreExport.h" #include "core/html/forms/HTMLFormControlElement.h" -#include "platform/wtf/DoublyLinkedList.h" namespace blink { class FormControlState; class CORE_EXPORT HTMLFormControlElementWithState - : public HTMLFormControlElement, - public DoublyLinkedListNode { + : public HTMLFormControlElement { public: ~HTMLFormControlElementWithState() override; @@ -48,8 +46,6 @@ class CORE_EXPORT HTMLFormControlElementWithState virtual void RestoreFormControlState(const FormControlState&) {} void NotifyFormStateChanged(); - void Trace(Visitor*) override; - protected: HTMLFormControlElementWithState(const QualifiedName& tag_name, Document&); @@ -57,15 +53,6 @@ class CORE_EXPORT HTMLFormControlElementWithState InsertionNotificationRequest InsertedInto(ContainerNode*) override; void RemovedFrom(ContainerNode*) override; bool IsFormControlElementWithState() const final; - - private: - // Pointers for DoublyLinkedListNode. This - // is used for adding an instance to a list of form controls stored in - // DocumentState. Each instance is only added to its containing document's - // DocumentState list. - friend class WTF::DoublyLinkedListNode; - Member prev_; - Member next_; }; DEFINE_TYPE_CASTS(HTMLFormControlElementWithState, diff --git a/third_party/WebKit/Source/platform/heap/HeapAllocator.h b/third_party/WebKit/Source/platform/heap/HeapAllocator.h index 60af8f95369f1..490e6b665a954 100644 --- a/third_party/WebKit/Source/platform/heap/HeapAllocator.h +++ b/third_party/WebKit/Source/platform/heap/HeapAllocator.h @@ -12,7 +12,6 @@ #include "platform/wtf/Allocator.h" #include "platform/wtf/Assertions.h" #include "platform/wtf/Deque.h" -#include "platform/wtf/DoublyLinkedList.h" #include "platform/wtf/HashCountedSet.h" #include "platform/wtf/HashMap.h" #include "platform/wtf/HashSet.h" @@ -480,23 +479,6 @@ class HeapDeque : public Deque { : Deque(other) {} }; -template -class HeapDoublyLinkedList : public DoublyLinkedList> { - IS_GARBAGE_COLLECTED_TYPE(); - DISALLOW_NEW(); - - public: - HeapDoublyLinkedList() { - static_assert(WTF::IsGarbageCollectedType::value, - "This should only be used for garbage collected types."); - } - - void Trace(Visitor* visitor) { - visitor->Trace(this->head_); - visitor->Trace(this->tail_); - } -}; - } // namespace blink namespace WTF { diff --git a/third_party/WebKit/Source/platform/heap/HeapTerminatedArray.h b/third_party/WebKit/Source/platform/heap/HeapTerminatedArray.h index c422d318af319..5cd2ec8ea3398 100644 --- a/third_party/WebKit/Source/platform/heap/HeapTerminatedArray.h +++ b/third_party/WebKit/Source/platform/heap/HeapTerminatedArray.h @@ -59,6 +59,12 @@ class HeapTerminatedArray : public TerminatedArray { friend class WTF::TerminatedArrayBuilder; }; +template +class TraceEagerlyTrait> { + public: + static const bool value = TraceEagerlyTrait::value; +}; + } // namespace blink #endif // HeapTerminatedArray_h diff --git a/third_party/WebKit/Source/platform/heap/HeapTest.cpp b/third_party/WebKit/Source/platform/heap/HeapTest.cpp index d772ab86bebab..99bf5dd8e17be 100644 --- a/third_party/WebKit/Source/platform/heap/HeapTest.cpp +++ b/third_party/WebKit/Source/platform/heap/HeapTest.cpp @@ -6709,62 +6709,4 @@ TEST(HeapTest, HeapHashMapCallsDestructor) { EXPECT_TRUE(string.Impl()->HasOneRef()); } -class DoublyLinkedListNodeImpl - : public GarbageCollectedFinalized, - public DoublyLinkedListNode { - public: - DoublyLinkedListNodeImpl() {} - static DoublyLinkedListNodeImpl* Create() { - return new DoublyLinkedListNodeImpl(); - } - - static int destructor_calls_; - ~DoublyLinkedListNodeImpl() { ++destructor_calls_; } - - void Trace(Visitor* visitor) { - visitor->Trace(prev_); - visitor->Trace(next_); - } - - private: - friend class WTF::DoublyLinkedListNode; - Member prev_; - Member next_; -}; - -int DoublyLinkedListNodeImpl::destructor_calls_ = 0; - -template -class HeapDoublyLinkedListContainer - : public GarbageCollected> { - public: - static HeapDoublyLinkedListContainer* Create() { - return new HeapDoublyLinkedListContainer(); - } - HeapDoublyLinkedListContainer() {} - HeapDoublyLinkedList list_; - void Trace(Visitor* visitor) { visitor->Trace(list_); } -}; - -TEST(HeapTest, HeapDoublyLinkedList) { - Persistent> - container = - HeapDoublyLinkedListContainer::Create(); - DoublyLinkedListNodeImpl::destructor_calls_ = 0; - - container->list_.Append(DoublyLinkedListNodeImpl::Create()); - container->list_.Append(DoublyLinkedListNodeImpl::Create()); - - PreciselyCollectGarbage(); - EXPECT_EQ(DoublyLinkedListNodeImpl::destructor_calls_, 0); - - container->list_.RemoveHead(); - PreciselyCollectGarbage(); - EXPECT_EQ(DoublyLinkedListNodeImpl::destructor_calls_, 1); - - container->list_.RemoveHead(); - PreciselyCollectGarbage(); - EXPECT_EQ(DoublyLinkedListNodeImpl::destructor_calls_, 2); -} - } // namespace blink diff --git a/third_party/WebKit/Source/platform/heap/TraceTraits.h b/third_party/WebKit/Source/platform/heap/TraceTraits.h index f2464fe95e571..899966d095849 100644 --- a/third_party/WebKit/Source/platform/heap/TraceTraits.h +++ b/third_party/WebKit/Source/platform/heap/TraceTraits.h @@ -27,10 +27,6 @@ class CrossThreadPersistent; template class CrossThreadWeakPersistent; template -class HeapDoublyLinkedList; -template -class HeapTerminatedArray; -template class Member; template class TraceEagerlyTrait; @@ -438,20 +434,6 @@ class TraceEagerlyTrait> { static const bool value = TraceEagerlyTrait::value; }; -template -class TraceEagerlyTrait> { - public: - static const bool value = TraceEagerlyTrait::value; -}; - -template -class TraceEagerlyTrait> { - STATIC_ONLY(TraceEagerlyTrait); - - public: - static const bool value = TraceEagerlyTrait::value; -}; - template class HeapListHashSetAllocator; template diff --git a/third_party/WebKit/Source/platform/wtf/DoublyLinkedList.h b/third_party/WebKit/Source/platform/wtf/DoublyLinkedList.h index 4c2e9a02e0f52..5280f77df19be 100644 --- a/third_party/WebKit/Source/platform/wtf/DoublyLinkedList.h +++ b/third_party/WebKit/Source/platform/wtf/DoublyLinkedList.h @@ -70,7 +70,7 @@ inline T* DoublyLinkedListNode::Next() const { return static_cast(this)->next_; } -template +template class DoublyLinkedList { USING_FAST_MALLOC(DoublyLinkedList); @@ -90,90 +90,83 @@ class DoublyLinkedList { void Append(T*); void Remove(T*); - protected: - PointerType head_; - PointerType tail_; + private: + T* head_; + T* tail_; DISALLOW_COPY_AND_ASSIGN(DoublyLinkedList); }; -template -inline DoublyLinkedList::DoublyLinkedList() - : head_(nullptr), tail_(nullptr) { - static_assert( - !IsGarbageCollectedType::value || - !std::is_same::value, - "Cannot use DoublyLinkedList<> with garbage collected types, use " - "HeapDoublyLinkedList<> instead."); -} +template +inline DoublyLinkedList::DoublyLinkedList() : head_(0), tail_(0) {} -template -inline bool DoublyLinkedList::IsEmpty() const { +template +inline bool DoublyLinkedList::IsEmpty() const { return !head_; } -template -inline size_t DoublyLinkedList::size() const { +template +inline size_t DoublyLinkedList::size() const { size_t size = 0; for (T* node = head_; node; node = node->Next()) ++size; return size; } -template -inline void DoublyLinkedList::Clear() { - head_ = nullptr; - tail_ = nullptr; +template +inline void DoublyLinkedList::Clear() { + head_ = 0; + tail_ = 0; } -template -inline T* DoublyLinkedList::Head() const { +template +inline T* DoublyLinkedList::Head() const { return head_; } -template -inline T* DoublyLinkedList::Tail() const { +template +inline T* DoublyLinkedList::Tail() const { return tail_; } -template -inline void DoublyLinkedList::Push(T* node) { +template +inline void DoublyLinkedList::Push(T* node) { if (!head_) { DCHECK(!tail_); head_ = node; tail_ = node; - node->SetPrev(nullptr); - node->SetNext(nullptr); + node->SetPrev(0); + node->SetNext(0); return; } DCHECK(tail_); head_->SetPrev(node); node->SetNext(head_); - node->SetPrev(nullptr); + node->SetPrev(0); head_ = node; } -template -inline void DoublyLinkedList::Append(T* node) { +template +inline void DoublyLinkedList::Append(T* node) { if (!tail_) { DCHECK(!head_); head_ = node; tail_ = node; - node->SetPrev(nullptr); - node->SetNext(nullptr); + node->SetPrev(0); + node->SetNext(0); return; } DCHECK(head_); tail_->SetNext(node); node->SetPrev(tail_); - node->SetNext(nullptr); + node->SetNext(0); tail_ = node; } -template -inline void DoublyLinkedList::Remove(T* node) { +template +inline void DoublyLinkedList::Remove(T* node) { if (node->Prev()) { DCHECK_NE(node, head_); node->Prev()->SetNext(node->Next()); @@ -191,8 +184,8 @@ inline void DoublyLinkedList::Remove(T* node) { } } -template -inline T* DoublyLinkedList::RemoveHead() { +template +inline T* DoublyLinkedList::RemoveHead() { T* node = Head(); if (node) Remove(node);