727501 - GC metrics for telemetry

Status: RESOLVED WONTFIX

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

Description

[Ruchi Lohani]

This bug is for landing/tracking GC metrics using telemetry in MMgc.

Comment 1

[Ruchi Lohani]

Attachment: Patch v0

Thinking behind putting these macros for time measurement:

start incremental mark  = sweep [SweepNeedsSweeping()]  +  mark [MarkNonstackRoots()] + mark [IncrementalMark()]
finish incremental mark = mark (final root and stack scan) + sweep(finalize and sweep)
Incremental mark = mark[IncrementalMark()]
time spent by GC in last collection = start incremental mark +  incremental mark + finish incremental mark

GC policymanager also tracks time and some other metrics internally for profiling use case and we could use timeInLastCollection as an estimate of how accurate these values are.

Comment 2

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

(am working towards review now; I wanted to get a telemetry monitoring app set up first, but now I have that and feel better able to evaluate the work here and elsewhere.)

Comment 3

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

Comment on attachment 597543 [details] [diff] [review]
Patch v0

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

Looks good.  I had some small asides but really I wouldn't be too upset if this landed as-is, as long as you fix the small tab/whitespace issues first.  :)

::: MMgc/GC.cpp
@@ +498,4 @@
>              return;
>          }
>  
> +        TELEMETRY_METHOD(getTelemetry(), ".gc.Collect");

aside: "sigh.  I dislike the Collect API; it never made sense to me to have to invoke Collect twice to reliably force a collection.  Maybe now is the time to consider teasing a StartCollect method out, and then make Collect always mean Collect-to-the-finish?  Or do you think that is just making work for ourselves with no payoff?"

@@ +703,4 @@
>          if (nogc)
>              return;
>  
> +        TELEMETRY_METHOD(getTelemetry(), ".gc.CollectionWork");

1. Does Monocle readily allow one to collapse together multiple metrics with a shared prefix, so that we could emit:
  .gc.CollectionWork.StartIncrementalMark
  .gc.CollectionWork.FinishIncrementalMark
  .gc.CollectionWork.IncrementalMark
?

2. (I'm actually on fence as to whether that would be a good idea at all.  My main thought is that when we have been unable to divvy up the work evenly, we'll should see time sucked up in FinishIncrementalMark.  But that may be useful to only we AVM developers, not to the Flash User community; does that affect our decision here?)

@@ +2079,5 @@
>  
> +        {
> +            TELEMETRY_METHOD(getTelemetry(), ".gc.Mark");
> +            if (incremental)
> +                MarkNonstackRoots();

Any reason this doesn't belong underneath the "if (incremental) { ... }" ?  (I don't care too much either way, just curious about whether we end up sending out a bunch of ~0 time .gc.Mark events.)

@@ +2088,3 @@
>          // FIXME (policy): arguably a bug to do this here if StartIncrementalMark has exhausted its quantum
>          // doing eager sweeping.
> +            

nit: get rid of spurious whitespace addition here before pushing

@@ +3029,5 @@
> +            ClearMarkStack();               // Frees any cached resources
> +            m_barrierWork.Clear();
> +            zct.Prune();                            // Frees unused memory
> +            
> +        } // Telemetry

Good to note this here.

I recommend being even more explicit, and say something like:
// End of Telemetry ".gc.Mark" extent.

::: MMgc/ZCT.cpp
@@ +317,5 @@
>      {
>          if(gc->collecting)
>              return;
> +		
> +		TELEMETRY_METHOD(gc->getTelemetry(), ".gc.Reap");

1. nit: tabs here.

2. Why not move this down below the assert and the "reaping = true;" assignment, putting it in the spot a few lines down where we similarly send a signal to the policy manager, so that all that similar instrumentation is close together?

Comment 4

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

(oh, p.s.: This is pretty cool to see in the Telemetry Monitor.  Fun fun fun!)

Comment 5

[Ruchi Lohani]

(In reply to Felix S Klock II from comment #3)
> Comment on attachment 597543 [details] [diff] [review]
> Patch v0

> ::: MMgc/GC.cpp

> >  
> > +        TELEMETRY_METHOD(getTelemetry(), ".gc.Collect");
> 
> aside: "sigh.  I dislike the Collect API; it never made sense to me to have
> to invoke Collect twice to reliably force a collection.  Maybe now is the
> time to consider teasing a StartCollect method out, and then make Collect
> always mean Collect-to-the-finish?  Or do you think that is just making work
> for ourselves with no payoff?"

I agree with you. I disliked it as well when I was looking at it. I will file a separate bug for those improvements. 

> 
> @@ +703,4 @@
> >          if (nogc)
> >              return;
> >  
> > +        TELEMETRY_METHOD(getTelemetry(), ".gc.CollectionWork");
> 
> 1. Does Monocle readily allow one to collapse together multiple metrics with
> a shared prefix, so that we could emit:
>   .gc.CollectionWork.StartIncrementalMark
>   .gc.CollectionWork.FinishIncrementalMark
>   .gc.CollectionWork.IncrementalMark
> ?

I am sure it can collapse together multiple with a shared prefix but that would mean some caution at the monocle end (I haven't explored monocle logic yet, I am mostly going to rely on the monocle team for making changes to monocle for any metrics I am adding). 
Also, notice that FinishIncrementalMark() does some mark work, for which a .gc.mark metric is sent. So sending a .gc.CollectionWork.FinishIncremental mark will not be able to tell that it is the time for some mark as well as sweep. 


> 
> @@ +2079,5 @@
> >  
> > +        {
> > +            TELEMETRY_METHOD(getTelemetry(), ".gc.Mark");
> > +            if (incremental)
> > +                MarkNonstackRoots();
> 
> Any reason this doesn't belong underneath the "if (incremental) { ... }" ? 
> (I don't care too much either way, just curious about whether we end up
> sending out a bunch of ~0 time .gc.Mark events.)

Agreed.

> 
> @@ +3029,5 @@
> > +            ClearMarkStack();               // Frees any cached resources
> > +            m_barrierWork.Clear();
> > +            zct.Prune();                            // Frees unused memory
> > +            
> > +        } // Telemetry
> 
> Good to note this here.
> 
> I recommend being even more explicit, and say something like:
> // End of Telemetry ".gc.Mark" extent.
> 
Will do.

> ::: MMgc/ZCT.cpp
> @@ +317,5 @@
> >      {
> >          if(gc->collecting)
> >              return;
> > +		
> > +		TELEMETRY_METHOD(gc->getTelemetry(), ".gc.Reap");
> 
> 1. nit: tabs here.
> 
> 2. Why not move this down below the assert and the "reaping = true;"
> assignment, putting it in the spot a few lines down where we similarly send
> a signal to the policy manager, so that all that similar instrumentation is
> close together?

Will do.

Comment 6

[Tamarin Bot]

changeset: 7211:5c0dcac88c8b
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 727501 - GC metrics for telemetry (p=rulohani, r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/5c0dcac88c8b