Closed Bug 1153959 Opened 9 years ago Closed 9 years ago

Replace the trace details infrastructure with RAII on CallbackTracer

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This lets us rip out the last of the prior marking interface. It also gets rid of all of the random calls to unsetTracingDetails we used to have to sprinkle all over the place, since there was no well-defined lifetime.
Attachment #8591796 - Flags: review?(jcoppeard)
Comment on attachment 8591796 [details] [diff] [review]
4.12_remove_tracing_details-v0.diff

Review of attachment 8591796 [details] [diff] [review]:
-----------------------------------------------------------------

This is much better than setting/unsetting everything manually.

::: js/public/TracingAPI.h
@@ +159,5 @@
> +    // trace, as it is traced. This contains, at a minimum, an edge name and,
> +    // when tracing an array, the index. Further specialization can be achived
> +    // (with some complexity), by associating a functor with the tracer so
> +    // that, when requested, the user can generate totally custom edge
> +    // descriptions.

Thanks for adding this explanation.

@@ +180,5 @@
> +    const char* getTracingEdgeName(char* buffer, size_t bufferSize);
> +
> +    // The trace implementation may associate a callback with one or more edges
> +    // using AutoTracingDetails. This functor is called by getTracingEdgeName
> +    // and is responsible for providing a textural representation of the

typo: textual

@@ +185,5 @@
> +    // currently being traced edge. The callback has access to the full heap,
> +    // including the currently set tracing context.
> +    class ContextFunctor {
> +      public:
> +        virtual void operator()(CallbackTracer* trc, char* buf, size_t bufsize) {};

Maybe make this pure virtual?

@@ +286,5 @@
> +// exists in the object graph. When marking some types of things, e.g. Value
> +// edges, it is necessary to copy into a temporary on the stack. This class
> +// records the original location if we need to copy the tracee, so that the
> +// relevant analyses can continue to operate correctly.
> +class AutoFixupTraceLocation

Should we use something like 'original' or 'actual' rather than 'fixup' as the name?  Might be more descriptive.

@@ +294,5 @@
> +  public:
> +#ifdef JS_GC_ZEAL
> +    template <typename T>
> +    AutoFixupTraceLocation(JSTracer* trc, T*const* realLocation) : trc_(nullptr) {
> +        if (trc->isCallbackTracer() && trc->asCallbackTracer()->contextRealLocation_ == nullptr) {

I'm not clear on how these can nest.  AutoTracingIndex asserts it can't be nested, but this just does nothing if the real location is already set.  Is that correct?

::: js/src/gc/RootMarking.cpp
@@ +609,5 @@
>      MOZ_ASSERT(grayBufferState == GrayBufferState::Okay);
>      MOZ_ASSERT(zone->isGCMarkingGray() || zone->isGCCompacting());
>  
> +    for (auto elem : zone->gcGrayRoots)
> +        TraceManuallyBarrieredGenericPointerEdge(&marker, &elem.thing_, "buffered gray root");

Can we make use of the recorded thing kind here?
Attachment #8591796 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #1)
> Comment on attachment 8591796 [details] [diff] [review]
> 4.12_remove_tracing_details-v0.diff
> 
> Review of attachment 8591796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +185,5 @@
> > +    // currently being traced edge. The callback has access to the full heap,
> > +    // including the currently set tracing context.
> > +    class ContextFunctor {
> > +      public:
> > +        virtual void operator()(CallbackTracer* trc, char* buf, size_t bufsize) {};
> 
> Maybe make this pure virtual?

Good catch! I changed this to test a different architecture and forgot to change it back.

> @@ +286,5 @@
> > +// exists in the object graph. When marking some types of things, e.g. Value
> > +// edges, it is necessary to copy into a temporary on the stack. This class
> > +// records the original location if we need to copy the tracee, so that the
> > +// relevant analyses can continue to operate correctly.
> > +class AutoFixupTraceLocation
> 
> Should we use something like 'original' or 'actual' rather than 'fixup' as
> the name?  Might be more descriptive.

I like 'original' much better!

> @@ +294,5 @@
> > +  public:
> > +#ifdef JS_GC_ZEAL
> > +    template <typename T>
> > +    AutoFixupTraceLocation(JSTracer* trc, T*const* realLocation) : trc_(nullptr) {
> > +        if (trc->isCallbackTracer() && trc->asCallbackTracer()->contextRealLocation_ == nullptr) {
> 
> I'm not clear on how these can nest.  AutoTracingIndex asserts it can't be
> nested, but this just does nothing if the real location is already set.  Is
> that correct?

Index doesn't nest only because it does not currently nest in practice. Name and location both do nest, so they need extra code to support that. In particular for location, we -- as you surmised -- do want the /original/ location. Take, for example, a Value hashtable key. We copy it out of the table, storing the original location, then when we mark it as a Value we dump the contained pointer back on the stack and mark from there, trying to update the original location a second time. The address the barrier is going to see is an internal pointer to the hash table key, so we need to not update the second (or later) time, only the first.

> ::: js/src/gc/RootMarking.cpp
> @@ +609,5 @@
> >      MOZ_ASSERT(grayBufferState == GrayBufferState::Okay);
> >      MOZ_ASSERT(zone->isGCMarkingGray() || zone->isGCCompacting());
> >  
> > +    for (auto elem : zone->gcGrayRoots)
> > +        TraceManuallyBarrieredGenericPointerEdge(&marker, &elem.thing_, "buffered gray root");
> 
> Can we make use of the recorded thing kind here?

It was (1) the only thing keeping MarkKind alive and (2) it's not clear that carrying it around is going to be faster than getting it from the arena header. I will remove the kind from the edge set so that this will iterate faster.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=923c4c4e199d
Bit of orange, but that appears to be all pre-existing on the tip and misc infra bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/15df3c88bb31
https://hg.mozilla.org/mozilla-central/rev/15df3c88bb31
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1158809
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: