Refactor the Heap into ThreadHeap to prepare for per thread heaps

Refactor the Heap into ThreadHeap to prepare for per thread heaps

For per thread heap, we need to convert the static members in Heap to non-static so we can have them for each per thread heap.

New classes:
ThreadHeap is for data tied to the set of ThreadStates that need to be stopped for GC.
ProcessHeap is for global data about the Oilpan heap.
ThreadHeapStats is a bunch of stats for each ThreadHeap.

BUG=591606
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq

[haraken, 2015-11-30 02:54:41]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-11-30 02:54:42]

The approach looks good.

I'd recommend you insert a RELEASE_ASSERT to MarkingVisitorImpl.h to verify that
a GC never traces into heaps of unexpected threads.

- A global GC => a heap of an attached thread (Allowed)
- A global GC => a heap of an isolated thread (Forbidden)
- An isolated GC => a heap of another thread (Forbidden; but this should not
happen in the first place because a thread-local GC already has a mechanism to
not trace into another thread's heap)

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

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.cpp:225: CallbackStack::Item* item
= ThreadState::current()->markingStack()->pop();

At the moment this is okay, but ThreadState::current() should be avoided as much
as possible (because it needs to look up thread-local storage). We'll need to
pass ThreadState* around or associate it with the Visitor.

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.cpp:467: GCScope gcScope(state,
BlinkGC::NoHeapPointersOnStack, BlinkGC::ThreadTerminationGC);

As commented in ThreadState.cpp, it is wrong to set
BlinkGC::NoHeapPointersOnStac. Heap::collectGarbageForIsolatedThread can be
called for conservative GCs.

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.cpp:601: state =
ThreadState::terminating();

I'm wondering why this is needed. We should make sure that
ThreadState::current() returns a valid pointer while the thread is operating on
Oilpan's heap (e.g., calling Heap::removePageMemoryRegion).

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.cpp:604:
state->setRegionTree(regionTree);

Help me understand: What are you doing by setting the RegionTree on the
ThreadState? Isn't it already set on the ThreadState?

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

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1463:
ThreadState::current()->increaseMarkedObjectSize(size());

ThreadState::current() needs to look up thread-local storage. Use threadState()
as much as possible.

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

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/SafePoint.cpp:49: continue;

Instead of making this change, we should not add the isolated thread to
ThreadState::attachedThreads() in the first place.

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

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:168:
**s_threadSpecificTerminating = this;

I wonder why we need this.

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:348: if
(!state->isolated())

As commented somewhere, I recommend you not add isolated threads to
attachedThreads().

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:648:
Heap::collectGarbageForIsolatedThread(this);

At the moment it is okay to ignore lazy sweeping (i.e., GCWithoutSweep), but it
is not okay to ignore HeapPointersOnStack. The current implementation of
Heap::collectGarbageForIsolatedThread does not scan on-stack pointers (i.e., it
runs a precise GC). This means that if the isolated thread triggers a
conservative GC, it can mis-collect objects reachable from on-stack pointers.

[keishi, 2016-01-06 05:35:33]

I'm still trying to fix some crashes related to PersistentNodes but PTAL.

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

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.cpp:225: CallbackStack::Item* item
= ThreadState::current()->markingStack()->pop();
On 2015/11/30 02:54:41, haraken wrote:
> 
> At the moment this is okay, but ThreadState::current() should be avoided as
much
> as possible (because it needs to look up thread-local storage). We'll need to
> pass ThreadState* around or associate it with the Visitor.

Done.

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Heap.cpp:467: GCScope gcScope(state,
BlinkGC::NoHeapPointersOnStack, BlinkGC::ThreadTerminationGC);
On 2015/11/30 02:54:42, haraken wrote:
> 
> As commented in ThreadState.cpp, it is wrong to set
> BlinkGC::NoHeapPointersOnStac. Heap::collectGarbageForIsolatedThread can be
> called for conservative GCs.

Done.

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

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/SafePoint.cpp:49: continue;
On 2015/11/30 02:54:42, haraken wrote:
> 
> Instead of making this change, we should not add the isolated thread to
> ThreadState::attachedThreads() in the first place.

Done.

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

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:168:
**s_threadSpecificTerminating = this;
On 2015/11/30 02:54:42, haraken wrote:
> 
> I wonder why we need this.

Removed

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:348: if
(!state->isolated())
On 2015/11/30 02:54:42, haraken wrote:
> 
> As commented somewhere, I recommend you not add isolated threads to
> attachedThreads().

Done.

https://codereview.chromium.org/1477023003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:648:
Heap::collectGarbageForIsolatedThread(this);
On 2015/11/30 02:54:42, haraken wrote:
> 
> At the moment it is okay to ignore lazy sweeping (i.e., GCWithoutSweep), but
it
> is not okay to ignore HeapPointersOnStack. The current implementation of
> Heap::collectGarbageForIsolatedThread does not scan on-stack pointers (i.e.,
it
> runs a precise GC). This means that if the isolated thread triggers a
> conservative GC, it can mis-collect objects reachable from on-stack pointers.

Done.

[haraken, 2016-01-07 05:32:18]

I'll take a closer look from now, but would you make sure that this CL doesn't
regress performance of blink_perf.bindings.*?

[haraken, 2016-01-07 08:06:22]

Thanks for working on this!

The main points of my comments are:

- Remove XThreadPersistent.

- Remove Heap::collectGarbageForIsolatedThread.

- Move stats variables from Heap to ThreadState in a separate CL before landing
this CL. This CL is a bit too large to review...

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WrapperTypeInfo.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WrapperTypeInfo.h:154:
ThreadState::current()->increaseCollectedWrapperCount(1);

Avoid calling ThreadState::current() twice.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WrapperTypeInfo.h:165:
ThreadState::current()->increaseCollectedWrapperCount(1);

Ditto.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:219:
ASSERT(ThreadState::forObject(transform) == ThreadState::forObject(this));

Is this useful?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/FrameRequestCallbackCollection.cpp
(right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/FrameRequestCallbackCollection.cpp:23:
ASSERT(ThreadState::forObject(callback) == ThreadState::current());

Is this useful?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/SharedWorkerThread.cpp (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/SharedWorkerThread.cpp:61: m_thread =
WebThreadSupportingGC::create("SharedWorker Thread",
ThreadState::PerThreadHeapDisabled);

It might be better to make ThreadState::PerThreadHeapDisabled the default
parameter.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerInspectorProxy.cpp (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerInspectorProxy.cpp:42:
ASSERT(ThreadState::forObject(m_executionContext.get()) ==
ThreadState::mainThreadState());

Is this useful?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.h:113:
OwnPtrWillBeXThreadPersistent<WorkerClients> m_workerClients;

Just to confirm: WorkerMessagingProxy (and the Persistent handle) is created and
destructed in the main thread but it is pointing to an object in a worker
thread, right?

I just want to understand why you're changing a (thread-safe) Persistent to a
XThreadPersistent.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/Database.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/Database.h:171:
Member<HeapDeque<Member<SQLTransactionBackend>>> m_transactionQueue;

What is this change for?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/DatabaseTask.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/DatabaseTask.h:47: // DatabaseTask
can be created from main thread and database thread.

from main thread => by the main thread

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp:48:
m_openDatabaseSet.reserveCapacityForSize(1000);

What is this change for?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/SQLTransactionCoordinator.h
(right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/SQLTransactionCoordinator.h:45:
class SQLTransactionCoordinator : public
GarbageCollectedFinalized<SQLTransactionCoordinator> {

It looks like there is no benefit in making SQLTransactionCoordinator
GarbageCollected. Can we move it back to PartitionAlloc?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp:23:
WebThreadSupportingGC::WebThreadSupportingGC(const char* name, WebThread*
thread, ThreadState::PerThreadHeapState perThreadHeapState)

Can we move the parameter from the constructor to initialize()? Then
m_perThreadHeapEnabled wouldn't be needed.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Handle.h:548: class XThreadPersistent :
public PersistentBase<T, NonWeakPersistentConfiguration,
XThreadPersistentConfiguration> {

I'm considering a way to land the per-thread heap without introducing
XThreadPersistent to the code base. Can we do something like this?

- Remove XThreadPersistent and XThreadPersistentRegion.

- Just use CrossThreadPersistent to store any cross-thread pointer. These
CrossThreadPersistent handles are stored in
ThreadState::crossThreadPersistentRegion(), which is per-process not per-thread.

- When we run a marking phase of a per-thread GC, we iterate
ThreadState::crossThreadPersistentRegion() but ignore all CrossThreadPersistent
handles that hold pointers to objects that don't reside in the thread.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:246: CallbackStack::Item* slot
= ThreadState::current()->postMarkingCallbackStack()->allocateEntry();

Can we avoid calling ThreadState::current()?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:469: GCScope gcScope(state,
BlinkGC::HeapPointersOnStack, BlinkGC::ThreadTerminationGC);

It is not a good idea to "reuse" the concept of ThreadTerminationGC for the two
reasons:

- ThreadTerminationGC is going to be removed once the per-thread heap is
completely implemented.

- ThreadTerminationGC is a GC that works with the assumption that all pages
belonging to the thread are marked as terminating. Given that we don't do the
marking when running a per-thread heap, it is wrong to reuse the
ThreadTerminationGC. We should just use a GlobalMarkingVisitor and make it
workable for per-thread GCs (or we may need to introduce another Visitor).

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:469: GCScope gcScope(state,
BlinkGC::HeapPointersOnStack, BlinkGC::ThreadTerminationGC);

It is not a good idea to unconditionally assume BlinkGC::HeapPointersOnStack.
You should pass in the StackState to collectGarbageForIsolatedThread.

However, I suggest removing collectGarbageForIsolatedThread. See my comment
below.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:496:
state->postGC(BlinkGC::GCWithSweep);

We don't want to force sweeping.

I don't think we should introduce collectGarbageForIsolatedThread. The logic we
want for a per-thread GC is the one implemented in collectGarbage, not the one
implemented in collectGarbageForTerminatingThread.

The only difference between the collectGarbage for a main thread's GC and the
collectGarbage for a per-thread GC is that the latter doesn't stop other
threads. The difference could be easily encapsulated in GCScope. If
collectGarbage is called by an isolated thread, it means that we need to use a
GCScope for a per-thread GC. Otherwise, we can use a GCScope for a main thread's
GC.

So my suggestion is to reuse collectGarbage, encapsulating the difference in
GCScope.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:557: void
Heap::collectAllGarbage()

This is another reason I don't want to introduce
collectGarbageForIsolatedThread. If an isolated thread calls collectAllGarbage()
to clean up the thread's heap, it is intending to trigger multiple per-thread
GCs but in the current code it will call multiple main thread's GCs.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.h:139: static void
pushTraceCallback(void* containerObject, TraceCallback, ThreadState*);

This change is okay in this CL, but ideally these pushXXX methods should be
moved to ThreadState*. Let's add a TODO.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/MarkingVisitorImpl.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/MarkingVisitorImpl.h:43:
RELEASE_ASSERT(ThreadState::forObject(objectPointer) ==
toDerived()->threadState());

forObject => fromObject

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/MarkingVisitorImpl.h:43:
RELEASE_ASSERT(ThreadState::forObject(objectPointer) ==
toDerived()->threadState());

Hmm. This RELEASE_ASSERT would be a bit problematic for the following two
reasons:

- markHeader must be super fast to minimize GC's pause time. I want to avoid
introducing an if branch and the ASSERT.

- The ASSERT checks that a per-thread heap's GC doesn't trace into a
non-per-thread heap, but it doesn't check that a non-per-thread heap's GC
doesn't trace into a per-thread heap.

That said, I have no alternative idea at the moment. Let me think about it more.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.h:191: class
XThreadPersistentRegion final {

XThreadPersistentRegion and CrossThreadPersistentRegion is the same thing,
right? Would typedef be enough?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:644:
Heap::collectGarbageForIsolatedThread(this);

It wouldn't be a good idea to introduce Heap::collectGarbageForIsolatedThread. I
think we can just reuse Heap::collectGarbage(). See my comment in Heap.cpp.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1592: // FIXME: Separate
report per thread

In practice, we're just interested in the memory consumption of the main thread.
Can we just add:

  if (!isMainThread())
    return;

?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1594: //
dataLogF("allocatedSpace=%ldMB, allocatedObjectSize=%ldMB,
markedObjectSize=%ldMB, partitionAllocSize=%ldMB, wrapperCount=%ld,
collectedWrapperCount=%ld\n", Heap::allocatedSpace() / 1024 / 1024,
Heap::allocatedObjectSize() / 1024 / 1024, Heap::markedObjectSize() / 1024 /
1024, WTF::Partitions::totalSizeOfCommittedPages() / 1024 / 1024,
Heap::wrapperCount(), Heap::collectedWrapperCount());

Heap:: => ThreadState::

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1604:
TRACE_COUNTER1("blink_gc", "Heap::allocatedObjectSizeKB",
std::min(allocatedObjectSize() / 1024, static_cast<size_t>(INT_MAX)));

Heap:: => ThreadState::

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:133: class GCHeapStats {

Can we make this change (i.e., move a bunch of stats variables) in a separate CL
before landing this CL?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:186: class RegionTree {

Let's move out the RegionTree to a separate file. Let's make this change in a
separate CL.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:316: CallbackStack*
markingStack() const { return m_markingStack.get(); }

Can we move these methods before landing this CL?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:526: static
CrossThreadPersistentRegion& crossThreadPersistentRegion();

Not related to your CL, this should be moved to Heap since
crossThreadPersistentRegion() is per-process, not per-thread.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:684:
ThreadState(PerThreadHeapState);

Add explicit.

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:840: GCHeapStats*
m_heapStats;

How about using an OwnPtr?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:846: RegionTree*
m_regionTree;

How about using an OwnPtr?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:852: static size_t
s_totalMarkedObjectSize;

Given that these variables keep track of the stats for the entire heap, it
doesn't make sense to put it on ThreadState. Can we move them back to Heap?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/HashSet.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/HashSet.h:60: ValueType* table() const { return
m_impl.table(); }

What is this change for?

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/HashTable.h (right):

https://codereview.chromium.org/1477023003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/HashTable.h:438: ValueType* table() const { return
m_table; }

Ditto.

[keishi, 2016-01-27 05:59:32]

I've refactored the CL and introduced GCGroup.

Seems to be mostly working based on layout tests but I'm fixing a memory leak
that happens when per thread heap is enabled.
haraken@ could PTAL meanwhile and see if there are any obvious problems.

Thanks.

GCGroup: Group of ThreadStates that need to be parked for GC.
GCData: Data used for GC.
GCHeapStats: Stat counter for entire heap in GCGroup.

[haraken, 2016-01-28 14:14:34]

(Sorry, I'm running out of my time today again due to the stabilization work on
ToT -- hopefully I'll take a look at this one tomorrow.)

[haraken, 2016-01-28 15:52:50]

(70% reviewed. I'll take a look at ThreadState.{h,cpp} tomorrow.)

The main point is:

- In long term, I think we should unify Visitor, GCScope and GCData into one
class. Conceptually Visitor and GCScope should have the same lifetime (and
should be the same thing).

- At least, in order not to introduce more classes, I'd suggest unifying GCScope
and GCData into one class.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:21:
allocatorDump->addScalar("size", "bytes",
ThreadState::current()->gcGroup()->heapStats().allocatedSpace());

I think allocatedSpace should be a member of Heap, not a member of GCGroup.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:116: class GCScope final :
public GCData {

Is there any reason you want to distinguish GCScope from GCData? i.e., would it
make more sense to unify the two classes?

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:132: explicit
ResumeThreadScope(BlinkGC::GCType gcType, GCGroup* gcGroup)

Remove explicit.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:165:
ThreadState::current()->gcGroup()->shutdownIfNecessary();

I guess this is doing something wrong. Heap::shutdown() is a function to shut
down the entire system. It must be called after all GCGroups are shut down.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:178: //
ASSERT(Heap::allocatedSpace() == 0);

We want to keep this ASSERT somehow. This ASSERT is important to make sure that
underlying memory doesn't leak.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:188: BasePage*
Heap::findPageFromAddress(Address address)

This method should be moved to GCGroup. I think what we want to do here is to
find the given address from the given GCGroup, not from all threads in the heap.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:190: for (GCGroup* gcGroup :
GCGroup::all()) {

In general, it is wrong to replace:

  for (ThreadState* state : ThreadState::attachedThreads())

with:

  for (GCGroup* gcGroup : GCGroup::all())

It must be replaced with:

  for (ThreadState* state : gcGroup->threads())

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:203: if
(visitor->gcData()->threadState()->gcGroup()->heapDoesNotContainCache()->lookup(address))

It's nasty that we have to write visitor->gcData()->threadState()->gcGroup()->
every time but is okay for now. (Ideally I want to unify Visitor and GCData into
one class.)

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:220:
visitor->gcData()->threadState()->gcGroup()->heapDoesNotContainCache()->addEntry(address);

Cache the gcGroup into a local variable and avoid the code duplication.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:235: bool
Heap::popAndInvokeTraceCallback(Visitor* visitor)

It's inconsistent that you sometimes pass Visitor but sometimes GCData. Shall we
unify them into Visitor? (In long term, I want to unify Visitor and GCData into
one class.)

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:273: void
Heap::pushThreadLocalWeakCallback(void* closure, void* object, WeakCallback
callback)

We should pass in Visitor.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:315: {

Is this helper function still useful?

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:320: void
Heap::postGC(BlinkGC::GCType gcType)

Ditto.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:581: for (GCGroup* gcGroup :
GCGroup::all()) {

This should not iterate all GCGroups. Just return:

  gcGroup->objectPayloadSizeForTesting();

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.h:166: static void
pushTraceCallback(void* containerObject, TraceCallback, GCData*);

Nit: These push methods should be members of GCData. You don't need to address
it in this CL though.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:631: Address
NormalPageHeap::lazySweepPages(size_t allocationSize, size_t gcInfoIndex,
GCGroup*)

The GCGroup parameter looks unnecessary.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:788: Address
LargeObjectHeap::allocateLargeObjectPage(size_t allocationSize, size_t
gcInfoIndex, GCGroup* gcGroup)

Remove the GCGroup parameter.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:810: Address
LargeObjectHeap::doAllocateLargeObjectPage(size_t allocationSize, size_t
gcInfoIndex, GCGroup* gcGroup)

Remove the GCGroup parameter.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:820: PageMemory* pageMemory
= PageMemory::allocate(largeObjectSize, gcGroup);

And use threadState()->gcGroup().

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:876: Address
LargeObjectHeap::lazySweepPages(size_t allocationSize, size_t gcInfoIndex,
GCGroup* gcGroup)

Remove the GCGroup parameter.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1160:
ThreadState::current()->gcGroup()->heapStats().increaseMarkedObjectSize(markedObjectSize);

heap()->threadState()

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1185:
ThreadState::current()->gcGroup()->heapStats().increaseMarkedObjectSize(markedObjectSize);

heap()->threadState()

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1452:
ThreadState::current()->gcGroup()->heapStats().increaseMarkedObjectSize(size());

heap()->threadState()

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1460:
ThreadState::current()->gcGroup()->heapStats().increaseMarkedObjectSize(size());

heap()->threadState()

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapTest.cpp:284:
CountingVisitor(GCData* gcData)

Add explicit.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/MarkingVisitor.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/MarkingVisitor.h:18:
MarkingVisitor(GCData* gcData)

Add explicit.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PagePool.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PagePool.cpp:83:
ASSERT(ThreadState::mainThreadState()->gcGroup()->isParked());

isParked => isAtSafePoint

Note that "safe point" != "parked". A thread may be running a sync file
operation at a safe point. The thread is not parked in that case.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:72: return;

Shouldn't this be 'continue'?

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:104: {

Add ASSERT(!node->isUnused()).

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:108: // TODO: Why do
we need to return true?

I wonder why as well :) This should return false.

Maybe is this related to the above mistake between return and continue?

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:111: return
visitor->gcData()->threadState()->gcGroup()->threads().contains(ThreadState::forObject(rawObject));

forObject => fromAddress (for consistency with HeapPage's methods)

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.h:146: void
tracePersistentNodes(Visitor*, bool (*shouldTrace)(Visitor*, PersistentNode*) =
nullptr);

Use typedef for the function signature.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/SafePoint.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/SafePoint.cpp:44: if
(state->isAtSafePoint()) // MEMO: original code: if (state == current)

Why do you need to make this change?

This change may be correct but I don't want to touch the SafePoint's algorithm
as much as possible.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/SafePoint.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/SafePoint.h:93:
SafePointBarrier(MultiThreadGCGroup*);

Add explicit.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Visitor.h:125: VisitorHelper(GCData*
gcData) : m_gcData(gcData) {}

Add explicit.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebKit.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebKit.cpp:174:
ThreadState::prepareForMainThread();

Nit: I'd prefer keeping "attach"/"attachMainThread" for consistency with
"detach".

[haraken, 2016-02-12 11:28:53]

Finished reviewing this CL :)

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:101:
m_partitionAllocSizeAtLastGC = WTF::Partitions::totalSizeOfCommittedPages();

It would be problematic to simply use PartitionAlloc's size for the threshold to
trigger per-thread GCs. If we do that, a per-thread GC of a worker thread may be
triggered because the main thread allocates a lot of objects on PartitionAlloc.

(Let's worry about this issue after resolving all other issues in this CL.)

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:108: void
GCHeapStats::increaseAllocatedObjectSize(size_t delta)

These changes will be made by peria-san's CL:

https://codereview.chromium.org/1670463002/

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:161: ,
m_freePagePool(adoptPtr(new FreePagePool))

Maybe we should share the FreePagePool among all threads to save memory
consumption.

(Let's revisit this issue after resolving all other issues in this CL.)

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:196: bool
MultiThreadGCGroup::isParked() const

Add #if ENABLE(ASSERT).

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:196: bool
MultiThreadGCGroup::isParked() const

isParked => isAtSafePoint

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:254: size_t
MultiThreadGCGroup::size() const

size => threadCount ?

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:301: shutdown();

As commented in Heap::shutdown(), I think we need to clean up the shutdown logic
a bit more. For example, Heap::shutdown() must be called only once. However, in
the current code, Heap::shutdown() will be called per MultiThreadGroup. Which is
wrong.

Let's visit this issue after resolving other issues.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:493: void
ThreadState::cleanupCallback()

cleanupCallback => cleanupThreadHeap ?

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:495: if (isMainThread())
{

This should be:

  if (isMainThread() || m_perThreadHeapEnabled)

If this is a per-thread heap, you don't need to run a termination GC.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:523: }

If m_perThreadHeapEnabled is true, the termination GC won't be executed. This
means that the above code doesn't run. This means that the per-thread heap
enables us to get rid of the restriction that all persistent handles must be
gone before the thread shuts down. This is great :)

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1713: ThreadState*
ThreadState::forObject(const void* object)

forObject => fromObject (for consistency with fromAddress etc)

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:219: class PLATFORM_EXPORT
MultiThreadGCGroup : public GCGroup {

Is it really worth distinguishing MultiThreadGCGroup from GCGroup? I guess we
can just unify the two classes into the GCGroup.

[keishi, 2016-02-29 00:46:14]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-29 00:46:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477023003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477023003/380001

[commit-bot: I haz the power, 2016-02-29 00:58:46]

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

[commit-bot: I haz the power, 2016-02-29 00:58:47]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[keishi, 2016-02-29 04:36:15]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-29 04:36:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477023003/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477023003/400001

[keishi, 2016-02-29 06:02:34]

I realized that GCGroup is Heap, so I renamed it to that. Then we have
"BaseHeap* ThreadState::heap(heapIndex)" and "Heap* ThreadState::heap()" which
is confusing so I renamed BaseHeap to BaseArena.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:21:
allocatorDump->addScalar("size", "bytes",
ThreadState::current()->gcGroup()->heapStats().allocatedSpace());
On 2016/01/28 15:52:48, haraken wrote:
> 
> I think allocatedSpace should be a member of Heap, not a member of GCGroup.

Used Heap::totalAllocatedSpace

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:165:
ThreadState::current()->gcGroup()->shutdownIfNecessary();
On 2016/01/28 15:52:49, haraken wrote:
> 
> I guess this is doing something wrong. Heap::shutdown() is a function to shut
> down the entire system. It must be called after all GCGroups are shut down.
> 

I had some mistakes in the new CL but
Heap::shutdown calls GCGroup::shutdownIfNecessary which calls GCGroup::shutdown
when all threads are detached. MultipleThreadGCGroup::shutdown will call
Heap::doShutdown because MultipleThreadGCGroup is always the main thread.
SingleThreadGCGroup which will be added later will not call Heap::doShutdown.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:178: //
ASSERT(Heap::allocatedSpace() == 0);
On 2016/01/28 15:52:49, haraken wrote:
> 
> We want to keep this ASSERT somehow. This ASSERT is important to make sure
that
> underlying memory doesn't leak.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:188: BasePage*
Heap::findPageFromAddress(Address address)
On 2016/01/28 15:52:49, haraken wrote:
> 
> This method should be moved to GCGroup. I think what we want to do here is to
> find the given address from the given GCGroup, not from all threads in the
heap.
> 

Removed

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:203: if
(visitor->gcData()->threadState()->gcGroup()->heapDoesNotContainCache()->lookup(address))
On 2016/01/28 15:52:49, haraken wrote:
> 
> It's nasty that we have to write visitor->gcData()->threadState()->gcGroup()->
> every time but is okay for now. (Ideally I want to unify Visitor and GCData
into
> one class.)
> 

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:273: void
Heap::pushThreadLocalWeakCallback(void* closure, void* object, WeakCallback
callback)
On 2016/01/28 15:52:49, haraken wrote:
> 
> We should pass in Visitor.

Why? I don't think we can't use Visitor for anything

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:315: {
On 2016/01/28 15:52:49, haraken wrote:
> 
> Is this helper function still useful?

Remvoed

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:320: void
Heap::postGC(BlinkGC::GCType gcType)
On 2016/01/28 15:52:49, haraken wrote:
> 
> Ditto.

Removed

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:581: for (GCGroup* gcGroup :
GCGroup::all()) {
On 2016/01/28 15:52:49, haraken wrote:
> 
> This should not iterate all GCGroups. Just return:
> 
>   gcGroup->objectPayloadSizeForTesting();

Removed and used GCGroup::objectPayloadSizeForTesting directly.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.h:166: static void
pushTraceCallback(void* containerObject, TraceCallback, GCData*);
On 2016/01/28 15:52:49, haraken wrote:
> 
> Nit: These push methods should be members of GCData. You don't need to address
> it in this CL though.

I moved the callback stack related methods to VisitorScope where the
CallbackStacks exist.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:631: Address
NormalPageHeap::lazySweepPages(size_t allocationSize, size_t gcInfoIndex,
GCGroup*)
On 2016/01/28 15:52:49, haraken wrote:
> 
> The GCGroup parameter looks unnecessary.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:788: Address
LargeObjectHeap::allocateLargeObjectPage(size_t allocationSize, size_t
gcInfoIndex, GCGroup* gcGroup)
On 2016/01/28 15:52:50, haraken wrote:
> 
> Remove the GCGroup parameter.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:810: Address
LargeObjectHeap::doAllocateLargeObjectPage(size_t allocationSize, size_t
gcInfoIndex, GCGroup* gcGroup)
On 2016/01/28 15:52:49, haraken wrote:
> 
> Remove the GCGroup parameter.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:820: PageMemory* pageMemory
= PageMemory::allocate(largeObjectSize, gcGroup);
On 2016/01/28 15:52:49, haraken wrote:
> 
> And use threadState()->gcGroup().

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:876: Address
LargeObjectHeap::lazySweepPages(size_t allocationSize, size_t gcInfoIndex,
GCGroup* gcGroup)
On 2016/01/28 15:52:49, haraken wrote:
> 
> Remove the GCGroup parameter.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1160:
ThreadState::current()->gcGroup()->heapStats().increaseMarkedObjectSize(markedObjectSize);
On 2016/01/28 15:52:50, haraken wrote:
> 
> heap()->threadState()

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1185:
ThreadState::current()->gcGroup()->heapStats().increaseMarkedObjectSize(markedObjectSize);
On 2016/01/28 15:52:49, haraken wrote:
> 
> heap()->threadState()

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1452:
ThreadState::current()->gcGroup()->heapStats().increaseMarkedObjectSize(size());
On 2016/01/28 15:52:49, haraken wrote:
> 
> heap()->threadState()

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1460:
ThreadState::current()->gcGroup()->heapStats().increaseMarkedObjectSize(size());
On 2016/01/28 15:52:49, haraken wrote:
> 
> heap()->threadState()

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapTest.cpp:284:
CountingVisitor(GCData* gcData)
On 2016/01/28 15:52:50, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/MarkingVisitor.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/MarkingVisitor.h:18:
MarkingVisitor(GCData* gcData)
On 2016/01/28 15:52:50, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PagePool.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PagePool.cpp:83:
ASSERT(ThreadState::mainThreadState()->gcGroup()->isParked());
On 2016/01/28 15:52:50, haraken wrote:
> 
> isParked => isAtSafePoint
> 
> Note that "safe point" != "parked". A thread may be running a sync file
> operation at a safe point. The thread is not parked in that case.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:72: return;
On 2016/01/28 15:52:50, haraken wrote:
> 
> Shouldn't this be 'continue'?

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:104: {
On 2016/01/28 15:52:50, haraken wrote:
> 
> Add ASSERT(!node->isUnused()).

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:108: // TODO: Why do
we need to return true?
On 2016/01/28 15:52:50, haraken wrote:
> 
> I wonder why as well :) This should return false.
> 
> Maybe is this related to the above mistake between return and continue?

Thanks, looks like it was.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:111: return
visitor->gcData()->threadState()->gcGroup()->threads().contains(ThreadState::forObject(rawObject));
On 2016/01/28 15:52:50, haraken wrote:
> 
> forObject => fromAddress (for consistency with HeapPage's methods)

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.h:146: void
tracePersistentNodes(Visitor*, bool (*shouldTrace)(Visitor*, PersistentNode*) =
nullptr);
On 2016/01/28 15:52:50, haraken wrote:
> 
> Use typedef for the function signature.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/SafePoint.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/SafePoint.cpp:44: if
(state->isAtSafePoint()) // MEMO: original code: if (state == current)
On 2016/01/28 15:52:50, haraken wrote:
> 
> Why do you need to make this change?
> 
> This change may be correct but I don't want to touch the SafePoint's algorithm
> as much as possible.
> 

Done. Just thought it made more sense.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/SafePoint.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/SafePoint.h:93:
SafePointBarrier(MultiThreadGCGroup*);
On 2016/01/28 15:52:50, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:196: bool
MultiThreadGCGroup::isParked() const
On 2016/02/12 11:28:52, haraken wrote:
> 
> isParked => isAtSafePoint

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:196: bool
MultiThreadGCGroup::isParked() const
On 2016/02/12 11:28:52, haraken wrote:
> 
> Add #if ENABLE(ASSERT).

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:254: size_t
MultiThreadGCGroup::size() const
On 2016/02/12 11:28:52, haraken wrote:
> 
> size => threadCount ?

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:493: void
ThreadState::cleanupCallback()
On 2016/02/12 11:28:52, haraken wrote:
> 
> cleanupCallback => cleanupThreadHeap ?

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:495: if (isMainThread())
{
On 2016/02/12 11:28:52, haraken wrote:
> 
> This should be:
> 
>   if (isMainThread() || m_perThreadHeapEnabled)
> 
> If this is a per-thread heap, you don't need to run a termination GC.

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1713: ThreadState*
ThreadState::forObject(const void* object)
On 2016/02/12 11:28:52, haraken wrote:
> 
> forObject => fromObject (for consistency with fromAddress etc)

Done.

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:219: class PLATFORM_EXPORT
MultiThreadGCGroup : public GCGroup {
On 2016/02/12 11:28:53, haraken wrote:
> 
> Is it really worth distinguishing MultiThreadGCGroup from GCGroup? I guess we
> can just unify the two classes into the GCGroup.
> 

For per thread heap enabled threads I'm thinking of using SingleThreadGCGroup

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebKit.cpp (right):

https://codereview.chromium.org/1477023003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebKit.cpp:174:
ThreadState::prepareForMainThread();
On 2016/01/28 15:52:50, haraken wrote:
> 
> Nit: I'd prefer keeping "attach"/"attachMainThread" for consistency with
> "detach".

Done.

[commit-bot: I haz the power, 2016-02-29 06:40:17]

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

[commit-bot: I haz the power, 2016-02-29 06:40:19]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-02-29 11:17:45]

Overall looks good.

Let's split the CL into pieces and start landing!

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:441:
ASSERT(ThreadState::current()->isInGC());

ThreadState::current() => state() throughout this file.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:557: void
Heap::doShutdownIfNecessary()

BTW, I'm wondering if we still need the complicated shutdown sequence. This code
was added in days where it was not guaranteed that the main thread joins all
other threads (in other words, the main thread might not be the last thread that
gets detached).

Today it's guaranteed that the main thread is the last thread that gets
detached. With that assumption, maybe can we simplify the logic more? (i.e., we
can just make the main thread call ProcessHeap::shutdown().)

I'll take a look at this.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:560: if (!s_shutdownCalled ||
!all().isEmpty())

Checking all().isEmpty() here isn't quite correct. See the following comment.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:567: GCInfoTable::shutdown();

This must be executed once per process.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:568: ThreadState::shutdown();

This must be executed once per ThreadState.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:569:
ASSERT(Heap::totalAllocatedSpace() == 0);

This must be executed once per process.

Also maybe you want to add:

  ASSERT(heapStats()->allocatedSpace() == 0);

This must be executed once per ThreadState.

So, as I commented in Heap.h, it would be helpful to separate a class for
per-process stuff from a class for per-thread stuff. Then you can implement a
shutdown logic for each class.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:585:
CrossThreadPersistentRegion& Heap::crossThreadPersistentRegion()

This is also per-process stuff.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.h:396: static size_t
s_totalMarkedObjectSize;

Shall we factor out these static variables to another STATIC_ONLY class?

I'd suggest something like:

- Rename Heap to ThreadHeap.
- Rename GCHeapStats to ThreadHeapStats.
- Introduce ProcessHeap, which holds the static-only information.

Hmm, by the way, what's a conceptual difference between Heap and ThreadState?
Maybe would it make more sense to simply move the contents of the Heap to the
ThreadState? (Are you concerned that it will bloat the ThreadState class too
much?)

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PageMemory.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PageMemory.cpp:160: PageMemoryRegion*
pageMemoryRegion = PageMemoryRegion::allocateLargePage(allocationSize, heap);

A better fix would be to call heap->addPageMemoryRegion(pageMemoryRegion) here.

Then you can remove PageMemoryRegion::m_heap. (I don't think PageMemoryRegion
needs to know the heap.)

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PagePool.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PagePool.cpp:79:
ASSERT(ThreadState::current()->isInGC());

Add ASSERT(ThreadState::current() == ThreadState::mainThreadState()).

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PagePool.cpp:103:
ThreadState::current()->heap().freePagePool()->addFreePage(index, memory);

ThreadState::current() => ThreadState::mainThreadState()).

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (left):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:91:
ASSERT(persistentCount == m_persistentCount);

Can we keep this assert by incrementing persistentCount before the line 72?

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:71: if (shouldTrace
&& !shouldTrace(visitor, node))

Nit: Or you can implement PersistentRegion::shouldTracePersistentNode, which
unconditionally returns true. Then you won't need to check if shouldTrace is
nullptr or not.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/SafePoint.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/SafePoint.cpp:40: const ThreadStateSet&
threads = m_heap->threads();

It would be better to move lockThreadAttachMutex(), threads() etc from Heap to
ThreadState. Then SafePointBarrier doesn't need to know the heap. You can just
use |current| here.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:214: void
ThreadState::cleanup()

cleanup => runThreadTerminationGC

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:289:
this->~ThreadState();

Is this different from 'delete this'?

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Visitor.h:398: class PLATFORM_EXPORT
VisitorScope final {

Can we unify the VisitorScope and the Visitor into one class (before landing
this CL)? Then you don't need to look up the VisitorScope from the Visitor.
Conceptually, the VisitorScope and the Visitor should be the same thing.

class Visitor {
  STACK_ALLOCATED();
  ...;
  ThreadState* m_state;
};

[keishi, 2016-03-02 06:01:04]

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Heap.cpp:441:
ASSERT(ThreadState::current()->isInGC());
On 2016/02/29 11:17:45, haraken wrote:
> 
> ThreadState::current() => state() throughout this file.

? Heap doesn't have state()

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PageMemory.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PageMemory.cpp:160: PageMemoryRegion*
pageMemoryRegion = PageMemoryRegion::allocateLargePage(allocationSize, heap);
On 2016/02/29 11:17:45, haraken wrote:
> 
> A better fix would be to call heap->addPageMemoryRegion(pageMemoryRegion)
here.
> 
> Then you can remove PageMemoryRegion::m_heap. (I don't think PageMemoryRegion
> needs to know the heap.)

We need to call removePageMemoryRegion from ~PageMemoryRegion so I don't think I
can remove PageMemoryRegion::m_heap.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PagePool.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PagePool.cpp:79:
ASSERT(ThreadState::current()->isInGC());
On 2016/02/29 11:17:45, haraken wrote:
> 
> Add ASSERT(ThreadState::current() == ThreadState::mainThreadState()).

Turned out decommitOrphanedPages() was called from non main thread. Changed
mainThreadState() to currentThread() below.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PagePool.cpp:103:
ThreadState::current()->heap().freePagePool()->addFreePage(index, memory);
On 2016/02/29 11:17:45, haraken wrote:
> 
> ThreadState::current() => ThreadState::mainThreadState()).

Done.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (left):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:91:
ASSERT(persistentCount == m_persistentCount);
On 2016/02/29 11:17:45, haraken wrote:
> 
> Can we keep this assert by incrementing persistentCount before the line 72?

Done.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:71: if (shouldTrace
&& !shouldTrace(visitor, node))
On 2016/02/29 11:17:45, haraken wrote:
> 
> Nit: Or you can implement PersistentRegion::shouldTracePersistentNode, which
> unconditionally returns true. Then you won't need to check if shouldTrace is
> nullptr or not.
> 

Done.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/SafePoint.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/SafePoint.cpp:40: const ThreadStateSet&
threads = m_heap->threads();
On 2016/02/29 11:17:45, haraken wrote:
> 
> It would be better to move lockThreadAttachMutex(), threads() etc from Heap to
> ThreadState. Then SafePointBarrier doesn't need to know the heap. You can just
> use |current| here.
> 
> 

Done.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:214: void
ThreadState::cleanup()
On 2016/02/29 11:17:45, haraken wrote:
> 
> cleanup => runThreadTerminationGC

Done.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:289:
this->~ThreadState();
On 2016/02/29 11:17:45, haraken wrote:
> 
> Is this different from 'delete this'?

mainThreadState is stored in s_mainThreadStateStorage so it shouldn't be
deleted.

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Visitor.h:398: class PLATFORM_EXPORT
VisitorScope final {
On 2016/02/29 11:17:45, haraken wrote:
> 
> Can we unify the VisitorScope and the Visitor into one class (before landing
> this CL)? Then you don't need to look up the VisitorScope from the Visitor.
> Conceptually, the VisitorScope and the Visitor should be the same thing.
> 
> class Visitor {
>   STACK_ALLOCATED();
>   ...;
>   ThreadState* m_state;
> };

Created https://codereview.chromium.org/1753623002/

[haraken, 2016-03-02 07:21:07]

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PageMemory.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PageMemory.cpp:160: PageMemoryRegion*
pageMemoryRegion = PageMemoryRegion::allocateLargePage(allocationSize, heap);
On 2016/03/02 06:01:03, keishi wrote:
> On 2016/02/29 11:17:45, haraken wrote:
> > 
> > A better fix would be to call heap->addPageMemoryRegion(pageMemoryRegion)
> here.
> > 
> > Then you can remove PageMemoryRegion::m_heap. (I don't think
PageMemoryRegion
> > needs to know the heap.)
> 
> We need to call removePageMemoryRegion from ~PageMemoryRegion so I don't think
I
> can remove PageMemoryRegion::m_heap.

~PageMemoryRegion is called only by 'delete current' in RegionTree::remove. Can
you move m_heap->removePageMemoryRegion(this) into RegionTree::remove? The
RegionTree::remove can take the heap parameter.

(Nit: Ideally, those RegionTree methods should be members of the Heap class.
Heap::addToRegionTree(), Heap::removeFromRegionTree() etc, which accesses
Heap::m_regionTree.)

[keishi, 2016-03-02 10:19:31]

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PageMemory.cpp (right):

https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PageMemory.cpp:160: PageMemoryRegion*
pageMemoryRegion = PageMemoryRegion::allocateLargePage(allocationSize, heap);
On 2016/03/02 07:21:07, haraken wrote:
> On 2016/03/02 06:01:03, keishi wrote:
> > On 2016/02/29 11:17:45, haraken wrote:
> > > 
> > > A better fix would be to call heap->addPageMemoryRegion(pageMemoryRegion)
> > here.
> > > 
> > > Then you can remove PageMemoryRegion::m_heap. (I don't think
> PageMemoryRegion
> > > needs to know the heap.)
> > 
> > We need to call removePageMemoryRegion from ~PageMemoryRegion so I don't
think
> I
> > can remove PageMemoryRegion::m_heap.
> 
> ~PageMemoryRegion is called only by 'delete current' in RegionTree::remove.
Can
> you move m_heap->removePageMemoryRegion(this) into RegionTree::remove? The
> RegionTree::remove can take the heap parameter.
> 
> (Nit: Ideally, those RegionTree methods should be members of the Heap class.
> Heap::addToRegionTree(), Heap::removeFromRegionTree() etc, which accesses
> Heap::m_regionTree.)

Done.

[haraken, 2016-03-02 13:42:09]

Overall looks good to me. Let's split the CL into pieces and landing.

I'm still wondering if ThreadHeap and ThreadState could be merged into one class
(the current distinction is pretty unclear) but I think we can revisit the issue
after landing other parts.

[sof, 2016-03-02 13:45:13]

On 2016/03/02 13:42:09, haraken wrote:
> Overall looks good to me. Let's split the CL into pieces and landing.
> 
> I'm still wondering if ThreadHeap and ThreadState could be merged into one
class
> (the current distinction is pretty unclear) but I think we can revisit the
issue
> after landing other parts.

Can we hold off on landing until it has been decided that M50 will keep Oilpan?
Shouldn't be long...

[haraken, 2016-03-02 13:52:56]

On 2016/03/02 13:45:13, sof wrote:
> On 2016/03/02 13:42:09, haraken wrote:
> > Overall looks good to me. Let's split the CL into pieces and landing.
> > 
> > I'm still wondering if ThreadHeap and ThreadState could be merged into one
> class
> > (the current distinction is pretty unclear) but I think we can revisit the
> issue
> > after landing other parts.
> 
> Can we hold off on landing until it has been decided that M50 will keep
Oilpan?
> Shouldn't be long...

Do you have a concern even if we don't enable the per-thread heap?

(FWIW, M50 will go to beta in late March.)

[sof, 2016-03-02 14:00:35]

On 2016/03/02 13:52:56, haraken wrote:
> On 2016/03/02 13:45:13, sof wrote:
> > On 2016/03/02 13:42:09, haraken wrote:
> > > Overall looks good to me. Let's split the CL into pieces and landing.
> > > 
> > > I'm still wondering if ThreadHeap and ThreadState could be merged into one
> > class
> > > (the current distinction is pretty unclear) but I think we can revisit the
> > issue
> > > after landing other parts.
> > 
> > Can we hold off on landing until it has been decided that M50 will keep
> Oilpan?
> > Shouldn't be long...
> 
> Do you have a concern even if we don't enable the per-thread heap?
> 

I have no concerns about M50 quality at this point.

> (FWIW, M50 will go to beta in late March.)

ok, a bit later than expected. 

The potential problem here is that we'll have to address late-breaking issues w/
Oilpan infrastructure on M50, which is made trickier if M51 deviates more than
it has to.

[keishi, 2016-03-15 04:21:35]

Description was changed from

==========
PerThreadHeap

BUG=
==========

to

==========
Refactor the Heap into ThreadHeap to prepare for per thread heaps

For per thread heap, we need to convert the static members in Heap to non-static
so we can have them for each per thread heap.

New classes:
ThreadHeap is for data tied to the set of ThreadStates that need to be stopped
for GC.
ProcessHeap is for global data about the Oilpan heap.
ThreadHeapStats is a bunch of stats for each ThreadHeap.

BUG=591606
==========

[keishi, 2016-03-15 04:21:35]

keishi@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[keishi, 2016-03-15 04:25:26]

On 2016/03/02 07:21:07, haraken wrote:
>
https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/heap/PageMemory.cpp (right):
> 
>
https://codereview.chromium.org/1477023003/diff/400001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/heap/PageMemory.cpp:160: PageMemoryRegion*
> pageMemoryRegion = PageMemoryRegion::allocateLargePage(allocationSize, heap);
> On 2016/03/02 06:01:03, keishi wrote:
> > On 2016/02/29 11:17:45, haraken wrote:
> > > 
> > > A better fix would be to call heap->addPageMemoryRegion(pageMemoryRegion)
> > here.
> > > 
> > > Then you can remove PageMemoryRegion::m_heap. (I don't think
> PageMemoryRegion
> > > needs to know the heap.)
> > 
> > We need to call removePageMemoryRegion from ~PageMemoryRegion so I don't
think
> I
> > can remove PageMemoryRegion::m_heap.
> 
> ~PageMemoryRegion is called only by 'delete current' in RegionTree::remove.
Can
> you move m_heap->removePageMemoryRegion(this) into RegionTree::remove? The
> RegionTree::remove can take the heap parameter.
> 
> (Nit: Ideally, those RegionTree methods should be members of the Heap class.
> Heap::addToRegionTree(), Heap::removeFromRegionTree() etc, which accesses
> Heap::m_regionTree.)

~PageMemoryRegion was actually called from PageMemoryRegion::pageDeleted so my
previous fix was wrong.

I realized that I need to bundle the RegionTree and the mutex into one object so
I can reference it without referencing the Heap.
So I did than in a separate CL:
https://codereview.chromium.org/1804863002/

This CL now depends on that CL.

[sof, 2016-03-16 20:33:56]

On 2016/03/02 14:00:35, sof wrote:
> On 2016/03/02 13:52:56, haraken wrote:
> > On 2016/03/02 13:45:13, sof wrote:
> > > On 2016/03/02 13:42:09, haraken wrote:
> > > > Overall looks good to me. Let's split the CL into pieces and landing.
> > > > 
> > > > I'm still wondering if ThreadHeap and ThreadState could be merged into
one
> > > class
> > > > (the current distinction is pretty unclear) but I think we can revisit
the
> > > issue
> > > > after landing other parts.
> > > 
> > > Can we hold off on landing until it has been decided that M50 will keep
> > Oilpan?
> > > Shouldn't be long...
> > 
> > Do you have a concern even if we don't enable the per-thread heap?
> > 
> 
> I have no concerns about M50 quality at this point.
> 
> > (FWIW, M50 will go to beta in late March.)
> 
> ok, a bit later than expected. 
> 
> The potential problem here is that we'll have to address late-breaking issues
w/
> Oilpan infrastructure on M50, which is made trickier if M51 deviates more than
> it has to.

The promotion of M50 on beta channels happened sooner than expected, Android
upgraded to 50.0.2661.35 today.

[keishi, 2016-03-17 06:06:24]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-17 06:06:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477023003/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477023003/540001

[commit-bot: I haz the power, 2016-03-17 06:41:50]

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

[commit-bot: I haz the power, 2016-03-17 06:41:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[keishi, 2016-03-17 07:42:28]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-17 07:42:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477023003/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477023003/560001

[commit-bot: I haz the power, 2016-03-17 08:36:19]

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

[commit-bot: I haz the power, 2016-03-17 08:36:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[keishi, 2016-03-18 03:31:17]

Description was changed from

==========
Refactor the Heap into ThreadHeap to prepare for per thread heaps

For per thread heap, we need to convert the static members in Heap to non-static
so we can have them for each per thread heap.

New classes:
ThreadHeap is for data tied to the set of ThreadStates that need to be stopped
for GC.
ProcessHeap is for global data about the Oilpan heap.
ThreadHeapStats is a bunch of stats for each ThreadHeap.

BUG=591606
==========

to

==========
Refactor the Heap into ThreadHeap to prepare for per thread heaps

For per thread heap, we need to convert the static members in Heap to non-static
so we can have them for each per thread heap.

New classes:
ThreadHeap is for data tied to the set of ThreadStates that need to be stopped
for GC.
ProcessHeap is for global data about the Oilpan heap.
ThreadHeapStats is a bunch of stats for each ThreadHeap.

BUG=591606
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq
==========

[keishi, 2016-03-18 03:32:59]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-18 03:33:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477023003/580001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477023003/580001

[commit-bot: I haz the power, 2016-03-18 05:01:32]

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

[commit-bot: I haz the power, 2016-03-18 05:01:33]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[keishi, 2016-03-18 05:27:30]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-18 05:27:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477023003/600001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477023003/600001

[commit-bot: I haz the power, 2016-03-18 06:28:55]

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

[commit-bot: I haz the power, 2016-03-18 06:28:56]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[haraken, 2016-03-19 08:12:06]

This CL is a bit too big to land in one go. Would it be possible to split the CL
into at least the following two pieces?

1) Introduce ProcessHeap, ThreadHeapStats, ThreadHeap etc without any functional
change.
2) Change the logic of thread attach, detach and shutdown.

Also would you confirm that this CL does not regress performance of the marking
phase? It looks like that this CL introduces some pointer dereferences to the
performance-sensitive tracing function, so we might want to mitigate the
concern.