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%. Change-Id: Ic6deac03e63cf0583ced9de9caccd1b533c6b88d Reviewed-on: https://chromium-review.googlesource.com/803944 Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#521732}

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%. Change-Id: Ic6deac03e63cf0583ced9de9caccd1b533c6b88d Reviewed-on: https://chromium-review.googlesource.com/803944 Commit-Queue: Adithya Srinivasan <adithyas@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#521732}
c42a6705 · Adithya Srinivasan · Commit Bot · a5abcdc9 · c42a6705 · c42a6705
Commit c42a6705 authored Dec 05, 2017 by Adithya Srinivasan Committed by Commit Bot Dec 05, 2017
10 changed files
--- a/third_party/WebKit/LayoutTests/fast/forms/form-control-with-state-eager-tracing-crashTest.html
+++ b/third_party/WebKit/LayoutTests/fast/forms/form-control-with-state-eager-tracing-crashTest.html
+<!doctype html>
+<html>
+<head>
+  <title>HTMLFormControlElementWithState eager tracing crash test</title>
+  <script src="../../resources/testharness.js"></script>
+  <script src="../../resources/testharnessreport.js"></script>
+</head>
+<body>
+  <ul id="list"></ul>
+  <script>
+    function gc() {
+      if (typeof GCController !== "undefined")
+          GCController.collect();
+      else {
+        for (var i = 0; i < 10000; i++) {
+            // > force garbage collection
+            var s = new String("");
+        }
+      }
+    }
+    const numberOfFormControls = 1000;
+    var list = document.getElementById("list");
+    for(var i = 0; i < numberOfFormControls; i++) {
+      var li = document.createElement("li");
+      var textField = document.createElement("input");
+      textField.value = "Hello world!";
+      li.appendChild(textField);
+      list.appendChild(li);
+    }
+    test(() => {
+      gc();
+      assert_true(true);
+    }, "Should not crash during tracing, see https://crbug.com/790739#c5");
+  </script>
+</body>
+</html>
--- 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) {
-  auto result = form_controls_.insert(control);
+  DCHECK(!control->Next() && !control->Prev());
-  DCHECK(result.is_new_entry);
+  form_controls_.Append(control);
 }
 void DocumentState::RemoveControl(HTMLFormControlElementWithState* control) {
-  auto it = form_controls_.find(control);
+  form_controls_.Remove(control);
-  CHECK(it != form_controls_.end());
+  control->SetPrev(nullptr);
-  form_controls_.erase(it);
+  control->SetNext(nullptr);
 }
 static String FormStateSignature() {
@@ -433,8 +433,8 @@ Vector<String> DocumentState::ToStateVector() {
  FormKeyGenerator* key_generator = FormKeyGenerator::Create();
  std::unique_ptr<SavedFormStateMap> state_map =
      WTF::WrapUnique(new SavedFormStateMap);
-  for (const auto& form_control : form_controls_) {
+  for (HTMLFormControlElementWithState* control = form_controls_.Head();
-    HTMLFormControlElementWithState* control = form_control.Get();
+       control; control = control->Next()) {
    DCHECK(control->isConnected());
    if (!control->ShouldSaveAndRestoreFormControlState())
      continue;

--- a/third_party/WebKit/Source/core/html/forms/FormController.h
+++ b/third_party/WebKit/Source/core/html/forms/FormController.h
@@ -25,6 +25,7 @@
 #include <memory>
 #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"
@@ -91,9 +92,8 @@ class DocumentState final : public GarbageCollected<DocumentState> {
  Vector<String> ToStateVector();
 private:
-  using FormElementListHashSet =
+  using FormElementList = HeapDoublyLinkedList<HTMLFormControlElementWithState>;
-      HeapListHashSet<Member<HTMLFormControlElementWithState>, 64>;
+  FormElementList form_controls_;
-  FormElementListHashSet form_controls_;
 };
 class FormController final : public GarbageCollectedFinalized<FormController> {

--- a/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.cpp
+++ b/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.cpp
@@ -87,4 +87,10 @@ bool HTMLFormControlElementWithState::IsFormControlElementWithState() const {
  return true;
 }
+void HTMLFormControlElementWithState::Trace(Visitor* visitor) {
+  visitor->Trace(prev_);
+  visitor->Trace(next_);
+  HTMLFormControlElement::Trace(visitor);
+}
 }  // namespace blink
--- a/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.h
+++ b/third_party/WebKit/Source/core/html/forms/HTMLFormControlElementWithState.h
@@ -27,13 +27,21 @@
 #include "core/CoreExport.h"
 #include "core/html/forms/HTMLFormControlElement.h"
+#include "platform/wtf/DoublyLinkedList.h"
 namespace blink {
 class FormControlState;
+class HTMLFormControlElementWithState;
+// Cannot use eager tracing as HTMLFormControlElementWithState objects are part
+// of a HeapDoublyLinkedList and have pointers to the previous and next elements
+// in the list, which can cause very deep stacks in eager tracing.
+WILL_NOT_BE_EAGERLY_TRACED_CLASS(HTMLFormControlElementWithState);
 class CORE_EXPORT HTMLFormControlElementWithState
-    : public HTMLFormControlElement {
+    : public HTMLFormControlElement,
+      public DoublyLinkedListNode<HTMLFormControlElementWithState> {
 public:
  ~HTMLFormControlElementWithState() override;
@@ -46,6 +54,8 @@ class CORE_EXPORT HTMLFormControlElementWithState
  virtual void RestoreFormControlState(const FormControlState&) {}
  void NotifyFormStateChanged();
+  void Trace(Visitor*) override;
 protected:
  HTMLFormControlElementWithState(const QualifiedName& tag_name, Document&);
@@ -53,6 +63,15 @@ class CORE_EXPORT HTMLFormControlElementWithState
  InsertionNotificationRequest InsertedInto(ContainerNode*) override;
  void RemovedFrom(ContainerNode*) override;
  bool IsFormControlElementWithState() const final;
+ private:
+  // Pointers for DoublyLinkedListNode<HTMLFormControlElementWithState>. 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<HTMLFormControlElementWithState>;
+  Member<HTMLFormControlElementWithState> prev_;
+  Member<HTMLFormControlElementWithState> next_;
 };
 DEFINE_TYPE_CASTS(HTMLFormControlElementWithState,

--- a/third_party/WebKit/Source/platform/heap/HeapAllocator.h
+++ b/third_party/WebKit/Source/platform/heap/HeapAllocator.h
@@ -12,6 +12,7 @@
 #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"
@@ -479,6 +480,23 @@ class HeapDeque : public Deque<T, inlineCapacity, HeapAllocator> {
      : Deque<T, inlineCapacity, HeapAllocator>(other) {}
 };
+template <typename T>
+class HeapDoublyLinkedList : public DoublyLinkedList<T, Member<T>> {
+  IS_GARBAGE_COLLECTED_TYPE();
+  DISALLOW_NEW();
+ public:
+  HeapDoublyLinkedList() {
+    static_assert(WTF::IsGarbageCollectedType<T>::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 {

--- a/third_party/WebKit/Source/platform/heap/HeapTerminatedArray.h
+++ b/third_party/WebKit/Source/platform/heap/HeapTerminatedArray.h
@@ -59,12 +59,6 @@ class HeapTerminatedArray : public TerminatedArray<T> {
  friend class WTF::TerminatedArrayBuilder;
 };
-template <typename T>
-class TraceEagerlyTrait<HeapTerminatedArray<T>> {
- public:
-  static const bool value = TraceEagerlyTrait<T>::value;
-};
 }  // namespace blink
 #endif  // HeapTerminatedArray_h
--- a/third_party/WebKit/Source/platform/heap/HeapTest.cpp
+++ b/third_party/WebKit/Source/platform/heap/HeapTest.cpp
@@ -6709,4 +6709,62 @@ TEST(HeapTest, HeapHashMapCallsDestructor) {
  EXPECT_TRUE(string.Impl()->HasOneRef());
 }
+class DoublyLinkedListNodeImpl
+    : public GarbageCollectedFinalized<DoublyLinkedListNodeImpl>,
+      public DoublyLinkedListNode<DoublyLinkedListNodeImpl> {
+ 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<DoublyLinkedListNodeImpl>;
+  Member<DoublyLinkedListNodeImpl> prev_;
+  Member<DoublyLinkedListNodeImpl> next_;
+};
+int DoublyLinkedListNodeImpl::destructor_calls_ = 0;
+template <typename T>
+class HeapDoublyLinkedListContainer
+    : public GarbageCollected<HeapDoublyLinkedListContainer<T>> {
+ public:
+  static HeapDoublyLinkedListContainer<T>* Create() {
+    return new HeapDoublyLinkedListContainer<T>();
+  }
+  HeapDoublyLinkedListContainer<T>() {}
+  HeapDoublyLinkedList<T> list_;
+  void Trace(Visitor* visitor) { visitor->Trace(list_); }
+};
+TEST(HeapTest, HeapDoublyLinkedList) {
+  Persistent<HeapDoublyLinkedListContainer<DoublyLinkedListNodeImpl>>
+      container =
+          HeapDoublyLinkedListContainer<DoublyLinkedListNodeImpl>::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
--- a/third_party/WebKit/Source/platform/heap/TraceTraits.h
+++ b/third_party/WebKit/Source/platform/heap/TraceTraits.h
@@ -27,6 +27,10 @@ class CrossThreadPersistent;
 template <typename T>
 class CrossThreadWeakPersistent;
 template <typename T>
+class HeapDoublyLinkedList;
+template <typename T>
+class HeapTerminatedArray;
+template <typename T>
 class Member;
 template <typename T>
 class TraceEagerlyTrait;
@@ -434,6 +438,20 @@ class TraceEagerlyTrait<CrossThreadWeakPersistent<T>> {
  static const bool value = TraceEagerlyTrait<T>::value;
 };
+template <typename T>
+class TraceEagerlyTrait<HeapTerminatedArray<T>> {
+ public:
+  static const bool value = TraceEagerlyTrait<T>::value;
+};
+template <typename T>
+class TraceEagerlyTrait<HeapDoublyLinkedList<T>> {
+  STATIC_ONLY(TraceEagerlyTrait);
+ public:
+  static const bool value = TraceEagerlyTrait<T>::value;
+};
 template <typename ValueArg, size_t inlineCapacity>
 class HeapListHashSetAllocator;
 template <typename T, size_t inlineCapacity>

--- 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<T>::Next() const {
  return static_cast<const T*>(this)->next_;
 }
-template <typename T>
+template <typename T, typename PointerType = T*>
 class DoublyLinkedList {
  USING_FAST_MALLOC(DoublyLinkedList);
@@ -90,83 +90,90 @@ class DoublyLinkedList {
  void Append(T*);
  void Remove(T*);
- private:
+ protected:
-  T* head_;
+  PointerType head_;
-  T* tail_;
+  PointerType tail_;
  DISALLOW_COPY_AND_ASSIGN(DoublyLinkedList);
 };
-template <typename T>
+template <typename T, typename PointerType>
-inline DoublyLinkedList<T>::DoublyLinkedList() : head_(0), tail_(0) {}
+inline DoublyLinkedList<T, PointerType>::DoublyLinkedList()
+    : head_(nullptr), tail_(nullptr) {
+  static_assert(
+      !IsGarbageCollectedType<T>::value ||
+          !std::is_same<PointerType, T*>::value,
+      "Cannot use DoublyLinkedList<> with garbage collected types, use "
+      "HeapDoublyLinkedList<> instead.");
+}
-template <typename T>
+template <typename T, typename PointerType>
-inline bool DoublyLinkedList<T>::IsEmpty() const {
+inline bool DoublyLinkedList<T, PointerType>::IsEmpty() const {
  return !head_;
 }
-template <typename T>
+template <typename T, typename PointerType>
-inline size_t DoublyLinkedList<T>::size() const {
+inline size_t DoublyLinkedList<T, PointerType>::size() const {
  size_t size = 0;
  for (T* node = head_; node; node = node->Next())
    ++size;
  return size;
 }
-template <typename T>
+template <typename T, typename PointerType>
-inline void DoublyLinkedList<T>::Clear() {
+inline void DoublyLinkedList<T, PointerType>::Clear() {
-  head_ = 0;
+  head_ = nullptr;
-  tail_ = 0;
+  tail_ = nullptr;
 }
-template <typename T>
+template <typename T, typename PointerType>
-inline T* DoublyLinkedList<T>::Head() const {
+inline T* DoublyLinkedList<T, PointerType>::Head() const {
  return head_;
 }
-template <typename T>
+template <typename T, typename PointerType>
-inline T* DoublyLinkedList<T>::Tail() const {
+inline T* DoublyLinkedList<T, PointerType>::Tail() const {
  return tail_;
 }
-template <typename T>
+template <typename T, typename PointerType>
-inline void DoublyLinkedList<T>::Push(T* node) {
+inline void DoublyLinkedList<T, PointerType>::Push(T* node) {
  if (!head_) {
    DCHECK(!tail_);
    head_ = node;
    tail_ = node;
-    node->SetPrev(0);
+    node->SetPrev(nullptr);
-    node->SetNext(0);
+    node->SetNext(nullptr);
    return;
  }
  DCHECK(tail_);
  head_->SetPrev(node);
  node->SetNext(head_);
-  node->SetPrev(0);
+  node->SetPrev(nullptr);
  head_ = node;
 }
-template <typename T>
+template <typename T, typename PointerType>
-inline void DoublyLinkedList<T>::Append(T* node) {
+inline void DoublyLinkedList<T, PointerType>::Append(T* node) {
  if (!tail_) {
    DCHECK(!head_);
    head_ = node;
    tail_ = node;
-    node->SetPrev(0);
+    node->SetPrev(nullptr);
-    node->SetNext(0);
+    node->SetNext(nullptr);
    return;
  }
  DCHECK(head_);
  tail_->SetNext(node);
  node->SetPrev(tail_);
-  node->SetNext(0);
+  node->SetNext(nullptr);
  tail_ = node;
 }
-template <typename T>
+template <typename T, typename PointerType>
-inline void DoublyLinkedList<T>::Remove(T* node) {
+inline void DoublyLinkedList<T, PointerType>::Remove(T* node) {
  if (node->Prev()) {
    DCHECK_NE(node, head_);
    node->Prev()->SetNext(node->Next());
@@ -184,8 +191,8 @@ inline void DoublyLinkedList<T>::Remove(T* node) {
  }
 }
-template <typename T>
+template <typename T, typename PointerType>
-inline T* DoublyLinkedList<T>::RemoveHead() {
+inline T* DoublyLinkedList<T, PointerType>::RemoveHead() {
  T* node = Head();
  if (node)
    Remove(node);