ASSERT that Visitor::mark is only called when inGC()

ASSERT that Visitor::mark is only called when inGC()

This CL adds an ASSERT to check that Visitor::mark is only called when
all threads are parked and the gc state is at ThreadState::inGC().

The current exception to this is ephemeron weak processing, where it call
trace() to see if an element is dead. This should be refactored in future
so that it would check without calling trace(), but it is left as
an exception as of now.

BUG=210454
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188420

[kouhei (in TOK), 2015-01-13 06:29:27]

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

[haraken, 2015-01-13 07:55:53]

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

[haraken, 2015-01-13 07:55:54]

https://codereview.chromium.org/824513004/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/824513004/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2031: virtual void checkIsMarkingAllowed()
override

It sounds inconsistent to have check"IsMarking"Allowed and
m_allow"Trace"ForHashTableWeakProcessing.

checkMarkingAllowed and m_setAllowMarking ?

https://codereview.chromium.org/824513004/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2038: m_allowTraceForHashTableWeakProcessing =
true;

Shouldn't this be '= allow' ?

https://codereview.chromium.org/824513004/diff/1/Source/wtf/HashTable.h
File Source/wtf/HashTable.h (right):

https://codereview.chromium.org/824513004/diff/1/Source/wtf/HashTable.h#newco...
Source/wtf/HashTable.h:1260:
visitor->setAllowTraceForHashTableWeakProcessing(true);

Add a FIXME to remove this.

BTW, do you really need to special-case the following trace()? I guess that the
following trace does not call mark() at all, and thus we don't need to set the
AllowTraceForHashTableWeakProcessing flag.

[kouhei (in TOK), 2015-01-14 02:10:05]

https://codereview.chromium.org/824513004/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/824513004/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2031: virtual void checkIsMarkingAllowed()
override
On 2015/01/13 07:55:53, haraken wrote:
> 
> It sounds inconsistent to have check"IsMarking"Allowed and
> m_allow"Trace"ForHashTableWeakProcessing.
> 
> checkMarkingAllowed and m_setAllowMarking ?

Done.

https://codereview.chromium.org/824513004/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2038: m_allowTraceForHashTableWeakProcessing =
true;
On 2015/01/13 07:55:53, haraken wrote:
> 
> Shouldn't this be '= allow' ?

Done.

https://codereview.chromium.org/824513004/diff/1/Source/wtf/HashTable.h
File Source/wtf/HashTable.h (right):

https://codereview.chromium.org/824513004/diff/1/Source/wtf/HashTable.h#newco...
Source/wtf/HashTable.h:1260:
visitor->setAllowTraceForHashTableWeakProcessing(true);
On 2015/01/13 07:55:53, haraken wrote:
> 
> Add a FIXME to remove this.
Done.

> BTW, do you really need to special-case the following trace()? I guess that
the
> following trace does not call mark() at all, and thus we don't need to set the
> AllowTraceForHashTableWeakProcessing flag.
This trace does call mark(). I'll try to remove this hack in near future.

[haraken, 2015-01-14 02:50:43]

LGTM

https://codereview.chromium.org/824513004/diff/20001/Source/platform/heap/Inl...
File Source/platform/heap/InlinedGlobalMarkingVisitor.h (right):

https://codereview.chromium.org/824513004/diff/20001/Source/platform/heap/Inl...
Source/platform/heap/InlinedGlobalMarkingVisitor.h:84: inline void
checkMarkingAllowed() { getUninlined()->checkMarkingAllowed(); }

getUninlined() => m_visitor.

https://codereview.chromium.org/824513004/diff/20001/Source/platform/heap/Inl...
Source/platform/heap/InlinedGlobalMarkingVisitor.h:87:
getUninlined()->setAllowMarkingForHashTableWeakProcessing(allow);

getUninlined() => m_visitor.

[kouhei (in TOK), 2015-01-14 02:59:32]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-01-14 02:59:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/824513004/40001

[kouhei (in TOK), 2015-01-14 02:59:51]

The CQ bit was unchecked by kouhei@chromium.org

[kouhei (in TOK), 2015-01-14 03:00:56]

https://codereview.chromium.org/824513004/diff/20001/Source/platform/heap/Inl...
File Source/platform/heap/InlinedGlobalMarkingVisitor.h (right):

https://codereview.chromium.org/824513004/diff/20001/Source/platform/heap/Inl...
Source/platform/heap/InlinedGlobalMarkingVisitor.h:84: inline void
checkMarkingAllowed() { getUninlined()->checkMarkingAllowed(); }
On 2015/01/14 02:50:42, haraken wrote:
> 
> getUninlined() => m_visitor.

Done.

https://codereview.chromium.org/824513004/diff/20001/Source/platform/heap/Inl...
Source/platform/heap/InlinedGlobalMarkingVisitor.h:87:
getUninlined()->setAllowMarkingForHashTableWeakProcessing(allow);
On 2015/01/14 02:50:42, haraken wrote:
> 
> getUninlined() => m_visitor.

Done.

[kouhei (in TOK), 2015-01-14 03:01:18]

kouhei@chromium.org changed reviewers:
+ jochen@chromium.org

[kouhei (in TOK), 2015-01-14 03:01:18]

jochen: Would you review wtf/ changes? Thanks

[jochen (gone - plz use gerrit), 2015-01-14 13:21:35]

lgtm

[kouhei (in TOK), 2015-01-14 23:07:36]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-01-14 23:07:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/824513004/40001

[commit-bot: I haz the power, 2015-01-14 23:11:26]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188420