Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
371623002:
[oilpan]: Make thread shutdown more robust. (Closed)
|
Created:
6 years, 5 months ago by wibling-chromium Modified:
6 years, 5 months ago Reviewers:
Vyacheslav Egorov (Chromium), Mads Ager (chromium), tkent, zerny-chromium, Erik Corry, haraken, oilpan-reviews CC:
blink-reviews, kouhei+heap_chromium.org, Mads Ager (chromium) Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
Description[oilpan]: Make thread shutdown more robust.
Change the way we shutdown threads to ensure we are not leaving dangling pointers from other thread heaps into pages being freed at shutdown.
Thread shutdown algorithm:
1. Mark all pages belonging to the thread as being in the shutdown phase. This helps us to avoid trace pages not belonging to the thread being shut down.
2. Trace the thread’s objects using only the thread local persistents as roots.
3. Sweep the thread’s pages, any empty pages are put in the orphaned page pool since there is a risk a conservatively found (dead) object in another thread could trace this thread’s pages in the following global GC. The page is moved from the orphaned page pool to the memory pool after a global GC marking at which point we know it cannot be reached.
4. Repeat step 2 and 3 until the number of thread local persistents reaches zero or does not decrease.
5. Do a final sweep with no tracing to finalize the remaining objects in the thread’s heaps.
6. When destructing the thread state move any remaining pages or large objects to the orphaned page pool.
To make the above assumption of the next global GC marking being sufficient to know that a page cannot be reached we need to guarantee that an object cannot be revived. Reviving an object can happen if a thread doesn’t get to sweep before the next GC marking phase. In that case the new marking could conservatively find an object on the heap that were not found in the first marking and hence revive an object that were dead.
This newly revived object could contain pointers to another object on a thread that had swept its pages and we would then try to trace potential garbage or even a newly allocated object on the swept object’s location.
We have introduced a dead bit to avoid the above. This dead bit is set during the prepareForGC call if a thread didn’t get to sweep before the new GC.
R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=375671
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178044
Patch Set 1 #Patch Set 2 : #
Total comments: 28
Patch Set 3 : review feedback #
Total comments: 116
Patch Set 4 : review feedback #
Total comments: 43
Patch Set 5 : #
Total comments: 15
Patch Set 6 : more review feedback #Patch Set 7 : rebase and merge #Patch Set 8 : #
Total comments: 20
Patch Set 9 : #Patch Set 10 : fix heapTests compile on android #Patch Set 11 : #
Messages
Total messages: 34 (0 generated)
PTAL. I have updated the description and fixed a null pointer issue.
Mostly lgtm. I think the comments can be tightened a bit and updated to include the description of the orphanage as a way to avoid use-after-free on cross-thread pointers (both programmer errors and conservative-interpretation errors). https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:764: // it is still reachable. After the next full GC it can be released. Update the reason to explain that it could also be found if by a programming error another thread has kept a cross-thread pointer. I think this is the "primary" cause since it is the assumption that we are making in doing a "thread-local GC". If it turns out not to be true we still want to maintain safety. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:852: m_pool[index] = entry; Nit: call addOrphanedPage(index, page) instead https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:868: // We mark the page as dead. This clears the traced flag => We mark the page as orphaned. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:932: // The thread is shutting down so this page is being removed as part of Nit: as before, it is just as much guarding against an actual live pointer from another heap. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1526: if (ThreadLocal ? (heapPage->orphaned() || !heapPage->shuttingDown()) : heapPage->orphaned()) { Nit: if (heapPage->orphaned() || !(ThreadLocal && heapPage->shuttingDown())) https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2005: // objects that were not traced in the previous GC as dead. In this GC's marking => that are dead but not swept. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2007: // where we need to check for this is during conservative scanning. Nit: again, a programming error could also result in tracing on an orphaned page. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:72: // The dead bit is used for objects that have gone through a GC marking, but did Simpler: The dead bit is set for objects that are dead but were not swept before a new GC started. This bit is used to make sure we do not revive a dead object via a conservatively found pointer. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:226: void setDeadMark(); Nit: markDead/unmarkDead for consistency with the existing methods. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:304: inline bool hasDeadMark() const; markDead/unmarkDead/isMarkedDead https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:463: void clearLiveAndMarkDead(); Nit: maybe unmarkLiveAndMarkDead would read more clearly. I initially read it as "clear live-bits and mark-dead-bits", ie, "clear all marks". https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:2360: if (ThreadLocal ? (heapPage->orphaned() || !heapPage->shuttingDown()) : heapPage->orphaned()) { if (heapPage->orphaned() || !(ThreadLocal && heapPage->shuttingDown())) https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:450: // Subsequently they are promoted to the memoryPool once they are no s/promoted/moved https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:479: // do nothing when popping the trace method. Simpler: We assume there are no CrossThreadPersistents to this threads heap. If there is, it will be handled in the same way as any other cross-thread pointer pointing to an orphaned page. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:605: return m_weakCallbackStack->popAndInvokeCallback<false>(&m_weakCallbackStack, visitor); Nit: maybe we make an type/enum for ThreadLocal/GlobalGC https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:752: } Nit: no curlies
Thanks for the review. I will update the diff ASAP. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:764: // it is still reachable. After the next full GC it can be released. On 2014/07/07 12:11:56, zerny-chromium wrote: > Update the reason to explain that it could also be found if by a programming > error another thread has kept a cross-thread pointer. > > I think this is the "primary" cause since it is the assumption that we are > making in doing a "thread-local GC". If it turns out not to be true we still > want to maintain safety. Done. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:852: m_pool[index] = entry; On 2014/07/07 12:11:56, zerny-chromium wrote: > Nit: call addOrphanedPage(index, page) instead Done. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:868: // We mark the page as dead. This clears the traced flag On 2014/07/07 12:11:56, zerny-chromium wrote: > => We mark the page as orphaned. Done. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:932: // The thread is shutting down so this page is being removed as part of On 2014/07/07 12:11:56, zerny-chromium wrote: > Nit: as before, it is just as much guarding against an actual live pointer from > another heap. Done. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1526: if (ThreadLocal ? (heapPage->orphaned() || !heapPage->shuttingDown()) : heapPage->orphaned()) { On 2014/07/07 12:11:56, zerny-chromium wrote: > Nit: if (heapPage->orphaned() || !(ThreadLocal && heapPage->shuttingDown())) I don't think that would work. In the case where ThreadLocal is false we would always be true independent of heapPage->orphaned(). I agree the current style is not very pretty, but it is reasonably easy to determine what it does. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2005: // objects that were not traced in the previous GC as dead. In this GC's marking On 2014/07/07 12:11:56, zerny-chromium wrote: > => that are dead but not swept. Done. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2007: // where we need to check for this is during conservative scanning. On 2014/07/07 12:11:56, zerny-chromium wrote: > Nit: again, a programming error could also result in tracing on an orphaned > page. Done. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:450: // Subsequently they are promoted to the memoryPool once they are no On 2014/07/07 12:11:56, zerny-chromium wrote: > s/promoted/moved Done. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:479: // do nothing when popping the trace method. On 2014/07/07 12:11:56, zerny-chromium wrote: > Simpler: > > We assume there are no CrossThreadPersistents to this threads heap. If there is, > it will be handled in the same way as any other cross-thread pointer pointing to > an orphaned page. Done. Much better:) https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:605: return m_weakCallbackStack->popAndInvokeCallback<false>(&m_weakCallbackStack, visitor); On 2014/07/07 12:11:56, zerny-chromium wrote: > Nit: maybe we make an type/enum for ThreadLocal/GlobalGC I prefer keeping it as a bool to keep the if in the invokeCallback methods simpler, ie. no ==. https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:752: } On 2014/07/07 12:11:56, zerny-chromium wrote: > Nit: no curlies Done.
This is a nice change!!
I haven't looked at Heap.{h,cpp}, which I'll take a look tomorrow :)
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (right):
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:111: // out.
I'm not sure if this assumption is correct. Heap::collectGarbageForThread() at
line 415 can call performPendingSweep() on the thread heap. The sweep can call
any destructors, and the destructor can enter a safe point. Then the thread can
get involved in a global GC while doing the thread-local GC.
Can we make the logic of the mutual exclusion more explicit?
What we have to guarantee is that no two (thread-local or global) GCs must run
at the same time. Can we prepare one mutex shared among thread-local GCs and
global GCs in order to guarantee the fact more straightforwardly?
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:405: // Set a flag on all this thread's
heap pages to ensure we don't trace
Set flags on all heap pages of this thread to ensure we don't trace outside of
the heap pages.
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:413: int curCount =
anchor->numberOfPersistents();
curCount => currentCount
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:414: while (curCount > 0 && curCount !=
oldCount) {
I'm not sure if 'curCount == oldCount' is enough to check that we've finished
sweeping pages of the thread heap.
Destructing objects can decrease # of persistents, but it's possible that
destructors can allocate new persistents. Then 'curCount == oldCount' won't mean
that we've finished sweeping pages of the thread heap.
However, this would be a too artificial case, so it wouldn't be worth adding
special logic for this. I think we can just add a RELEASE_ASSERT to verify that
no new persistents are allocated during the thread-local GC. If we hit the
RELEASE_ASSERT, we should restructure the code.
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:444: MutexLocker
locker(threadAttachMutex());
This should be SafePointAwareMutexLocker. Then we can remove the line 440 and
441 above.
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:740: // If there are parked threads with
outstanding sweep requests, clear their mark bits,
If a next GC is requested before processing a sweeping request (this can happen
if the thread did not have time to wake up and sweep before the next GC
arrives), clear all the mark bits and ...
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:744: // object.
Just help me understand: Is this "revive" problem a new problem that arises when
we introduce the orphaned pages? Or have we already had the problem when a
thread fails to sweep the page before the next GC arrives?
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:757: heap->setShutdown();
setShutdown => setHeapForShutdown
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.cpp (right):
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.cpp:57: ASSERT(page->gcInfo() == gcInfo);
\
Don't we need to check that page->orphaned()?
ASSERT(page->orphaned() || page->gcInfo() == gcInfo);
Added a couple of more comments. Great change anyway. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (left): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:870: flushHeapContainsCache(); Just help me understand: Why can we drop flushHeapContainsCache()? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:577: flushHeapContainsCache(); Don't you need to flush HeapDoesNotContainCache as well? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:764: // in another thread heap keeps a dangling pointer to the this object. the this object => this object https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:765: // To guard agains this we put the large object memory in the against https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:767: // GC it can be released assuming no rogue/dangling pointers refer to full GC => global GC https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:806: MutexLocker locker(m_mutex[index]); Just help me understand: Why do we need a mutex here? I was thinking that HeapMemoryPool is per-thread to guarantee that virtual memory address is not shared among threads. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:859: // No locking needed as all threads are at safepoints at this point in time. Can we add an ASSERT about this? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:882: // Call the destructor before freeing or adding to the memory pool. Just help me understand: Why does this order matter? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:883: if (page->reuseMemory()) { reuseMemory => shouldReuseMemory ? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:899: bool HeapOrphanedPagePool::contains(void* object) Just to confirm: HeapOrphanedPagePool::contains() is not called in a hot call path, right? If it's called from a hot call path, we might want to put the result into HeapContainsCache. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:937: // in another thread heap keeps a dangling pointer to the this object. the this object => this object https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:938: // To guard agains this we put the page in the orphanedPagePool to against https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:939: // ensure it is still reachable. After the next full GC it can be full GC => global GC https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:955: while (!pageMemory) { I'm curious why we need change 'if' to 'while'? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1231: header->setDeadMark(); Slightly better: if (header->isMarked()) header->unmark(); else if (!header->isFree()) header->setDeadMark() headerAddress += header->size(); https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1946: return s_weakCallbackStack->popAndInvokeCallback<false>(&s_weakCallbackStack, visitor); We prefer enum than true/false. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2012: // a dangling pointer. In my understanding, if we're performing a precise GC, orphaned pages must not be traced. Can we add a RELEASE_ASSERT about the fact? I think the RELEASE_ASSERT will give us stack traces of the code where we should fix. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:229: // We clear out the payload to detect incorrect usage. Don't we want to zap the orphaned page in Debug builds? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:684: class HeapMemoryPool : public HeapPool<PageMemory> { It's a bit confusing to mix "HeapPool", "HeapMemoryPool", "OrphanedPagePool" etc. We might want to rename: HeapMemoryPool => PagePool HeapPool => PagePoolBase https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:900: // put their pages into the correct pool for avoiding type confusion. // This is used to ensure that the pages of the same type go into the correct memory pool and thus avoid type confusion. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:2354: // trace it. This can happen when a conservative GC kept a dead object // If the object being traced is located on a dead page, don't trace it. This can happen when a conservative GC finds a pointer to the dead object. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:2356: // Also if doing a thread local GC don't trace objects that are located // Also if we're now performing a thread local GC and the object being traced is located on another thread's heap, don't trace it. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:2360: if (ThreadLocal ? (heapPage->orphaned() || !heapPage->shuttingDown()) : heapPage->orphaned()) { I'd rename "shuttingDown" to "isTargetOfThreadLocalGC" or something like that. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:2360: if (ThreadLocal ? (heapPage->orphaned() || !heapPage->shuttingDown()) : heapPage->orphaned()) { Would it be possible to add RELEASE_ASSERT(!heapPage->orphaned()) when we're performing a precise GC? It will give us stack traces of the code we should fix. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:2363: heapPage->setTraced(); Slightly more readable: if (ThreadLocal) { if (heapPage->orphaned() || !heapPage->shuttingDown()) return true; } else { if (heapPage->orphaned()) { heapPage->setTraced(); return true; } } https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:2: * Copyright (C) 2013 Google Inc. All rights reserved. I understand that it's a pain to add tests, but we might want to have tests for this change because a lot of non-trivial things happen around the orphaned pages. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:703: void setReuseMemory() { m_reuseMemory = true; } Instead of introducing clearReuseMemory and setReuseMemory, can we just override reuseMemory() in LargeHeapObject so that reuseMemory() returns true only for LargeHeapObjects?
https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1526: if (ThreadLocal ? (heapPage->orphaned() || !heapPage->shuttingDown()) : heapPage->orphaned()) { On 2014/07/07 13:50:07, wibling-chromium wrote: > On 2014/07/07 12:11:56, zerny-chromium wrote: > > Nit: if (heapPage->orphaned() || !(ThreadLocal && heapPage->shuttingDown())) > > I don't think that would work. In the case where ThreadLocal is false we would > always be true independent of heapPage->orphaned(). > I agree the current style is not very pretty, but it is reasonably easy to > determine what it does. Sorry, I messed up the negation. It should be: heapPage->orphaned() || (ThreadLocal && !heapPage->shuttingDown()) which reads well: if orphaned or if thread-local for a non-shutting-down thread...
Thank you for working on this Gustav. This change is fixing a bunch of real issues that we have with the infrastructure now! :) https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (left): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:870: flushHeapContainsCache(); On 2014/07/08 05:44:50, haraken wrote: > > Just help me understand: Why can we drop flushHeapContainsCache()? Because unlink now calls removePageFromHeap which does this. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:761: // of a thread local GC. In that case the object could be revived in the revived -> traced https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:762: // next global GC either due to a dead object being revived via a revived -> traced https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:843: void HeapOrphanedPagePool::addOrphanedPage(int index, BaseHeapPage* page) For these methods we know that some mutex is already held so that only one thread can modify the orphaned page pool at a time, right? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:883: if (page->reuseMemory()) { !page->isLargeObject() https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:885: Heap::memoryPool()->addMemory(index, memory); So this is where we use that the page pool is now global. We could keep the page pool thread local and then always delete the pages here. That would avoid the mutex when you want to add pages to the page pool and get pages from the page pool. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:934: // of a thread local GC. In that case the page could be revived in the revived -> accessed https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:935: // next global GC either due to a dead object being revived via a revived -> traced https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:955: while (!pageMemory) { On 2014/07/08 05:44:50, haraken wrote: > > I'm curious why we need change 'if' to 'while'? Because the page pool is not global and other threads could have stolen the pages that we just added. I think we might want to keep the page pool thread local. It is simpler and it will not require us to acquire a mutex to add and retrieve pages to the pool. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2007: // cannot be revived in a subsequent GC. This is due to a thread either having revived -> reached https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2010: // marking we check that any object marked as dead is not revived. E.g. via a revived -> traced https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2035: NoAllocationScope<AnyThread> noAllocationScope; This no allocation scope covers the sweep as well. It shouldn't since we allow allocation during finalization. While sweeping allocations always succeed so we will not enter a safe point when allocating during sweeping. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:74: // objects that were not marked in the previous GC to ensure we are not reviving I think we should avoid using the word 'reviving' here (the object doesn't actually become live again). ... to ensure that we are not tracing them via a conservatively found pointer. Tracing dead objects could lead to tracing of already finalized objects in another thread's heap which is a use-after-free situation. ? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:231: clearReuseMemory(); // Don't reuse memory for large objects, ie. don't move to the memory pool. It looks like this is only used to indicate that this page is a large object page. However, we already know that because we have a virtual isLargeObject method. Can we use that and remove the duplication of that information in the reuse memory flag? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:477: // We clear out the payload to detect incorrect usage. Maybe update the comment to state that incorrect usage means cross-thread pointers into the heap of a dead thread? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:479: setReuseMemory(); // Reuse memory for normal blink pages. Can we get rid of this reuse memory bit and just use the isLargeObject virtual method? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:937: template<bool ThreadLocal> static bool popAndInvokeTraceCallback(Visitor*); Could you introduce an enum for this to make the call-sites easier to read? enum GCType { NORMAL_GC, THREAD_LOCAL_GC } https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:954: static void collectGarbageForThread(ThreadState*, bool); Maybe call this collectGarbageForTerminatingThread to make it a bit clearer that this is not something that can be called to do a general GC for a thread. This is only valid when the thread is dying and we therefore have the assumption that there should be no real cross-thread pointers. Could you add a name to the bool so it is clear for the signature what that bool means? (The rule about adding names to arguments is that you should not add a name if it doesn't add anything, so don't add 'ThreadState* threadState' since that just repeats that it is a thread state. However, bool doesn't mean anything unless it has a name.) https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:956: template<bool ThreadLocal> static void tracingAndGlobalWeakProcessing(); traceAndPerformGlobalWeakProcessing? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:1006: static HeapMemoryPool* s_memoryPool; Do we want the page pool to be global or do we want that to be thread local? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:2337: bool CallbackStack::popAndInvokeCallback(CallbackStack** first, Visitor* visitor) We might want to keep this implementation in the cpp file. You should be able to do that by forcing the two template instantiations in the cpp file. template bool CallbackStack::popAndInvokeCallback<true>(args); template bool CallbackStack::popAndInvokeCallback<false>(args); We have such explicit instantiations in order to be able to have much of the implementation in the cpp file already for the different object header types. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:422: Heap::collectGarbageForThread(this, true); Add enum instead of bool? https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:742: // GC marking does not revive already dead objects. If we revived a dead object we revive -> trace revived -> traced https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:664: // Common header for heap pages. Needs to be defined before class Visitor. Our file setup is becoming really messy. :-\ This change is important to land, so let's leave it this way but it seems messy to have BaseHeapPage here and all the subclasses in Heap.h. Maybe we should split into more files, but let's leave that for later. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:703: void setReuseMemory() { m_reuseMemory = true; } On 2014/07/08 05:44:51, haraken wrote: > > Instead of introducing clearReuseMemory and setReuseMemory, can we just override > reuseMemory() in LargeHeapObject so that reuseMemory() returns true only for > LargeHeapObjects? We already have this information. We have the virtual isLargeObject method, so I think we should just use that one? :) https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:714: bool m_reuseMemory; Are you sure we are maintaining correct alignment now? Unfortunately, this might only break on Windows if we get it wrong.
Thanks for all the reviews! I have replied inline below and will update the diff ASAP. As part of this I also changed the cleanup algorithm to do a "normal" thread local GC for the final sweep in response to tkent's recent change. This should avoid us doing any finalization of objects that are still referred by left-over persistents. I have added a debug assert to validate that we do reach zero thread local persistents when terminating, if that hits we should investigate further. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (left): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:870: flushHeapContainsCache(); On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > On 2014/07/08 05:44:50, haraken wrote: > > > > Just help me understand: Why can we drop flushHeapContainsCache()? > > Because unlink now calls removePageFromHeap which does this. Yes, it seemed like we did this a bit too many times before. Also having the flushing happen at the points where we either add (in the HeapDoesNotContainsCache) or remove (in the HeapContainsCache) seems simpler to reason about. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:577: flushHeapContainsCache(); On 2014/07/08 05:44:51, haraken wrote: > > Don't you need to flush HeapDoesNotContainCache as well? No, we only need to flush the HeapDoesNotContainCache when adding pages to a heap. Removing a page only means we have something that is removed and not in the cache. It will then be added to cache if we try looking it up. The reverse is the case for the HeapContainsCache. Here we only need to flush when removing a page from a heap, ie. during shutdown. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:761: // of a thread local GC. In that case the object could be revived in the On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > revived -> traced Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:762: // next global GC either due to a dead object being revived via a On 2014/07/08 08:24:55, Mads Ager (chromium) wrote: > revived -> traced Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:764: // in another thread heap keeps a dangling pointer to the this object. On 2014/07/08 05:44:50, haraken wrote: > > the this object => this object Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:765: // To guard agains this we put the large object memory in the On 2014/07/08 05:44:50, haraken wrote: > > against Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:767: // GC it can be released assuming no rogue/dangling pointers refer to On 2014/07/08 05:44:51, haraken wrote: > > full GC => global GC Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:806: MutexLocker locker(m_mutex[index]); On 2014/07/08 05:44:51, haraken wrote: > > Just help me understand: Why do we need a mutex here? I was thinking that > HeapMemoryPool is per-thread to guarantee that virtual memory address is not > shared among threads. I have changed the page pool to be global. The reason being that we could not reuse any orphaned pages if not and it also guarantees that no virtual address space will be reused for another object type even after a thread shuts down. See my reply to Mads on this as well for some more details. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:843: void HeapOrphanedPagePool::addOrphanedPage(int index, BaseHeapPage* page) On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > For these methods we know that some mutex is already held so that only one > thread can modify the orphaned page pool at a time, right? Yes, basically we only add pages to the orphaned page pool during a thread local GC which there can only be one of at a time and we also guarantee that no global GC is currently decommiting any orphaned pages at this point since there can be no global GC when doing a thread local GC. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:859: // No locking needed as all threads are at safepoints at this point in time. On 2014/07/08 05:44:51, haraken wrote: > > Can we add an ASSERT about this? Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:882: // Call the destructor before freeing or adding to the memory pool. On 2014/07/08 05:44:50, haraken wrote: > > Just help me understand: Why does this order matter? We cannot call the destructor after adding the memory to the page pool (or deleting it) since in the former case the memory backing the BaseHeapPage is decommited and in the latter it is freed. That said we right now don't do anything in the page destructor, but it seemed like the right thing to ensure it is called in case we add something that needs destruction in the future. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:883: if (page->reuseMemory()) { On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > !page->isLargeObject() Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:885: Heap::memoryPool()->addMemory(index, memory); On 2014/07/08 08:24:55, Mads Ager (chromium) wrote: > So this is where we use that the page pool is now global. We could keep the page > pool thread local and then always delete the pages here. That would avoid the > mutex when you want to add pages to the page pool and get pages from the page > pool. Yes, I would like to keep it global for now and in the case it becomes a performance issue we can make it local to the threads and get rid of the lock. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:899: bool HeapOrphanedPagePool::contains(void* object) On 2014/07/08 05:44:50, haraken wrote: > > Just to confirm: HeapOrphanedPagePool::contains() is not called in a hot call > path, right? If it's called from a hot call path, we might want to put the > result into HeapContainsCache. No, it is only called inside an ASSERT. I will wrap it in #ifndef NDEBUG. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:934: // of a thread local GC. In that case the page could be revived in the On 2014/07/08 08:24:55, Mads Ager (chromium) wrote: > revived -> accessed Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:935: // next global GC either due to a dead object being revived via a On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > revived -> traced Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:937: // in another thread heap keeps a dangling pointer to the this object. On 2014/07/08 05:44:51, haraken wrote: > > the this object => this object Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:938: // To guard agains this we put the page in the orphanedPagePool to On 2014/07/08 05:44:50, haraken wrote: > > against Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:939: // ensure it is still reachable. After the next full GC it can be On 2014/07/08 05:44:51, haraken wrote: > > full GC => global GC Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1231: header->setDeadMark(); On 2014/07/08 05:44:50, haraken wrote: > > Slightly better: > > if (header->isMarked()) > header->unmark(); > else if (!header->isFree()) > header->setDeadMark() > headerAddress += header->size(); Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1946: return s_weakCallbackStack->popAndInvokeCallback<false>(&s_weakCallbackStack, visitor); On 2014/07/08 05:44:51, haraken wrote: > > We prefer enum than true/false. Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2007: // cannot be revived in a subsequent GC. This is due to a thread either having On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > revived -> reached Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2010: // marking we check that any object marked as dead is not revived. E.g. via a On 2014/07/08 08:24:55, Mads Ager (chromium) wrote: > revived -> traced Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2012: // a dangling pointer. On 2014/07/08 05:44:50, haraken wrote: > > In my understanding, if we're performing a precise GC, orphaned pages must not > be traced. Can we add a RELEASE_ASSERT about the fact? I think the > RELEASE_ASSERT will give us stack traces of the code where we should fix. That is correct. I will try to add a RELEASE_ASSERT(Heap::s_lastGCWasConservative) under the orphaned page check, when popping trace callbacks. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2035: NoAllocationScope<AnyThread> noAllocationScope; On 2014/07/08 08:24:55, Mads Ager (chromium) wrote: > This no allocation scope covers the sweep as well. It shouldn't since we allow > allocation during finalization. While sweeping allocations always succeed so we > will not enter a safe point when allocating during sweeping. Good point. Fixed. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:74: // objects that were not marked in the previous GC to ensure we are not reviving On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > I think we should avoid using the word 'reviving' here (the object doesn't > actually become live again). > > ... to ensure that we are not tracing them via a conservatively found pointer. > Tracing dead objects could lead to tracing of already finalized objects in > another thread's heap which is a use-after-free situation. > > ? Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:229: // We clear out the payload to detect incorrect usage. On 2014/07/08 05:44:51, haraken wrote: > > Don't we want to zap the orphaned page in Debug builds? Good idea. I added a new zap value to detect orphaned pages. I did it in both Release and Debug since it should perform the same AFAICT. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:231: clearReuseMemory(); // Don't reuse memory for large objects, ie. don't move to the memory pool. On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > It looks like this is only used to indicate that this page is a large object > page. However, we already know that because we have a virtual isLargeObject > method. Can we use that and remove the duplication of that information in the > reuse memory flag? Good idea. Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:477: // We clear out the payload to detect incorrect usage. On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > Maybe update the comment to state that incorrect usage means cross-thread > pointers into the heap of a dead thread? Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:479: setReuseMemory(); // Reuse memory for normal blink pages. On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > Can we get rid of this reuse memory bit and just use the isLargeObject virtual > method? Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:684: class HeapMemoryPool : public HeapPool<PageMemory> { On 2014/07/08 05:44:51, haraken wrote: > > It's a bit confusing to mix "HeapPool", "HeapMemoryPool", "OrphanedPagePool" > etc. We might want to rename: > > HeapMemoryPool => PagePool > HeapPool => PagePoolBase Good idea. I changed it to HeapPool => PagePool HeapMemoryPool = FreePagePool HeapOrphanedPagePool => OrphanedPagePool https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:900: // put their pages into the correct pool for avoiding type confusion. On 2014/07/08 05:44:51, haraken wrote: > > // This is used to ensure that the pages of the same type go into the correct > memory pool and thus avoid type confusion. Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:937: template<bool ThreadLocal> static bool popAndInvokeTraceCallback(Visitor*); On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > Could you introduce an enum for this to make the call-sites easier to read? > > enum GCType { > NORMAL_GC, > THREAD_LOCAL_GC > } Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:954: static void collectGarbageForThread(ThreadState*, bool); On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > Maybe call this collectGarbageForTerminatingThread to make it a bit clearer that > this is not something that can be called to do a general GC for a thread. This > is only valid when the thread is dying and we therefore have the assumption that > there should be no real cross-thread pointers. > > Could you add a name to the bool so it is clear for the signature what that bool > means? > > (The rule about adding names to arguments is that you should not add a name if > it doesn't add anything, so don't add 'ThreadState* threadState' since that just > repeats that it is a thread state. However, bool doesn't mean anything unless it > has a name.) Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:956: template<bool ThreadLocal> static void tracingAndGlobalWeakProcessing(); On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > traceAndPerformGlobalWeakProcessing? Done. Changed it to traceRootsAndPerformGlobalWeakProcessing. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:1006: static HeapMemoryPool* s_memoryPool; On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > Do we want the page pool to be global or do we want that to be thread local? There are trade-offs for both. With the global version we can reuse orphaned pages whereas we cannot do that if we have a per thread free page pool. Reusing ensures we are never using virtual adress space for an object of a different type even when a thread dies. The downside of the global version is the mutex, but I doubt that will be an issue since it is only on page allocation which is relatively rare. If it becomes an issue we can change it to thread local, but I would prefer to keep it as is for this change. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:2: * Copyright (C) 2013 Google Inc. All rights reserved. On 2014/07/08 05:44:51, haraken wrote: > > I understand that it's a pain to add tests, but we might want to have tests for > this change because a lot of non-trivial things happen around the orphaned > pages. I will try to cook something up. It might be in a follow-up change if that's okay with you. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:111: // out. On 2014/07/07 16:13:47, haraken wrote: > > I'm not sure if this assumption is correct. Heap::collectGarbageForThread() at > line 415 can call performPendingSweep() on the thread heap. The sweep can call > any destructors, and the destructor can enter a safe point. Then the thread can > get involved in a global GC while doing the thread-local GC. > > Can we make the logic of the mutual exclusion more explicit? > > What we have to guarantee is that no two (thread-local or global) GCs must run > at the same time. Can we prepare one mutex shared among thread-local GCs and > global GCs in order to guarantee the fact more straightforwardly? It should not be the case that we enter a safepoint while sweeping. If we do so we will have hit the issues discussed in the SafePointAwareMutexLocker change, https://codereview.chromium.org/367633002. Basically entering a safepoint while sweeping is not allowed. There is also an ASSERT in SafePointBarriers::enterSafePoint which will fire if we try to enter a safepoint while sweeping. Regarding allocations during sweep, Mads changed the shouldGC/shouldForceConservativeGC methods to return false if sweeping to ensure we don't try to GC while sweeping. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:405: // Set a flag on all this thread's heap pages to ensure we don't trace On 2014/07/07 16:13:47, haraken wrote: > > Set flags on all heap pages of this thread to ensure we don't trace outside of > the heap pages. Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:413: int curCount = anchor->numberOfPersistents(); On 2014/07/07 16:13:46, haraken wrote: > > curCount => currentCount Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:414: while (curCount > 0 && curCount != oldCount) { On 2014/07/07 16:13:47, haraken wrote: > > I'm not sure if 'curCount == oldCount' is enough to check that we've finished > sweeping pages of the thread heap. > > Destructing objects can decrease # of persistents, but it's possible that > destructors can allocate new persistents. Then 'curCount == oldCount' won't mean > that we've finished sweeping pages of the thread heap. > > However, this would be a too artificial case, so it wouldn't be worth adding > special logic for this. I think we can just add a RELEASE_ASSERT to verify that > no new persistents are allocated during the thread-local GC. If we hit the > RELEASE_ASSERT, we should restructure the code. I have added an ASSERT to PersistentBase to check that we are not in the while doing thread shutdown. I didn't make it a RELEASE_ASSERT since it seems like a rather large overhead to pay on each Persistent allocation given that I have to get to the ThreadState. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:422: Heap::collectGarbageForThread(this, true); On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > Add enum instead of bool? Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:444: MutexLocker locker(threadAttachMutex()); On 2014/07/07 16:13:46, haraken wrote: > > This should be SafePointAwareMutexLocker. Then we can remove the line 440 and > 441 above. Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:740: // If there are parked threads with outstanding sweep requests, clear their mark bits, On 2014/07/07 16:13:47, haraken wrote: > > If a next GC is requested before processing a sweeping request (this can happen > if the thread did not have time to wake up and sweep before the next GC > arrives), clear all the mark bits and ... Rephrased it a bit. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:742: // GC marking does not revive already dead objects. If we revived a dead object we On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > revive -> trace > > revived -> traced Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:744: // object. On 2014/07/07 16:13:46, haraken wrote: > > Just help me understand: Is this "revive" problem a new problem that arises when > we introduce the orphaned pages? Or have we already had the problem when a > thread fails to sweep the page before the next GC arrives? This is an issue we discovered while I was doing this change. Basically if we have two threads with a cross thread pointer going from one thread heap to the other thread heap we could end up tracing into garbage or another object if the "pointing" thread didn't sweep, but the "pointed to" thread did sweep. The new dead bits should fix this. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:757: heap->setShutdown(); On 2014/07/07 16:13:46, haraken wrote: > > setShutdown => setHeapForShutdown Changed it to prepareHeapForShutdown. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:664: // Common header for heap pages. Needs to be defined before class Visitor. On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > Our file setup is becoming really messy. :-\ This change is important to land, > so let's leave it this way but it seems messy to have BaseHeapPage here and all > the subclasses in Heap.h. Maybe we should split into more files, but let's leave > that for later. Agree. I was more thinking about going in the opposite direction and combining some of our header files, since they seems to be quite intertwined anyway. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:703: void setReuseMemory() { m_reuseMemory = true; } On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > On 2014/07/08 05:44:51, haraken wrote: > > > > Instead of introducing clearReuseMemory and setReuseMemory, can we just > override > > reuseMemory() in LargeHeapObject so that reuseMemory() returns true only for > > LargeHeapObjects? > > We already have this information. We have the virtual isLargeObject method, so I > think we should just use that one? :) Done. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:703: void setReuseMemory() { m_reuseMemory = true; } On 2014/07/08 05:44:51, haraken wrote: > > Instead of introducing clearReuseMemory and setReuseMemory, can we just override > reuseMemory() in LargeHeapObject so that reuseMemory() returns true only for > LargeHeapObjects? I got rid of this and used isLargeObject as suggested by Mads. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:714: bool m_reuseMemory; On 2014/07/08 08:24:56, Mads Ager (chromium) wrote: > Are you sure we are maintaining correct alignment now? Unfortunately, this might > only break on Windows if we get it wrong. No, I will spend some time to make sure it is okay. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.cpp (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Vis... Source/platform/heap/Visitor.cpp:57: ASSERT(page->gcInfo() == gcInfo); \ On 2014/07/07 16:13:47, haraken wrote: > > Don't we need to check that page->orphaned()? > > ASSERT(page->orphaned() || page->gcInfo() == gcInfo); No, it should be necessary since we can get to the GC info even for an orphaned page since it is at the page header rather than the object header.
PTAL. The diff is updated with the review feedback. Notice I have also changed the representation of m_traced and m_shuttingDown in BaseHeapPage to ensure we have the correct page header alignment.
(I'll take another close look at Heap.{h,cpp} shortly...)
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea...
File Source/platform/heap/HeapTest.cpp (right):
https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea...
Source/platform/heap/HeapTest.cpp:2: * Copyright (C) 2013 Google Inc. All rights
reserved.
On 2014/07/08 13:39:47, wibling-chromium wrote:
> On 2014/07/08 05:44:51, haraken wrote:
> >
> > I understand that it's a pain to add tests, but we might want to have tests
> for
> > this change because a lot of non-trivial things happen around the orphaned
> > pages.
>
> I will try to cook something up. It might be in a follow-up change if that's
> okay with you.
Actually I want to have tests on this CL. Once this CL lands, the orphaned page
logic starts running in production builds. It's a bit scary to land the
complicated logic without having tests.
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (right):
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:112: // out.
Probably it might be a good idea to add NoSafePointScope to ensure that we don't
enter any safe point scope while doing the thread-local GC. We can introduce
NoSafePointScope in a follow-up.
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:423: ASSERT(currentCount == 0);
ASSERT(!currentCount);
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:426: // collected in the last round of GC
above.
Why do we need the final GC? Isn't it already guaranteed that the last round of
the above GCs did not collect any persistent?
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:477: // pointer pointing to an orphaned
page.
This comment is a bit misleading. We can read it as if cross-thread pointers
pointing to orphaned pages are allowed. It's not allowed, as far as I
understand.
// We assume that orphaned pages have no objects reachable from persistent
handles on other threads or CrossThreadPersistents.
?
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.h (right):
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:243: bool isCleaningUp() { return
m_isCleaningUp; }
isCleaningUp => isTerminating ?
We don't want to mix "clean up", "shut down" and "terminate".
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:697: m_traced = false;
Shall we also clear m_gcInfo here?
It looks a bit inconsistent that GCInfos on object headers are already cleared
but GCInfos on page headers still remain.
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:701: void setShutdown() { m_shuttingDown =
true; }
m_shuttingDown => m_terminating ?
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:703: void setTraced() { m_traced = true; }
m_traced => m_tracedAfterShutdown or m_tracedAfterOrphaned ?
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.cpp (right):
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.cpp:47: ASSERT(page->orphaned() ||
FinalizedHeapObjectHeader::fromPayload(payload)->gcInfo() == gcInfo);
I'm not sure if we want to have the page->orphaned() check here. I agree that if
you call checkGCInfo() for an object on an orphaned page, the gcInfo of the
object is already cleared and thus gcInfo() == gcInfo fails. However, it seems
to me that it's already an insane situation that you're calling checkGCInfo()
for an object on an orphaned page.
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.h (right):
https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:604: if (heapPage->orphaned()) {
Just to confirm: Is this the only place where we have to insert the
heapPage->orphaned() check before dispatching mark/trace/adjustAndMark? For
example, don't we need to insert the check into Visitor::mark() as well?
Mostly looks good! https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:808: MutexLocker locker(m_mutex[index]); Just to confirm: Doesn't this need to be SafePointAwareMutexLocker? I think addFreePage() can be called from Heap::allocate() (which can be called from everywhere). https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:815: MutexLocker locker(m_mutex[index]); Ditto. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:879: page->markOrphaned(); Do we need to call markOrphaned()? I guess the page is already marked as orphaned. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:907: bool OrphanedPagePool::contains(void* object) Shall we add #ifndef NDEBUG ? https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:964: while (!pageMemory) { Let's add a comment why this needs to be 'while'. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1505: heapPage->setTraced(); We'll need the tracing flag only on orphaned pages. Can we change the condition to: if (Mode == GlobalGC && heapPage->orphaned()) ? Slightly better: // Comment RELEASE_ASSERT(!heapPage->orphaned() || Heap::lastGCWasConservative()); // Comment if (Mode == ThreadLocalGC && !heapPage->shuttingDown()) return true; // Comment if (Mode == GlobalGC && heapPage->orphaned()) { heapPage->setTraced(); retun true; } https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1559: heapPage->setTraced(); Ditto. We might want to restructure the branches as suggested above. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2068: traceRootsAndPerformGlobalWeakProcessing<ThreadLocalGC>(); traceRootsAndPerform"Global"WeakProcessing sounds a bit confusing with a global GC. traceRootsAndPerformBuiltInWeakProcessing ? https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:810: virtual void prepareHeapForShutdown() = 0; prepareForHeapForTermination ? We want to avoid mixing "shut down" and "terminate.
Code looks good to me. I agree with Haraken that writing a couple of regression tests that crashed before and are fixed with this patch would be great! https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:583: pages.append(page); I think we might as well just add the pages directly to the orphanedPagePool here instead of first building up a temporary vector backing of all the pointers and then adding them all to the pool? https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:771: // NOTE: large objects are not moved to the memory pool as it is unlikely memory pool -> free page pool.
Thanks again for the thorough reviews. I will update a diff with the changes and also rebase to be able to start some tests. I will make sure to add some heapTests before committing as well. https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/371623002/diff/40001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:2: * Copyright (C) 2013 Google Inc. All rights reserved. On 2014/07/09 05:17:48, haraken wrote: > On 2014/07/08 13:39:47, wibling-chromium wrote: > > On 2014/07/08 05:44:51, haraken wrote: > > > > > > I understand that it's a pain to add tests, but we might want to have tests > > for > > > this change because a lot of non-trivial things happen around the orphaned > > > pages. > > > > I will try to cook something up. It might be in a follow-up change if that's > > okay with you. > > Actually I want to have tests on this CL. Once this CL lands, the orphaned page > logic starts running in production builds. It's a bit scary to land the > complicated logic without having tests. Okay. That said we do have all the layout tests which should exercise this. To verify I also changed WorkerThreadableWebSocketChannel::Peer::destroy to not clear the keepAlive and verified both in debugger and without that the code is working. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:583: pages.append(page); On 2014/07/09 09:31:40, Mads Ager (chromium) wrote: > I think we might as well just add the pages directly to the orphanedPagePool > here instead of first building up a temporary vector backing of all the pointers > and then adding them all to the pool? Done. I previously had a lock on the orphanedPagePool however that is now gone so will get rid of addOrphanedPages. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:771: // NOTE: large objects are not moved to the memory pool as it is unlikely On 2014/07/09 09:31:40, Mads Ager (chromium) wrote: > memory pool -> free page pool. Done. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:808: MutexLocker locker(m_mutex[index]); On 2014/07/09 08:01:59, haraken wrote: > > Just to confirm: Doesn't this need to be SafePointAwareMutexLocker? I think > addFreePage() can be called from Heap::allocate() (which can be called from > everywhere). > No, we are not at a safepoint here since this can be called via destructors, during sweep, etc. So the heap is not in a consistent state at this point. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:879: page->markOrphaned(); On 2014/07/09 08:01:59, haraken wrote: > > Do we need to call markOrphaned()? I guess the page is already marked as > orphaned. Yes, we need to call it to clear the trace bits, both on the page and on the objects. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:907: bool OrphanedPagePool::contains(void* object) On 2014/07/09 08:01:59, haraken wrote: > > Shall we add #ifndef NDEBUG ? Done. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:964: while (!pageMemory) { On 2014/07/09 08:01:59, haraken wrote: > > Let's add a comment why this needs to be 'while'. Done. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1505: heapPage->setTraced(); On 2014/07/09 08:01:59, haraken wrote: > > We'll need the tracing flag only on orphaned pages. Can we change the condition > to: > > if (Mode == GlobalGC && heapPage->orphaned()) > > ? > > Slightly better: > > // Comment > RELEASE_ASSERT(!heapPage->orphaned() || Heap::lastGCWasConservative()); > > // Comment > if (Mode == ThreadLocalGC && !heapPage->shuttingDown()) > return true; > > // Comment > if (Mode == GlobalGC && heapPage->orphaned()) { > heapPage->setTraced(); > retun true; > } We also need to ignore the traceCallback when doing a thread local GC for orphaned pages. Ie. the case where Mode == ThreadLocalGC && heapPage->orphaned(). This can happen when a previous ThreadLocal GC add an empty page to the orphaned page pool. We could change it to: if (Mode == GlobalGC && heapPage->orphaned()) { heapPage->setTraced(); return true; } if (Mode == ThreadLocalGC && (heapPage->orphaned || !heapPage->shuttingDown())) return true; I would also prefer to keep the release assert under the ifs since we don't want to do additional checks in release mode if not needed. I am not confident the above is better, but it might be slightly simpler to reason about. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2068: traceRootsAndPerformGlobalWeakProcessing<ThreadLocalGC>(); On 2014/07/09 08:01:59, haraken wrote: > > traceRootsAndPerform"Global"WeakProcessing sounds a bit confusing with a global > GC. traceRootsAndPerformBuiltInWeakProcessing ? I agree GlobalWeakProcessing is a bit confusing, but I actually prefer it over BuiltInWeakProcessing since BuiltIn is not a term we have used elsewhere to describe the global weak processing. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:810: virtual void prepareHeapForShutdown() = 0; On 2014/07/09 08:01:59, haraken wrote: > > prepareForHeapForTermination ? We want to avoid mixing "shut down" and > "terminate. Done. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:112: // out. On 2014/07/09 05:17:48, haraken wrote: > > Probably it might be a good idea to add NoSafePointScope to ensure that we don't > enter any safe point scope while doing the thread-local GC. We can introduce > NoSafePointScope in a follow-up. We already have an ASSERT ensuring that we cannot enter a safepoint while m_sweepInProgress == true, see SafePointBarrier::enterSafepoint. I think that is the only part of the ThreadLocal GC where we have a risk. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:423: ASSERT(currentCount == 0); On 2014/07/09 05:17:48, haraken wrote: > > ASSERT(!currentCount); Done. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:426: // collected in the last round of GC above. On 2014/07/09 05:17:48, haraken wrote: > > Why do we need the final GC? Isn't it already guaranteed that the last round of > the above GCs did not collect any persistent? We need it since when the count of persistents reach zero we still need to sweep and finalize the objects the persistents previously pointed to. We also need it in the case where there are no persistents to ensure we at least sweep and finalize the heap once. I will change it to: int oldCount = -1; int currentCount = anchor->numberOfPersistents(); while (currentCount != oldCount) { Heap::CollectGarbageForTerminatingThread(this); oldCount = currentCount; currentCount = anchor->numberOfPersistents(); } which should give the same guarantees without having an extra GC after the loop. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:477: // pointer pointing to an orphaned page. On 2014/07/09 05:17:48, haraken wrote: > > This comment is a bit misleading. We can read it as if cross-thread pointers > pointing to orphaned pages are allowed. It's not allowed, as far as I > understand. > > // We assume that orphaned pages have no objects reachable from persistent > handles on other threads or CrossThreadPersistents. > > ? Done. Rephrased it a bit. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:243: bool isCleaningUp() { return m_isCleaningUp; } On 2014/07/09 05:17:49, haraken wrote: > > isCleaningUp => isTerminating ? > > We don't want to mix "clean up", "shut down" and "terminate". Done. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:697: m_traced = false; On 2014/07/09 05:17:48, haraken wrote: > > Shall we also clear m_gcInfo here? > > It looks a bit inconsistent that GCInfos on object headers are already cleared > but GCInfos on page headers still remain. Sure, I don't have a strong feeling either way. That does require us to also add the orphaned page check to the other checkGCInfo. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:701: void setShutdown() { m_shuttingDown = true; } On 2014/07/09 05:17:48, haraken wrote: > > m_shuttingDown => m_terminating ? Done. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:703: void setTraced() { m_traced = true; } On 2014/07/09 05:17:48, haraken wrote: > > m_traced => m_tracedAfterShutdown or m_tracedAfterOrphaned ? Done. Changed it to m_tracedAfterOrphaned. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.cpp (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.cpp:47: ASSERT(page->orphaned() || FinalizedHeapObjectHeader::fromPayload(payload)->gcInfo() == gcInfo); On 2014/07/09 05:17:49, haraken wrote: > > I'm not sure if we want to have the page->orphaned() check here. I agree that if > you call checkGCInfo() for an object on an orphaned page, the gcInfo of the > object is already cleared and thus gcInfo() == gcInfo fails. However, it seems > to me that it's already an insane situation that you're calling checkGCInfo() > for an object on an orphaned page. We need to have it here to avoid having to do any release build checks when parking an object on orphaned page. We have chosen to support marking objects (ie. setting the mark bit) for object on orphaned pages to avoid having to do the if orphanedPage check at every mark. Instead we check when we pop the trace callback off the marking stack and just avoid calling it at that point. The mark bits on the orphaned pages are then cleared (rezapped) in the decommitOrphanedPages method if the orphaned page was traced. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:604: if (heapPage->orphaned()) { On 2014/07/09 05:17:49, haraken wrote: > > Just to confirm: Is this the only place where we have to insert the > heapPage->orphaned() check before dispatching mark/trace/adjustAndMark? For > example, don't we need to insert the check into Visitor::mark() as well? AFAICT we can get away with just checking here for the marking code. We ideally don't want to put the check into the code where we are marking/pushing the trace methods onto the marking stack. The only reason we add it here is because we are calling a method on memory that is orphaned and hence zapped. Instead we are checking whether we hit an orphaned page when we pop the trace callback off the marking stack since this means we only have to check once for each object and it is done locally.
FYI. I am looking into an issue with tracing of large collections with value object. I will wait with the new diff until I have a solution for this as well.
I just want to understand, but is there any reason that makes it hard to write tests for orphaned pages? (I understand it's a pain though :-)
One more comment. This is tricky to get right, but I do think we are getting there. :) https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:450: // point all the thread's pages are added to the orphanedPagePool. This comment says that the pages are added to the orphanedPagePool at this point. Do we set the orphaned bit at this point as well? If we do, I think that is too late. We could get a normal GC at the SafePointAwareMutexLocker just above. The pages for this thread are now in an inconsistent state for tracing and therefore we need to make sure that the orphaned bit is set on all the pages at the end of cleanup and before we can get a normal GC so that normal GC will not perform tracing in the pages.
On 2014/07/09 16:02:09, haraken wrote: > I just want to understand, but is there any reason that makes it hard to write > tests for orphaned pages? (I understand it's a pain though :-) No, I just had to spent some time thinking about how to do it. I have updated the diff with the tests and various other improvements as of now.
On 2014/07/11 05:48:06, Mads Ager (chromium) wrote: > One more comment. This is tricky to get right, but I do think we are getting > there. :) > > https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... > File Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Thr... > Source/platform/heap/ThreadState.cpp:450: // point all the thread's pages are > added to the orphanedPagePool. > This comment says that the pages are added to the orphanedPagePool at this > point. Do we set the orphaned bit at this point as well? If we do, I think that > is too late. We could get a normal GC at the SafePointAwareMutexLocker just > above. The pages for this thread are now in an inconsistent state for tracing > and therefore we need to make sure that the orphaned bit is set on all the pages > at the end of cleanup and before we can get a normal GC so that normal GC will > not perform tracing in the pages. This should be fixed in the latest diff. Please take a look.
PTAL. I have incorporated all of the feedback, added tests for PersistentHeapCollection with weakness, cross thread pointer to orphaned page, and object dead bit. I have also removed the check for orphaned pages from the ephemeron stack since it is not needed (I have added a comment describing why). Finally I have changed the order in which we add pages to the orphaned page pool at thread shut down to address Mads' latest comment.
LGTM https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2008: // XXX: Add slow case check for not pushing an orphaned page Remove comment? https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:4815: static volatile bool s_workerRunning; I guess we should either use atomic ops here or have mutex protection when updating? Since the tests are not performance critical I would just add mutex protection for the access to these variables?
LGTM https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:963: // newly allocated page memory before this thread call takeFreePage. s/call/calls https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2008: // XXX: Add slow case check for not pushing an orphaned page s/XXX/FIXME https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2119: // on another thread's heap. The addition is dependent on Mode, so maybe add: If Mode is ThreadLocal then this will not continue ... https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:313: inline bool hasDeadMark() const; Nit: I still think we should be using a consistent convention for marking/unmarking bits, but won't press it further. https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:472: void clearLiveAndMarkDead(); For consistency this should be: unmarkAndSetDeadMark() (Unless the others are renamed to something else). https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:807: virtual void clearLiveAndMarkDead() = 0; ditto https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:846: virtual void clearLiveAndMarkDead(); ditto
Thanks for the reviews. I have updated the diffs with the latest feedback. https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:963: // newly allocated page memory before this thread call takeFreePage. On 2014/07/11 11:24:42, zerny-chromium wrote: > s/call/calls Done. https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2008: // XXX: Add slow case check for not pushing an orphaned page On 2014/07/11 10:59:34, Mads Ager (chromium) wrote: > Remove comment? Done. https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2008: // XXX: Add slow case check for not pushing an orphaned page On 2014/07/11 11:24:42, zerny-chromium wrote: > s/XXX/FIXME It was already done as part of the above assert. https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2119: // on another thread's heap. On 2014/07/11 11:24:42, zerny-chromium wrote: > The addition is dependent on Mode, so maybe add: If Mode is ThreadLocal then > this will not continue ... Done. https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Heap.h File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/Heap.h:313: inline bool hasDeadMark() const; On 2014/07/11 11:24:42, zerny-chromium wrote: > Nit: I still think we should be using a consistent convention for > marking/unmarking bits, but won't press it further. I agree and I will do this, but I would prefer to wait for a subsequent change to avoid additional churn on this one. https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/371623002/diff/80001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:4815: static volatile bool s_workerRunning; On 2014/07/11 10:59:34, Mads Ager (chromium) wrote: > I guess we should either use atomic ops here or have mutex protection when > updating? Since the tests are not performance critical I would just add mutex > protection for the access to these variables? Done.
LGTM! https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:604: if (heapPage->orphaned()) { On 2014/07/09 10:32:31, wibling-chromium wrote: > On 2014/07/09 05:17:49, haraken wrote: > > > > Just to confirm: Is this the only place where we have to insert the > > heapPage->orphaned() check before dispatching mark/trace/adjustAndMark? For > > example, don't we need to insert the check into Visitor::mark() as well? > > AFAICT we can get away with just checking here for the marking code. > > We ideally don't want to put the check into the code where we are > marking/pushing the trace methods onto the marking stack. The only reason we add > it here is because we are calling a method on memory that is orphaned and hence > zapped. > Instead we are checking whether we hit an orphaned page when we pop the trace > callback off the marking stack since this means we only have to check once for > each object and it is done locally. Just to help me understand, imagine the following object graph: root object => object X on an orphaned page A => object Y on a not-orphaned page => object Z on an orphaned page B Do we want to continue tracing from the object X to the object Y? Or do we want to stop the tracing when we reach the object X? (In other words, do we want to set an tracedAfterOrphaned flag on the page B or not?) https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:776: } else { Can we add ASSERT(!ThreadState::current()->isTerminating()) ? https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:949: } else { Can we add ASSERT(!ThreadState::current()->isTerminating()) ? https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1510: // on other thread's heaps, ie. pages where the shuttingDown flag is not shuttingDown flag => terminating flag https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1514: // When doing a GC we should only get a trace callback to an orphaned a GC => a global GC https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1524: // For WeaknessProcessing we should never reach an orphaned pages since an orphaned pages => orphaned pages https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2001: Nit: Unnecessary empty line. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.h:696: void addFreePage(int index, PageMemory*); Nit: Remove |index|. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.h:697: PageMemory* takeFreePage(int index); Ditto. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:576: if (m_isTerminating) Is this possible that we hit this branch? When a thread is terminating, we don't trigger conservative scanning and thus won't call checkAndMarkPointer, as far as I know. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:767: heap->prepareHeapForTermination(); Nit: m_heaps[i]->prepareHeapForTermination();
Thanks for the reviews! I have updated the diff and will start the commit. https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/371623002/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:604: if (heapPage->orphaned()) { On 2014/07/13 17:30:38, haraken wrote: > On 2014/07/09 10:32:31, wibling-chromium wrote: > > On 2014/07/09 05:17:49, haraken wrote: > > > > > > Just to confirm: Is this the only place where we have to insert the > > > heapPage->orphaned() check before dispatching mark/trace/adjustAndMark? For > > > example, don't we need to insert the check into Visitor::mark() as well? > > > > AFAICT we can get away with just checking here for the marking code. > > > > We ideally don't want to put the check into the code where we are > > marking/pushing the trace methods onto the marking stack. The only reason we > add > > it here is because we are calling a method on memory that is orphaned and > hence > > zapped. > > Instead we are checking whether we hit an orphaned page when we pop the trace > > callback off the marking stack since this means we only have to check once for > > each object and it is done locally. > > Just to help me understand, imagine the following object graph: > > root object => object X on an orphaned page A => object Y on a not-orphaned > page => object Z on an orphaned page B > > Do we want to continue tracing from the object X to the object Y? Or do we want > to stop the tracing when we reach the object X? (In other words, do we want to > set an tracedAfterOrphaned flag on the page B or not?) We don't want to continue tracing from X to Y. If Y is not reachable from other objects than X then there is no need to set the traced bit on Z (ie. page B) since it cannot be reached. Even in the case where Y is not swept and we do a follow-up conservative GC the dead bit should be set on Y meaning we won't continue tracing its pointers to Z. This guarantees that we are not trying to trace through to a, now, decommited page. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:776: } else { On 2014/07/13 17:30:39, haraken wrote: > > Can we add ASSERT(!ThreadState::current()->isTerminating()) ? Done. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:949: } else { On 2014/07/13 17:30:39, haraken wrote: > > Can we add ASSERT(!ThreadState::current()->isTerminating()) ? Done. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1510: // on other thread's heaps, ie. pages where the shuttingDown flag is not On 2014/07/13 17:30:38, haraken wrote: > > shuttingDown flag => terminating flag Done. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1514: // When doing a GC we should only get a trace callback to an orphaned On 2014/07/13 17:30:38, haraken wrote: > > a GC => a global GC Done. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1524: // For WeaknessProcessing we should never reach an orphaned pages since On 2014/07/13 17:30:38, haraken wrote: > > an orphaned pages => orphaned pages Done. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2001: On 2014/07/13 17:30:38, haraken wrote: > > Nit: Unnecessary empty line. Removed. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.h:696: void addFreePage(int index, PageMemory*); On 2014/07/13 17:30:39, haraken wrote: > > Nit: Remove |index|. Done. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/He... Source/platform/heap/Heap.h:697: PageMemory* takeFreePage(int index); On 2014/07/13 17:30:39, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:576: if (m_isTerminating) On 2014/07/13 17:30:39, haraken wrote: > > Is this possible that we hit this branch? When a thread is terminating, we don't > trigger conservative scanning and thus won't call checkAndMarkPointer, as far as > I know. This can happen in the case where we are waiting for the threadAttachMutex in cleanup with the safepoint aware locker. In that case we have already set the flag and there could be a conservatively found object that points to the terminating threads heap pages. https://codereview.chromium.org/371623002/diff/140001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:767: heap->prepareHeapForTermination(); On 2014/07/13 17:30:39, haraken wrote: > > Nit: m_heaps[i]->prepareHeapForTermination(); Done.
The CQ bit was checked by wibling@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wibling@chromium.org/371623002/160001
The CQ bit was checked by wibling@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wibling@chromium.org/371623002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
The CQ bit was checked by wibling@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wibling@chromium.org/371623002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16710)
Message was sent while issue was closed.
Change committed as 178044 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
