Remove ThreadHeap::m_threads

Remove ThreadHeap::m_threads

Now that ThreadHeap and ThreadState are 1:1, ThreadHeap doesn't need to maintain
a set of associated ThreadStates.

BUG=671856
Review-Url: https://codereview.chromium.org/2697703005
Cr-Commit-Position: refs/heads/master@{#451457}
Committed: https://chromium.googlesource.com/chromium/src/+/aebdbddd1caea79bd6d3875fb093be2197264585

[haraken, 2017-02-15 08:14:17]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2017-02-15 08:14:18]

PTAL

[sof, 2017-02-15 08:26:32]

https://codereview.chromium.org/2697703005/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/2697703005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.cpp:191: delete this;
Would it work for |ThreadState::m_heap| to be a |std::unique_ptr<ThreadHeap>| ?
And it is a bit odd for detach() now to call methods on the ThreadState while
(also) shutting down -- runTerminationGC() could be a private method on
ThreadState afaict.

[haraken, 2017-02-15 09:06:16]

On 2017/02/15 08:26:32, sof wrote:
>
https://codereview.chromium.org/2697703005/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/heap/Heap.cpp (right):
> 
>
https://codereview.chromium.org/2697703005/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/heap/Heap.cpp:191: delete this;
> Would it work for |ThreadState::m_heap| to be a |std::unique_ptr<ThreadHeap>|
?
> And it is a bit odd for detach() now to call methods on the ThreadState while
> (also) shutting down -- runTerminationGC() could be a private method on
> ThreadState afaict.

Done.

[sof, 2017-02-15 09:18:21]

lgtm

https://codereview.chromium.org/2697703005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.cpp (left):

https://codereview.chromium.org/2697703005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.cpp:200: if
(thread->isMainThread())
we could move this to the dtor, up to you.

[haraken, 2017-02-15 09:23:55]

>
https://codereview.chromium.org/2697703005/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/heap/Heap.cpp (left):
> 
>
https://codereview.chromium.org/2697703005/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/heap/Heap.cpp:200: if
> (thread->isMainThread())
> we could move this to the dtor, up to you.

Done.

[haraken, 2017-02-15 09:24:08]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-02-15 09:24:08]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2697703005/#ps80001
(title: "temp")

[commit-bot: I haz the power, 2017-02-15 09:24:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 11:03:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 11:03:08]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2017-02-15 11:05:15]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2017-02-15 11:05:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 11:13:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 11:13:25]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[haraken, 2017-02-15 14:48:45]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2017-02-15 14:49:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 14:56:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 14:56:56]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[haraken, 2017-02-16 01:15:56]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-02-16 01:15:57]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2697703005/#ps90001
(title: "temp")

[commit-bot: I haz the power, 2017-02-16 01:17:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 03:21:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 03:21:19]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[haraken, 2017-02-17 10:39:09]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2017-02-17 10:39:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 10:42:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 10:42:12]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2017-02-17 11:29:07]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-02-17 11:29:08]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2697703005/#ps110001
(title: "temp")

[commit-bot: I haz the power, 2017-02-17 11:29:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 13:07:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 13:07:03]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2017-02-17 13:36:37]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2017-02-17 13:36:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 15:02:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 15:02:37]

Failed to apply patch for third_party/WebKit/Source/platform/heap/Heap.cpp:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/Source/platform/heap/Heap.cpp:145
  error: third_party/WebKit/Source/platform/heap/Heap.cpp: patch does not apply

Patch:       third_party/WebKit/Source/platform/heap/Heap.cpp
Index: third_party/WebKit/Source/platform/heap/Heap.cpp
diff --git a/third_party/WebKit/Source/platform/heap/Heap.cpp
b/third_party/WebKit/Source/platform/heap/Heap.cpp
index
b5665c53f93c821ea55d8207811f81d9ae528f80..c8f6b0fddeec864f05dcba71e43160b451f367fb
100644
--- a/third_party/WebKit/Source/platform/heap/Heap.cpp
+++ b/third_party/WebKit/Source/platform/heap/Heap.cpp
@@ -145,8 +145,9 @@ void ThreadHeapStats::decreaseAllocatedSpace(size_t delta) {
   ProcessHeap::decreaseTotalAllocatedSpace(delta);
 }
 
-ThreadHeap::ThreadHeap()
-    : m_regionTree(WTF::makeUnique<RegionTree>()),
+ThreadHeap::ThreadHeap(ThreadState* threadState)
+    : m_threadState(threadState),
+      m_regionTree(WTF::makeUnique<RegionTree>()),
       m_heapDoesNotContainCache(WTF::wrapUnique(new HeapDoesNotContainCache)),
       m_safePointBarrier(WTF::makeUnique<SafePointBarrier>()),
       m_freePagePool(WTF::wrapUnique(new PagePool)),
@@ -161,41 +162,9 @@ ThreadHeap::ThreadHeap()
 ThreadHeap::~ThreadHeap() {
 }
 
-void ThreadHeap::attach(ThreadState* thread) {
-  MutexLocker locker(m_threadAttachMutex);
-  m_threads.insert(thread);
-}
-
-void ThreadHeap::detach(ThreadState* thread) {
-  ASSERT(ThreadState::current() == thread);
-  bool isLastThread = false;
-  {
-    // Grab the threadAttachMutex to ensure only one thread can shutdown at
-    // a time and that no other thread can do a global GC. It also allows
-    // safe iteration of the m_threads set which happens as part of
-    // thread local GC asserts. We enter a safepoint while waiting for the
-    // lock to avoid a dead-lock where another thread has already requested
-    // GC.
-    MutexLocker locker(m_threadAttachMutex);
-    thread->runTerminationGC();
-    ASSERT(m_threads.contains(thread));
-    m_threads.erase(thread);
-    isLastThread = m_threads.isEmpty();
-  }
-  if (thread->isMainThread())
-    DCHECK_EQ(heapStats().allocatedSpace(), 0u);
-  if (isLastThread)
-    delete this;
-}
-
 #if DCHECK_IS_ON()
 BasePage* ThreadHeap::findPageFromAddress(Address address) {
-  MutexLocker locker(m_threadAttachMutex);
-  for (ThreadState* state : m_threads) {
-    if (BasePage* page = state->findPageFromAddress(address))
-      return page;
-  }
-  return nullptr;
+  return m_threadState->findPageFromAddress(address);
 }
 #endif
 
@@ -347,19 +316,16 @@ void ThreadHeap::decommitCallbackStacks() {
 
 void ThreadHeap::preGC() {
   ASSERT(!ThreadState::current()->isInGC());
-  for (ThreadState* state : m_threads)
-    state->preGC();
+  m_threadState->preGC();
 }
 
 void ThreadHeap::postGC(BlinkGC::GCType gcType) {
   ASSERT(ThreadState::current()->isInGC());
-  for (ThreadState* state : m_threads)
-    state->postGC(gcType);
+  m_threadState->postGC(gcType);
 }
 
 void ThreadHeap::preSweep(BlinkGC::GCType gcType) {
-  for (ThreadState* state : m_threads)
-    state->preSweep(gcType);
+  m_threadState->preSweep(gcType);
 }
 
 void ThreadHeap::processMarkingStack(Visitor* visitor) {
@@ -511,15 +477,12 @@ void ThreadHeap::reportMemoryUsageForTracing() {
 }
 
 size_t ThreadHeap::objectPayloadSizeForTesting() {
-  // MEMO: is threadAttachMutex locked?
   size_t objectPayloadSize = 0;
-  for (ThreadState* state : m_threads) {
-    state->setGCState(ThreadState::GCRunning);
-    state->makeConsistentForGC();
-    objectPayloadSize += state->objectPayloadSizeForTesting();
-    state->setGCState(ThreadState::Sweeping);
-    state->setGCState(ThreadState::NoGCScheduled);
-  }
+  m_threadState->setGCState(ThreadState::GCRunning);
+  m_threadState->makeConsistentForGC();
+  objectPayloadSize += m_threadState->objectPayloadSizeForTesting();
+  m_threadState->setGCState(ThreadState::Sweeping);
+  m_threadState->setGCState(ThreadState::NoGCScheduled);
   return objectPayloadSize;
 }
 
@@ -528,15 +491,13 @@ void ThreadHeap::visitPersistentRoots(Visitor* visitor) {
   TRACE_EVENT0("blink_gc", "ThreadHeap::visitPersistentRoots");
   ProcessHeap::crossThreadPersistentRegion().tracePersistentNodes(visitor);
 
-  for (ThreadState* state : m_threads)
-    state->visitPersistents(visitor);
+  m_threadState->visitPersistents(visitor);
 }
 
 void ThreadHeap::visitStackRoots(Visitor* visitor) {
   ASSERT(ThreadState::current()->isInGC());
   TRACE_EVENT0("blink_gc", "ThreadHeap::visitStackRoots");
-  for (ThreadState* state : m_threads)
-    state->visitStack(visitor);
+  m_threadState->visitStack(visitor);
 }
 
 void ThreadHeap::enterSafePoint(ThreadState* threadState) {
@@ -564,8 +525,7 @@ void ThreadHeap::resetHeapCounters() {
   ProcessHeap::decreaseTotalMarkedObjectSize(m_stats.markedObjectSize());
 
   m_stats.reset();
-  for (ThreadState* state : m_threads)
-    state->resetHeapCounters();
+  m_threadState->resetHeapCounters();
 }
 
 ThreadHeap* ThreadHeap::s_mainThreadHeap = nullptr;

[sof, 2017-02-18 06:10:54]

(needs a rebase?)

[haraken, 2017-02-18 10:57:11]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-02-18 10:57:12]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2697703005/#ps130001
(title: "temp")

[commit-bot: I haz the power, 2017-02-18 10:57:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 12:29:14]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1487415431625400,
"parent_rev": "4e7c52b4577b682be39ad617ceb5d98a32279939", "commit_rev":
"aebdbddd1caea79bd6d3875fb093be2197264585"}

[commit-bot: I haz the power, 2017-02-18 12:29:53]

Description was changed from

==========
Remove ThreadHeap::m_threads

Now that ThreadHeap and ThreadState are 1:1, ThreadHeap doesn't need to maintain
a set of associated ThreadStates.

BUG=671856
==========

to

==========
Remove ThreadHeap::m_threads

Now that ThreadHeap and ThreadState are 1:1, ThreadHeap doesn't need to maintain
a set of associated ThreadStates.

BUG=671856

Review-Url: https://codereview.chromium.org/2697703005
Cr-Commit-Position: refs/heads/master@{#451457}
Committed:
https://chromium.googlesource.com/chromium/src/+/aebdbddd1caea79bd6d3875fb093...
==========

[commit-bot: I haz the power, 2017-02-18 12:29:54]

Committed patchset #8 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/aebdbddd1caea79bd6d3875fb093...