Skip to content

Commit

Permalink
Revert of Revert "Revert of [oilpan]: Change marking to do precise ro…
Browse files Browse the repository at this point in the history 
…ots first and conservative second. (http… (patchset #2 of https://codereview.chromium.org/413133006/)

Reason for revert:
This breaks strongification of ephemerons when there are pointers on the stack. See:

https://codereview.chromium.org/461413002/

for details.


Original issue's description:
> Revert "Revert of [oilpan]: Change marking to do precise roots first and conservative second. (https://codereview.chromium.org/405403003/)"
> 
> This reverts commit 765af3a.
> 
> AFAICT the CrossThreadPointerToOrphanedPage test was a bit flaky in the sense that the compiler was free to reuse the slot where the stackPtrValue was located after the RELEASE_ASSERT. However the test assumed the stackPtrValue was on the stack (to mimic a rogue integer value finding a dead object) when doing the conservative GC further down.
> After my change to Heap::CollectGarbage it seemed the android compiler no longer kept the value around and hence the test started failing. I have now moved the RELEASE_ASSERT down to after the point where we do the conservative GC which makes the test pass on android again.
> 
> I uploaded the unfixed diff first and then a second diff (moved RELEASE_ASSERT) with the fix.
> 
> R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, sigbjornf@opera.com, tkent@chromium.org, zerny@chromium.org
> BUG=
> 
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178845

NOTRY=true
NOTREECHECKS=true

Review URL: https://codereview.chromium.org/464253002

git-svn-id: svn://svn.chromium.org/blink/trunk@180160 bbb929c8-8fbe-4397-9dbb-9b2b20218538
  • Loading branch information
ager@chromium.org committed Aug 13, 2014
1 parent c374ef0 commit 8cfae43
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 65 deletions.
53 changes: 11 additions & 42 deletions Source/platform/heap/Heap.cpp
View file
Expand Up @@ -2082,23 +2082,7 @@ void Heap::collectGarbage(ThreadState::StackState stackState)

prepareForGC();

// 1. trace persistent roots.
ThreadState::visitPersistentRoots(s_markingVisitor);

// 2. trace objects reachable from the persistent roots including ephemerons.
processMarkingStack<GlobalMarking>();

// 3. trace objects reachable from the stack. We do this independent of the
// given stackState since other threads might have a different stack state.
ThreadState::visitStackRoots(s_markingVisitor);

// 4. trace objects reachable from the stack "roots" including ephemerons.
// Only do the processing if we found a pointer to an object on one of the
// thread stacks.
if (lastGCWasConservative())
processMarkingStack<GlobalMarking>();

globalWeakProcessingAndCleanup();
traceRootsAndPerformGlobalWeakProcessing<GlobalMarking>();

// After a global marking we know that any orphaned page that was not reached
// cannot be reached in a subsequent GC. This is due to a thread either having
Expand Down Expand Up @@ -2138,38 +2122,26 @@ void Heap::collectGarbageForTerminatingThread(ThreadState* state)
state->enterGC();
state->prepareForGC();

// 1. trace the thread local persistent roots. For thread local GCs we
// don't trace the stack (ie. no conservative scanning) since this is
// only called during thread shutdown where there should be no objects
// on the stack.
// We also assume that orphaned pages have no objects reachable from
// persistent handles on other threads or CrossThreadPersistents. The
// only cases where this could happen is if a subsequent conservative
// global GC finds a "pointer" on the stack or due to a programming
// error where an object has a dangling cross-thread pointer to an
// object on this heap.
state->visitPersistents(s_markingVisitor);

// 2. trace objects reachable from the thread's persistent roots
// including ephemerons.
processMarkingStack<ThreadLocalMarking>();

globalWeakProcessingAndCleanup();
traceRootsAndPerformGlobalWeakProcessing<ThreadLocalMarking>();

state->leaveGC();
}
state->performPendingSweep();
}

template<CallbackInvocationMode Mode>
void Heap::processMarkingStack()
void Heap::traceRootsAndPerformGlobalWeakProcessing()
{
if (Mode == ThreadLocalMarking)
ThreadState::current()->visitLocalRoots(s_markingVisitor);
else
ThreadState::visitRoots(s_markingVisitor);

// Ephemeron fixed point loop.
do {
// Iteratively mark all objects that are reachable from the objects
// currently pushed onto the marking stack. If Mode is ThreadLocalMarking
// don't continue tracing if the trace hits an object on another thread's
// heap.
// Recursively mark all objects that are reachable from the roots for
// this thread. If Mode is ThreadLocalMarking don't continue tracing if
// the trace hits an object on another thread's heap.
while (popAndInvokeTraceCallback<Mode>(s_markingVisitor)) { }

// Mark any strong pointers that have now become reachable in ephemeron
Expand All @@ -2178,10 +2150,7 @@ void Heap::processMarkingStack()

// Rerun loop if ephemeron processing queued more objects for tracing.
} while (!s_markingStack->isEmpty());
}

void Heap::globalWeakProcessingAndCleanup()
{
// Call weak callbacks on objects that may now be pointing to dead
// objects and call ephemeronIterationDone callbacks on weak tables
// to do cleanup (specifically clear the queued bits for weak hash
Expand Down
3 changes: 1 addition & 2 deletions Source/platform/heap/Heap.h
View file
Expand Up @@ -969,8 +969,7 @@ class PLATFORM_EXPORT Heap {
static void collectGarbage(ThreadState::StackState);
static void collectGarbageForTerminatingThread(ThreadState*);
static void collectAllGarbage();
template<CallbackInvocationMode Mode> static void processMarkingStack();
static void globalWeakProcessingAndCleanup();
template<CallbackInvocationMode Mode> static void traceRootsAndPerformGlobalWeakProcessing();
static void setForcePreciseGCForTesting();

static void prepareForGC();
Expand Down
9 changes: 2 additions & 7 deletions Source/platform/heap/HeapTest.cpp
View file
Expand Up @@ -4654,6 +4654,8 @@ class CrossThreadPointerTester {
// shutting it down.
stackPtrValue = reinterpret_cast<uintptr_t>(cto.get());
}
RELEASE_ASSERT(stackPtrValue);

// At this point it is "programatically" okay to shut down the worker thread
// since the cto object should be dead. However out stackPtrValue will cause a
// trace of the object when doing a conservative GC.
Expand All @@ -4677,13 +4679,6 @@ class CrossThreadPointerTester {
EXPECT_EQ(0, CrossThreadObject::s_destructorCalls);
EXPECT_EQ(1, IntWrapper::s_destructorCalls);

// This release assert is here to ensure the stackValuePtr is not
// optimized away before doing the above conservative GC. If the
// EXPECT_EQ(0, CrossThreadObject::s_destructorCalls) call above
// starts failing it means we have to find a better way to ensure
// the stackPtrValue is not optimized away.
RELEASE_ASSERT(stackPtrValue);

// Do a GC with no pointers on the stack to see the cto being collected.
Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
EXPECT_EQ(1, CrossThreadObject::s_destructorCalls);
Expand Down
25 changes: 16 additions & 9 deletions Source/platform/heap/ThreadState.cpp
View file
Expand Up @@ -447,7 +447,7 @@ void ThreadState::detach()
shutdownHeapIfNecessary();
}

void ThreadState::visitPersistentRoots(Visitor* visitor)
void ThreadState::visitRoots(Visitor* visitor)
{
{
// All threads are at safepoints so this is not strictly necessary.
Expand All @@ -459,14 +459,17 @@ void ThreadState::visitPersistentRoots(Visitor* visitor)

AttachedThreadStateSet& threads = attachedThreads();
for (AttachedThreadStateSet::iterator it = threads.begin(), end = threads.end(); it != end; ++it)
(*it)->visitPersistents(visitor);
(*it)->trace(visitor);
}

void ThreadState::visitStackRoots(Visitor* visitor)
void ThreadState::visitLocalRoots(Visitor* visitor)
{
AttachedThreadStateSet& threads = attachedThreads();
for (AttachedThreadStateSet::iterator it = threads.begin(), end = threads.end(); it != end; ++it)
(*it)->visitStack(visitor);
// We assume that orphaned pages have no objects reachable from persistent
// handles on other threads or CrossThreadPersistents. The only cases where
// this could happen is if a global conservative GC finds a "pointer" on
// the stack or due to a programming error where an object has a dangling
// cross-thread pointer to an object on this heap.
m_persistents->trace(visitor);
}

NO_SANITIZE_ADDRESS
Expand Down Expand Up @@ -500,9 +503,6 @@ void ThreadState::visitAsanFakeStackForPointer(Visitor* visitor, Address ptr)
NO_SANITIZE_ADDRESS
void ThreadState::visitStack(Visitor* visitor)
{
if (m_stackState == NoHeapPointersOnStack)
return;

Address* start = reinterpret_cast<Address*>(m_startOfStack);
// If there is a safepoint scope marker we should stop the stack
// scanning there to not touch active parts of the stack. Anything
Expand Down Expand Up @@ -547,6 +547,13 @@ void ThreadState::visitPersistents(Visitor* visitor)
m_persistents->trace(visitor);
}

void ThreadState::trace(Visitor* visitor)
{
if (m_stackState == HeapPointersOnStack)
visitStack(visitor);
visitPersistents(visitor);
}

bool ThreadState::checkAndMarkPointer(Visitor* visitor, Address address)
{
// If thread is terminating ignore conservative pointers.
Expand Down
10 changes: 5 additions & 5 deletions Source/platform/heap/ThreadState.h
View file
Expand Up @@ -242,11 +242,8 @@ class PLATFORM_EXPORT ThreadState {
static void attachMainThread();
static void detachMainThread();

// Trace all persistent roots, called when marking the managed heap objects.
static void visitPersistentRoots(Visitor*);

// Trace all objects found on the stack, used when doing conservative GCs.
static void visitStackRoots(Visitor*);
// Trace all GC roots, called when marking the managed heap objects.
static void visitRoots(Visitor*);

// Associate ThreadState object with the current thread. After this
// call thread can start using the garbage collected heap infrastructure.
Expand Down Expand Up @@ -512,6 +509,7 @@ class PLATFORM_EXPORT ThreadState {
HeapStats& statsAfterLastGC() { return m_statsAfterLastGC; }

void setupHeapsForTermination();
void visitLocalRoots(Visitor*);

private:
explicit ThreadState();
Expand Down Expand Up @@ -564,6 +562,8 @@ class PLATFORM_EXPORT ThreadState {
// and lazily construct ThreadState in it using placement new.
static uint8_t s_mainThreadStateStorage[];

void trace(Visitor*);

ThreadIdentifier m_thread;
OwnPtr<PersistentNode> m_persistents;
StackState m_stackState;
Expand Down

0 comments on commit 8cfae43

Please sign in to comment.