Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
765673004:
Oilpan: support eager tracing of objects when marking. (Closed)
DescriptionOilpan: support eager tracing of objects when marking.
This adds support for eager tracing of objects when marking. That is,
instead of pushing an unmarked object's trace callback onto a marker
stack for later processing, the trace callback is invoked there & then.
This may enable inlining of eager trace() methods at their call sites,
leading to potentially faster marking overall.
Eager tracing runs counter to the common approach of GC marking passes,
where an explicit mark stack is used. The marking phase pushes the
trace callbacks of unmarked objects onto this explicit stack, performing
instead an iterative deepening traversal of the object graph. This
ensures bounded system stack use, and any overflow of the marker stack
can be explicitly handled. The use of eager tracing must be carefully
considered with respect to the system stack use that it will bring:
uncontrollably deep object structures should not be eagerly traced as
that will risk overflowing the system stack at little or no gain. The
GC does not have a recovery or trapping mechanism against such overflows.
To eagerly call trace() over GCed objects at class type T, you declare
a specialization of the boolean-valued TraceEagerlyTrait<T> trait.
The default is currently not to perform any eager tracing.
R=kouhei,haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186355
Patch Set 1 #Patch Set 2 : Re-introduce TraceEagerly #
Total comments: 17
Patch Set 3 : Support defining traits for class hierarchies + remove TraceEagerly #Patch Set 4 : For eager tracing, check for cross-heap access #
Total comments: 22
Patch Set 5 : Parameterize MarkingVisitor over its marking mode/kind #Patch Set 6 : rebased #
Total comments: 13
Patch Set 7 : Tidying up #
Messages
Total messages: 30 (4 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look.
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2052: #if ENABLE(ASSERT) Would you describe why we need ENABLE(ASSERT) / no ENABLE(ASSERT) version? https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:420: template<typename T>void markNoTracing(const T* pointer) { mark(pointer, reinterpret_cast<TraceCallback>(0)); } insert space.
haraken@chromium.org changed reviewers: + haraken@chromium.org
Thanks for working on this! https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2033: virtual bool ensureMarked(const void* objectPointer) override Shall we flip true/false of the return value? It looks more natural to return true when ensureMarked succeeded in marking. https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2033: virtual bool ensureMarked(const void* objectPointer) override Can we share the implementation of ensureMarked with Heap::visitHeader()? Basically ensureMarked needs to do the same thing as Heap::visitHeader(), except that it returns true/false instead of calling pushTraceCallback(). https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:5262: #if ENABLE(OILPAN) Do we need #if ENABLE(OILPAN)? https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:600: TraceTrait<T>::trace(visitor, const_cast<T*>(t)); Just recursively tracing an object is not enough for correctness. Here we need the same logic as Heap::popAndInvokeTraceCallback() (e.g., checking page->terminating etc). It would be great if we could share the implementation with Heap::popAndInvokeTraceCallback().
https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2033: virtual bool ensureMarked(const void* objectPointer) override On 2014/11/28 05:20:05, haraken wrote: > > Shall we flip true/false of the return value? It looks more natural to return > true when ensureMarked succeeded in marking. Meaning is still not self-evident, but flipped. https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2033: virtual bool ensureMarked(const void* objectPointer) override On 2014/11/28 05:20:05, haraken wrote: > > Can we share the implementation of ensureMarked with Heap::visitHeader()? > > Basically ensureMarked needs to do the same thing as Heap::visitHeader(), except > that it returns true/false instead of calling pushTraceCallback(). I would prefer not to do that exercise; there's trivial amounts of logic involved, and if you somehow generalize visitHeader() (have it return a bool result), you risk slowing down your inner loop when marking by shuffling return values into registers (or onto the stack) for no good reason, as it is ignored most of the time. https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2052: #if ENABLE(ASSERT) On 2014/11/28 01:00:57, kouhei wrote: > Would you describe why we need ENABLE(ASSERT) / no ENABLE(ASSERT) version? A debug-specific version is provided essentially because of the asserts (and GC profile marking support) that visitHeader() performs, which markNoTracing() bottoms out in. 3-4 method calls are reqd to get to visitHeader(), so I didn't want to risk any inlining machinery pointlessly getting in the way in a release build. https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:5262: #if ENABLE(OILPAN) On 2014/11/28 05:20:05, haraken wrote: > > Do we need #if ENABLE(OILPAN)? With these eager-tracing macros expanding to nothing with !ENABLE(OILPAN), it doesn't make much sense to test what's below. However, should we want use eager-tracing before ENABLE(OILPAN) becomes the default, it makes sense to also define the eager-tracing macros for !ENABLE(OILPAN). Done so now + universally enabled this test.
https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2033: virtual bool ensureMarked(const void* objectPointer) override On 2014/11/28 12:05:08, sof wrote: > On 2014/11/28 05:20:05, haraken wrote: > > > > Can we share the implementation of ensureMarked with Heap::visitHeader()? > > > > Basically ensureMarked needs to do the same thing as Heap::visitHeader(), > except > > that it returns true/false instead of calling pushTraceCallback(). > > I would prefer not to do that exercise; there's trivial amounts of logic > involved, and if you somehow generalize visitHeader() (have it return a bool > result), you risk slowing down your inner loop when marking by shuffling return > values into registers (or onto the stack) for no good reason, as it is ignored > most of the time. Sounds reasonable. Given that the code that ensureMarked is missing against visitHeader is just code for heap profiling, it would be OK to omit it at the moment. https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:600: TraceTrait<T>::trace(visitor, const_cast<T*>(t)); On 2014/11/28 05:20:05, haraken wrote: > > Just recursively tracing an object is not enough for correctness. Here we need > the same logic as Heap::popAndInvokeTraceCallback() (e.g., checking > page->terminating etc). It would be great if we could share the implementation > with Heap::popAndInvokeTraceCallback(). Are you going to address this one? Or is it not a concern?
https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:600: TraceTrait<T>::trace(visitor, const_cast<T*>(t)); On 2014/11/28 12:39:27, haraken wrote: > On 2014/11/28 05:20:05, haraken wrote: > > > > Just recursively tracing an object is not enough for correctness. Here we need > > the same logic as Heap::popAndInvokeTraceCallback() (e.g., checking > > page->terminating etc). It would be great if we could share the implementation > > with Heap::popAndInvokeTraceCallback(). > > Are you going to address this one? Or is it not a concern? I need to make progress on other fronts right now. But it is a concern & I will add the required cross-heap bailout check (to ensureMarked, I think.)
https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:420: template<typename T>void markNoTracing(const T* pointer) { mark(pointer, reinterpret_cast<TraceCallback>(0)); } On 2014/11/28 01:00:57, kouhei wrote: > insert space. Done. https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:600: TraceTrait<T>::trace(visitor, const_cast<T*>(t)); On 2014/11/28 12:44:27, sof wrote: > On 2014/11/28 12:39:27, haraken wrote: > > On 2014/11/28 05:20:05, haraken wrote: > > > > > > Just recursively tracing an object is not enough for correctness. Here we > need > > > the same logic as Heap::popAndInvokeTraceCallback() (e.g., checking > > > page->terminating etc). It would be great if we could share the > implementation > > > with Heap::popAndInvokeTraceCallback(). > > > > Are you going to address this one? Or is it not a concern? > > I need to make progress on other fronts right now. But it is a concern & I will > add the required cross-heap bailout check (to ensureMarked, I think.) Now addressed, but a gnarlier issue to get right. i.e., the trace-needed check has now been moved up before pushing the trace callback onto the marking stack. At the cost of having to check a flag -- I don't see a way around that without having a separate marking visitor for thread-local markings. Let me know what you think.
lgtm https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2052: #if ENABLE(ASSERT) On 2014/11/28 12:05:08, sof wrote: > On 2014/11/28 01:00:57, kouhei wrote: > > Would you describe why we need ENABLE(ASSERT) / no ENABLE(ASSERT) version? > > A debug-specific version is provided essentially because of the asserts (and GC > profile marking support) that visitHeader() performs, which markNoTracing() > bottoms out in. > > 3-4 method calls are reqd to get to visitHeader(), so I didn't want to risk any > inlining machinery pointlessly getting in the way in a release build. Thanks for the explanation. Would you add a comment for this? https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1511: for (HeapPage<Header>* page = m_firstPage; page; page = page->next()) { Nit: Unintended {}?
Thanks for being persistent on this. Hopefully the final comment! https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1972: MarkingVisitor(CallbackStack* markingStack) Add explicit. https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2011: // release builds will crash at the following item->call Update this comment. Now we don't have "the following item->call". https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2087: if (UNLIKELY(m_checkIfNeedsTracing && !needsTracing(objectPointer))) Hmm, it's a bit unfortunate we need to have this branch. In the previous code, this branch was needed only for the mark() method in the ThreadLocalMarking mode. Would it be possible to keep the templatized Mode and avoid adding this branch to the GlobalMarking mode? https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:240: return true; \ Shouldn't this be false? https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:242: return false; \ Shouldn't this be true? https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:281: return true; Ditto. https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:283: return false; Ditto. https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:203: // invoke trace() on unmarked objects deriving from class T right away, unmarked objects => not-yet-marked objects https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:204: // and not queue its trace callback on the marker stack. its trace callback => their trace callbacks https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:221: class TraceEagerlyTrait<Member<T>> { Don't we need the TraceEagerlyTrait for Persistent and CrossThreadPersistent as well? https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:591: // If trait allows it, invoke trace callback right here on unmarked objects. unmarked objects => not-yet-marked objects
https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2087: if (UNLIKELY(m_checkIfNeedsTracing && !needsTracing(objectPointer))) On 2014/12/02 06:16:04, haraken wrote: > > Hmm, it's a bit unfortunate we need to have this branch. In the previous code, > this branch was needed only for the mark() method in the ThreadLocalMarking > mode. Would it be possible to keep the templatized Mode and avoid adding this > branch to the GlobalMarking mode? As I said in the previous comment, I don't see a way to do that without having separate MarkingVisitor for these two modes (either by parameterizing MarkingVisitor over a type, or making visitHeader() virtual.)
https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1511: for (HeapPage<Header>* page = m_firstPage; page; page = page->next()) { On 2014/12/02 00:41:58, kouhei wrote: > Nit: Unintended {}? Sorry, this was unrelated to your CL. Please ignore.
https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/20001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2052: #if ENABLE(ASSERT) On 2014/12/02 00:41:58, kouhei wrote: > On 2014/11/28 12:05:08, sof wrote: > > On 2014/11/28 01:00:57, kouhei wrote: > > > Would you describe why we need ENABLE(ASSERT) / no ENABLE(ASSERT) version? > > > > A debug-specific version is provided essentially because of the asserts (and > GC > > profile marking support) that visitHeader() performs, which markNoTracing() > > bottoms out in. > > > > 3-4 method calls are reqd to get to visitHeader(), so I didn't want to risk > any > > inlining machinery pointlessly getting in the way in a release build. > > Thanks for the explanation. Would you add a comment for this? Certainly, done. https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1511: for (HeapPage<Header>* page = m_firstPage; page; page = page->next()) { On 2014/12/02 06:51:02, kouhei wrote: > On 2014/12/02 00:41:58, kouhei wrote: > > Nit: Unintended {}? > Sorry, this was unrelated to your CL. Please ignore. Your bracing preference is identical to mine though :) https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:1972: MarkingVisitor(CallbackStack* markingStack) On 2014/12/02 06:16:04, haraken wrote: > > Add explicit. Done. https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2087: if (UNLIKELY(m_checkIfNeedsTracing && !needsTracing(objectPointer))) On 2014/12/02 06:19:27, sof wrote: > On 2014/12/02 06:16:04, haraken wrote: > > > > Hmm, it's a bit unfortunate we need to have this branch. In the previous code, > > this branch was needed only for the mark() method in the ThreadLocalMarking > > mode. Would it be possible to keep the templatized Mode and avoid adding this > > branch to the GlobalMarking mode? > > As I said in the previous comment, I don't see a way to do that without having > separate MarkingVisitor for these two modes (either by parameterizing > MarkingVisitor over a type, or making visitHeader() virtual.) Had a go at the former; acceptable? https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:240: return true; \ On 2014/12/02 06:16:04, haraken wrote: > > Shouldn't this be false? Good catch; hadn't been updated after ensureMarked() flipped meaning of its result. https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... File Source/platform/heap/Visitor.h (right): https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:203: // invoke trace() on unmarked objects deriving from class T right away, On 2014/12/02 06:16:05, haraken wrote: > > unmarked objects => not-yet-marked objects Done. https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:204: // and not queue its trace callback on the marker stack. On 2014/12/02 06:16:05, haraken wrote: > > its trace callback => their trace callbacks Done. https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:221: class TraceEagerlyTrait<Member<T>> { On 2014/12/02 06:16:05, haraken wrote: > > Don't we need the TraceEagerlyTrait for Persistent and CrossThreadPersistent as > well? That would be preferable; thanks for catching their omission. Now added, moving these partial specializations to Handle.h in the process. https://codereview.chromium.org/765673004/diff/60001/Source/platform/heap/Vis... Source/platform/heap/Visitor.h:591: // If trait allows it, invoke trace callback right here on unmarked objects. On 2014/12/02 06:16:05, haraken wrote: > > unmarked objects => not-yet-marked objects Thanks, done.
LGTM with comments! https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2023: #endif Can we just write: ASSERT(!pageFromObject(objectPointer)->orphaned()); ? https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2024: if (Mode == ThreadLocalMarking && !needsTracing(objectPointer)) Don't we need to check this before doing header->mark()? ThreadLocalMarking should not mark any object in another thread's heap. https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2095: if (Mode == ThreadLocalMarking && !needsTracing(objectPointer)) Ditto. I guess this check needs to be done before header->mark() (or even before header->isMarked()). Where to put this check wouldn't matter for performance because it just affects performance of a thread-local GC. https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2110: if (Mode == ThreadLocalMarking && !needsTracing(objectPointer)) \ Ditto. https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2125: if (Mode == ThreadLocalMarking && !needsTracing(objectPointer)) \ Ditto. https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2246: static inline bool needsTracing(const void* objectPointer) needsTracing => containedInThisThreadHeap ? https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2253: if (!page->terminating()) return page->terminating();
https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2246: static inline bool needsTracing(const void* objectPointer) On 2014/12/02 15:35:10, haraken wrote: > > needsTracing => containedInThisThreadHeap ? or terminatingThreadContains()
https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2024: if (Mode == ThreadLocalMarking && !needsTracing(objectPointer)) On 2014/12/02 15:35:10, haraken wrote: > > Don't we need to check this before doing header->mark()? ThreadLocalMarking > should not mark any object in another thread's heap. I'm just mirroring what is done now; don't we want to mark that the object is reachable from another heap?
On 2014/12/02 15:38:35, haraken wrote: > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > File Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:2246: static inline bool needsTracing(const void* > objectPointer) > On 2014/12/02 15:35:10, haraken wrote: > > > > needsTracing => containedInThisThreadHeap ? > > or terminatingThreadContains() That's what the needsTracing() predicate currently checks for. It seems a bit more general to name it after its purpose, rather than what it currently checks for, but I can do some alpha conversion.
https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2024: if (Mode == ThreadLocalMarking && !needsTracing(objectPointer)) On 2014/12/02 15:46:51, sof wrote: > On 2014/12/02 15:35:10, haraken wrote: > > > > Don't we need to check this before doing header->mark()? ThreadLocalMarking > > should not mark any object in another thread's heap. > > I'm just mirroring what is done now; don't we want to mark that the object is > reachable from another heap? Currently we're doing the check when popping an object from the marking stack. In other words: (1) Pop an object from the marking stack. (2) Do the check about ThreadLocalMarking. (3) Mark the object. To mirror the behavior in your new code where the check is done before pushing, I guess you need to add the check before marking the object. Conceptually we shouldn't mark objects outside the terminating thread's heap (and I hope we're doing that in the current code).
On 2014/12/02 15:50:53, sof wrote: > On 2014/12/02 15:38:35, haraken wrote: > > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > > File Source/platform/heap/Heap.cpp (right): > > > > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > > Source/platform/heap/Heap.cpp:2246: static inline bool needsTracing(const > void* > > objectPointer) > > On 2014/12/02 15:35:10, haraken wrote: > > > > > > needsTracing => containedInThisThreadHeap ? > > > > or terminatingThreadContains() > > That's what the needsTracing() predicate currently checks for. It seems a bit > more general to name it after its purpose, rather than what it currently checks > for, but I can do some alpha conversion. I don't have a strong opinion here. Since the method makes sense only for a thread local GC, I just thought needsTracing is something a bit too general :)
On 2014/12/02 15:56:54, haraken wrote: > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > File Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:2024: if (Mode == ThreadLocalMarking && > !needsTracing(objectPointer)) > On 2014/12/02 15:46:51, sof wrote: > > On 2014/12/02 15:35:10, haraken wrote: > > > > > > Don't we need to check this before doing header->mark()? ThreadLocalMarking > > > should not mark any object in another thread's heap. > > > > I'm just mirroring what is done now; don't we want to mark that the object is > > reachable from another heap? > > Currently we're doing the check when popping an object from the marking stack. > In other words: > > (1) Pop an object from the marking stack. > (2) Do the check about ThreadLocalMarking. > (3) Mark the object. > No, (1) mark the object. (2) push its trace callback onto the marking stack ... (3) pop the object from the marking stack (4) do the check about ThreadLocalMarking. (5) invoke its trace callback. ..or do I need fresh air? :)
On 2014/12/02 15:58:50, haraken wrote: > On 2014/12/02 15:50:53, sof wrote: > > On 2014/12/02 15:38:35, haraken wrote: > > > > > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > > > Source/platform/heap/Heap.cpp:2246: static inline bool needsTracing(const > > void* > > > objectPointer) > > > On 2014/12/02 15:35:10, haraken wrote: > > > > > > > > needsTracing => containedInThisThreadHeap ? > > > > > > or terminatingThreadContains() > > > > That's what the needsTracing() predicate currently checks for. It seems a bit > > more general to name it after its purpose, rather than what it currently > checks > > for, but I can do some alpha conversion. > > I don't have a strong opinion here. Since the method makes sense only for a > thread local GC, I just thought needsTracing is something a bit too general :) I think we've exceeded followup iterations and nit'ery on this review by now, tbh. Let's not spend time on trivialities (=> I will address.)
On 2014/12/02 16:01:03, sof wrote: > On 2014/12/02 15:56:54, haraken wrote: > > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > > File Source/platform/heap/Heap.cpp (right): > > > > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > > Source/platform/heap/Heap.cpp:2024: if (Mode == ThreadLocalMarking && > > !needsTracing(objectPointer)) > > On 2014/12/02 15:46:51, sof wrote: > > > On 2014/12/02 15:35:10, haraken wrote: > > > > > > > > Don't we need to check this before doing header->mark()? > ThreadLocalMarking > > > > should not mark any object in another thread's heap. > > > > > > I'm just mirroring what is done now; don't we want to mark that the object > is > > > reachable from another heap? > > > > Currently we're doing the check when popping an object from the marking stack. > > In other words: > > > > (1) Pop an object from the marking stack. > > (2) Do the check about ThreadLocalMarking. > > (3) Mark the object. > > > > No, > > (1) mark the object. > (2) push its trace callback onto the marking stack > ... > (3) pop the object from the marking stack > (4) do the check about ThreadLocalMarking. > (5) invoke its trace callback. > > ..or do I need fresh air? :) Hmm, you seem to be right. Your CL is keeping the current behavior. I don't think it's a right thing to mark objects in another thread's heap, but let's address the issue in a follow-up :)
On 2014/12/02 16:04:31, haraken wrote: > On 2014/12/02 16:01:03, sof wrote: > > On 2014/12/02 15:56:54, haraken wrote: > > > > > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... > > > Source/platform/heap/Heap.cpp:2024: if (Mode == ThreadLocalMarking && > > > !needsTracing(objectPointer)) > > > On 2014/12/02 15:46:51, sof wrote: > > > > On 2014/12/02 15:35:10, haraken wrote: > > > > > > > > > > Don't we need to check this before doing header->mark()? > > ThreadLocalMarking > > > > > should not mark any object in another thread's heap. > > > > > > > > I'm just mirroring what is done now; don't we want to mark that the object > > is > > > > reachable from another heap? > > > > > > Currently we're doing the check when popping an object from the marking > stack. > > > In other words: > > > > > > (1) Pop an object from the marking stack. > > > (2) Do the check about ThreadLocalMarking. > > > (3) Mark the object. > > > > > > > No, > > > > (1) mark the object. > > (2) push its trace callback onto the marking stack > > ... > > (3) pop the object from the marking stack > > (4) do the check about ThreadLocalMarking. > > (5) invoke its trace callback. > > > > ..or do I need fresh air? :) > > Hmm, you seem to be right. Your CL is keeping the current behavior. > > I don't think it's a right thing to mark objects in another thread's heap, but > let's address the issue in a follow-up :) Nit: The reason it is no harm to (mis-)mark another thread's heap would be because we clear all mark bits before starting any global GC (in markUnmarkedObjectsDead()).
https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2023: #endif On 2014/12/02 15:35:10, haraken wrote: > > Can we just write: > > ASSERT(!pageFromObject(objectPointer)->orphaned()); > > ? Done. https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2246: static inline bool needsTracing(const void* objectPointer) On 2014/12/02 15:38:35, haraken wrote: > On 2014/12/02 15:35:10, haraken wrote: > > > > needsTracing => containedInThisThreadHeap ? > > or terminatingThreadContains() objectInTerminatingThreadHeap(). https://codereview.chromium.org/765673004/diff/100001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:2253: if (!page->terminating()) On 2014/12/02 15:35:10, haraken wrote: > > return page->terminating(); Done.
Still LGTM. Landing.
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/765673004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=186355 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
