612561 - Infrastructure for exact/partly-exact tracing

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P3)

Description

[Lars T Hansen]

(Broken out from bug #576307, go there for historical discussions of older versions of the code.)

This is the infrastructure for exact tracing - support for a virtual gcTrace() method on GC objects, plus methods on the GC instances to trace pointers.

Comment 1

[Lars T Hansen]

Attachment: GC infrastructure for exact marking, v2

Addresses most comments made on the patch when it was offered for review on the parent bug.

The API to manually written tracers is now much simpler: the client code always passes a pointer to the location containing the value to be traced.  This way, the client code does not have to worry about the heap graph code.

The patch still has a gcTrace/gcTraceLarge distinction, and I expect it will land that way, with cleanup following.  It's not a big deal given that most tracers are not hand-written any longer.

I did not break the MarkItem refactoring out as a separate patch, sorry - I'm pressed for time and I would like this patch reviewed as-is.

Comment 2

[Lars T Hansen]

Attachment: GC infrastructure for exact marking, v3

Very minor changes from v2 (type casts, one signature change) to deal with MMGC_HEAP_GRAPH not compiling.  Reviewers can choose to stick with v2.

Comment 3

[Tommy Reilly]

Comment on attachment 491478 [details] [diff] [review]
GC infrastructure for exact marking, v3

I reviewed the changes to this since the last time and it looks good.   One thought regarding SetHasTrace.   Do we need to allow SetHasTrace to be exposed to the mutator?  I'm thinking when safegc forces ctor invocation to move in the GC we can completely hide the trace setting.   Ie:

    return MMgc::setExact(new (gc, AbcEnv::calcExtra(builtinPool)) AbcEnv(builtinPool, builtinCodeContext));

becomes:

return GCNewFlagsExtra<AbcEnv>(gc, kExtactTrace, AbcEnv::calcExtra(builtinPool), builtinPool, builtinCodeContext);

or maybe the macros could specialize GCNewExtra for AbcEnv so it'd just be:

return GCNewFlagsExtra<AbcEnv>(gc, AbcEnv::calcExtra(builtinPool), builtinPool, builtinCodeContext);

Comment 4

[Tommy Reilly]

The previous comment wasn't a knock on the patch I think it can go in as is, just thinking about how to address that FIXME by using the pending safegc GCNew stuff.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

i have not gotten to this yet but I will review either after dinner tonight or Saturday morning.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 491478 [details] [diff] [review]
GC infrastructure for exact marking, v3

Hesitant R+ ... it looked great (mostly issues with the comments) until I bit the bullet and started going through the conservative trace refactoring.  I do trust you to address these issues without a re-review though.

So, I'll put my notes about that first, and then the rest.


MMgc/GC.cpp
- GC::TraceConservativePointer(): you probably should have done the exercise of
  refactoring the existing body of the loop within GC::MarkItem(); it
  looks to me like you threw away the VALGRIND_MAKE_MEM_DEFINED calls
  without putting corresponding calls into
  GC::TraceConservativePointer().  Please fix or provide explanation;
  also, probably good to get Tommy's explicit thumbs-up either way
  since he owns the Valgrind work.

- There's other small differences that make this not a true
  refactoring; e.g.  a "block->containsPointers" got changed into
  "((GCAlloc*)block->alloc)->ContainsPointers()" and I do not know
  whether that was important.

- Also, in the previous version, the policy.signalDemographics invocation
  came after the while loop finished in MarkItem, while in your version it
  is invoked on every call to TraceConservativePointer.  I don't know
  enough about MMGC_POINTINESS_PROFILING to know if this matters, but
  again it makes me mistrust your refactoring.


MMgc/WriteBarrier.h
- I see you moved ZeroPtr<T> from GCObject.h.  I see docs for this in
  in 3rd part of your gc audit.  Is there a bugzilla ticket open for
  the work relating to this?

- Did you remove this comment (and the similar one with
  WriteBarrierRC) because they are false, or just superfluous?

  // Always pay for a single real function call; then inline & optimize massively in WriteBarrier()

  (I personally found notes like this useful when I was first reading
   the code, as signs that someone had already considered the
   structure here in depth)

MMgc/GCObject.h
- "variable-length objects like arrays must implement both methods"
  You're talking about ActionScript arrays there (as opposed to C++
  arrays).  I guess that is obvious after a moment's consideration,
  but it would save a bit of cognitive overhead if you said
  "ActionScript arrays"

- In comment above GCTraceableObject, you mistakenly refer to the
  MMgc::setExact() procedure as GC::setExact().

- In comment above GCFinalizedObject, you mistakenly refer to the
  MMgc::setExact() procedure as setExactGC().

- Am I correct that ~GCTraceableBase() will only ever be invoked from
  (the now implicitly defined) ~GCFinalizedObject method?  Maybe
  incorporate that fact into "must be empty" comment therein.


MMgc/GC.h
- I do not understand the documentation above GC::movePointers.  Its
  state is not the fault of your patch (there's some semi-humorous
  typos) , but there is quite confusing; e.g. it says "Start must be a
  GC object" and I cannot tell what "Start" was before your change, if
  anything, and what it is now, if anything.  I would not hold up
  landing your patch for fixing this, but if you do leave it unfixed,
  then open a new bug for fixing it.

- GC::Zero(): I'd *slightly* prefer that this be moved up into a patch
  that either used it or gave a hint of a usage;
  e.g. objectPopulationProfiling.  Not a deal-breaker though.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #6)
> - GC::TraceConservativePointer(): you probably should have done the exercise of
>   refactoring the existing body of the loop within GC::MarkItem()

I've started to get pretty good at patch queue hacking from the AsyncGC work, so maybe I'll have a go at doing this myself on Sunday, if I have some time to kill.  Lars, I'll let you know if that happens.

Comment 8

[Lars T Hansen]

(In reply to comment #3)
> One thought regarding SetHasTrace.   Do we need to allow SetHasTrace to
> be exposed to the mutator?  I'm thinking when safegc forces ctor invocation
> to move in the GC we can completely hide the trace setting.   Ie:
> 
>     return MMgc::setExact(new (gc, AbcEnv::calcExtra(builtinPool))
> AbcEnv(builtinPool, builtinCodeContext));
> 
> becomes:
> 
> return GCNewFlagsExtra<AbcEnv>(gc, kExtactTrace,
> AbcEnv::calcExtra(builtinPool), builtinPool, builtinCodeContext);
> 
> or maybe the macros could specialize GCNewExtra for AbcEnv so it'd just be:
> 
> return GCNewFlagsExtra<AbcEnv>(gc, AbcEnv::calcExtra(builtinPool), builtinPool,
> builtinCodeContext);

I believe that that would be incorrect.  If you set the exact-trace bit before all the constructors have run you risk - to some extent - that the object is in an improper state for exact tracing if the tracer is invoked during construction.  The trivial example of this is that AvmPlusScriptableObject causes an allocation when profiling is enabled, yet no non-faulting tracer is installed at that point - it shows up in ScriptObject, String, and Namespace.  (I found this the hard way but the analysis was easy.)

Now it is possible that you can work around that by having an object that's all-zero-bits but I would not count on it.  One of the things about exact tracing is that occasionally manually written tracers will know about object layout and off-to-the-side tag bits, and use those to drive tracing.  In principle such a tracer would not make sense until after the constructor for that particular subclass had run.

The cleanest solution seems to me to set the exact-tracing bit when the object has been constructed, and thus exposing MMgc::setExact().

Once we templatize createInstance() we may think about how to unify some of the logic so that there's less to worry about for programmers, at least for avmglue.

Comment 9

[Lars T Hansen]

(In reply to comment #6)
> 
> 
> MMgc/GC.cpp
> - GC::TraceConservativePointer(): you probably should have done the exercise of
>   refactoring the existing body of the loop within GC::MarkItem(); it
>   looks to me like you threw away the VALGRIND_MAKE_MEM_DEFINED calls
>   without putting corresponding calls into
>   GC::TraceConservativePointer().  Please fix or provide explanation;
>   also, probably good to get Tommy's explicit thumbs-up either way
>   since he owns the Valgrind work.

Some of these are probably bugs and need further attention (It's unlikely a refactoring would have saved my bacon in this case; the problem is that a large section of code is affected and every time somebody touches the marker I have to re-copy hundreds of lines of code over, it's easy to make mistakes.  That does not excuse the bugs of course.)

> - There's other small differences that make this not a true
>   refactoring; e.g.  a "block->containsPointers" got changed into
>   "((GCAlloc*)block->alloc)->ContainsPointers()" and I do not know
>   whether that was important.

Ditto.

> - Also, in the previous version, the policy.signalDemographics invocation
>   came after the while loop finished in MarkItem, while in your version it
>   is invoked on every call to TraceConservativePointer.  I don't know
>   enough about MMGC_POINTINESS_PROFILING to know if this matters, but
>   again it makes me mistrust your refactoring.

Is not a big deal but I'll look into it anyway.

> MMgc/WriteBarrier.h
> - I see you moved ZeroPtr<T> from GCObject.h.  I see docs for this in
>   in 3rd part of your gc audit.  Is there a bugzilla ticket open for
>   the work relating to this?

No, it had to happen because of a header file dependency and was done ad-hoc.  I can factor it into a separate file or perhaps find a better file for it (GC.h?) if you like but it cannot stay where it was.

> - Did you remove this comment (and the similar one with
>   WriteBarrierRC) because they are false, or just superfluous?
> 
>   // Always pay for a single real function call; then inline & optimize
> massively in WriteBarrier()

I don't remember, I'll look.

>   (I personally found notes like this useful when I was first reading
>    the code, as signs that someone had already considered the
>    structure here in depth)

Thanks, that's why they're there.

> MMgc/GCObject.h
> - "variable-length objects like arrays must implement both methods"
>   You're talking about ActionScript arrays there (as opposed to C++
>   arrays).  I guess that is obvious after a moment's consideration,
>   but it would save a bit of cognitive overhead if you said
>   "ActionScript arrays"

WILLFIX.

> - In comment above GCTraceableObject, you mistakenly refer to the
>   MMgc::setExact() procedure as GC::setExact().

WILLFIX.

> - In comment above GCFinalizedObject, you mistakenly refer to the
>   MMgc::setExact() procedure as setExactGC().

WILLFIX.

> - Am I correct that ~GCTraceableBase() will only ever be invoked from
>   (the now implicitly defined) ~GCFinalizedObject method?  

Yes.

> Maybe
> incorporate that fact into "must be empty" comment therein.

WILLFIX.


> MMgc/GC.h
> - I do not understand the documentation above GC::movePointers.  Its
>   state is not the fault of your patch (there's some semi-humorous
>   typos) , but there is quite confusing; e.g. it says "Start must be a
>   GC object" and I cannot tell what "Start" was before your change, if
>   anything, and what it is now, if anything.  I would not hold up
>   landing your patch for fixing this, but if you do leave it unfixed,
>   then open a new bug for fixing it.

Will look into it.

> - GC::Zero(): I'd *slightly* prefer that this be moved up into a patch
>   that either used it or gave a hint of a usage;
>   e.g. objectPopulationProfiling.  Not a deal-breaker though.

I would prefer that too but it ended up being just one more patch.  The problem is that it ends up being used in a patch that follows exactMarking but precedes objectPopulationProfiling, so it can't go in the latter, and it does not belong in that intermediate patch.  Thus I could create one to go after exactMarking but before the intermediate patch, and at that point I got fed up.

Comment 10

[Tommy Reilly]

(In reply to comment #8)
> (In reply to comment #3)

> I believe that that would be incorrect.  If you set the exact-trace bit before
> all the constructors have run you risk - to some extent - that the object is in
> an improper state for exact tracing if the tracer is invoked during
> construction. 

GCNew invokes GC::Alloc and then calls your ctor using placement new.  Then it would set the trace bit.  Functionally its exactly the same, just code movement from mutator to MMgc.

Comment 11

[Lars T Hansen]

(In reply to comment #10)
> 
> GCNew invokes GC::Alloc and then calls your ctor using placement new.  Then it
> would set the trace bit.  Functionally its exactly the same, just code movement
> from mutator to MMgc.

Well, then :-)

Comment 12

[Tommy Reilly]

ZeroPtr is dead code, I was %99 sure and I just checked and now I'm %100 sure, I'll open a bug to nuke it.

Comment 13

[Tommy Reilly]

https://bugzilla.mozilla.org/show_bug.cgi?id=613942

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #9)
> > - Also, in the previous version, the policy.signalDemographics invocation
> >   came after the while loop finished in MarkItem, while in your version it
> >   is invoked on every call to TraceConservativePointer.  I don't know
> >   enough about MMGC_POINTINESS_PROFILING to know if this matters, but
> >   again it makes me mistrust your refactoring.
> 
> Is not a big deal but I'll look into it anyway.

You can ignore this comment; I just realized why you made this change: you moved could_be_pointer and actually_is_pointer into TraceConservativePointer.  I believe they are now incremented at most once (on each call to TraceConservativePointer).  It looks you're doing the right thing here.

Comment 15

[Tommy Reilly]

> > MMgc/GC.cpp
> > it
> >   looks to me like you threw away the VALGRIND_MAKE_MEM_DEFINED calls
> >   without putting corresponding calls into
> >   GC::TraceConservativePointer().  

I'd consider this a blocking bug, I think Brent's logging it separately.   Unfortunately even after fixing this we still see issues with exact tracing wrt to valgrind.

Comment 16

[Lars T Hansen]

> > MMgc/GC.cpp
> > - GC::TraceConservativePointer() ... refactoring the existing body ...
> >   looks to me like you threw away the VALGRIND_MAKE_MEM_DEFINED calls
> >   without putting corresponding calls into
> >   GC::TraceConservativePointer().

Fixed.

> > - A "block->containsPointers" got changed into
> >   "((GCAlloc*)block->alloc)->ContainsPointers()"

Fixed.

> > - ... policy.signalDemographics invocation ...

Correct the way it is, given that TraceConservativePointer can also be invoked via the GC_CONSERVATIVE annotation; we either do accounting each time we examine a word (as now) or there will be additional arguments passed around to accumulate the values.  It's probably slower to do it for each word, but MMGC_POINTINESS_PROFILING is disabled in FP Release builds so it does not matter.

> > MMgc/WriteBarrier.h
> > - I see you moved ZeroPtr<T> from GCObject.h.

ZeroPtr has now been removed by a separate patch, cf bug #613942.

> > - Did you remove this comment ...

Fixed.  The comment was supposed to have been moved along with the body of the function to the new WriteBarrier-inlines.h.

> > MMgc/GCObject.h
> > - "variable-length objects like arrays must implement both methods"

Actually neither C++ or ActionScript arrays, but "array-like storage".  But the comment needs work regardless.

> > - In comment above GCTraceableObject, you mistakenly refer to the
> >   MMgc::setExact() procedure as GC::setExact().

Fixed.

> > - In comment above GCFinalizedObject, you mistakenly refer to the
> >   MMgc::setExact() procedure as setExactGC().

Fixed.

> > Maybe
> > incorporate that fact into "must be empty" comment therein.

Fixed.

> > MMgc/GC.h
> > - I do not understand the documentation above GC::movePointers. ...

Actually the documentation is criminally bad, and not much better for the next function (movePointersWithinBlock), which is additionally misnamed ("block" should be "object").  I will fix the docs but the renaming will have to wait, I'll file a bug.

> > - GC::Zero(): I'd prefer that this be moved up into a patch ...

WONTFIX.

Comment 17

[Tamarin Bot]

changeset: 5581:af8647c0925c
user:      Lars T Hansen <[e-mail]>
summary:   ExactGC work: Fix 612561 - Infrastructure for exact/partly-exact tracing (r=treilly, r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/af8647c0925c

Comment 18

[Lars T Hansen]

I will schedule follow-on work (hiding MMgc::setExact, merging gcTrace and gcTraceLarge) as separate bugs.