647229 - Profiling framework for weak references

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

Attachment: Simple profiling framework

We need to understand how the weak refs system behaves: how many weak refs are there (over time), how many probes per lookup, who is primarily responsible for allocating weak refs.

The attached framework provides data.  These are some of them:

* In the VM, nearly all weak refs are allocated by _buildMethodSignature 
  and _buildTraitsBindings (all but 3 out of about 1500 for a run of 
  abcdump on itself in a Mac DD build).

* The average probe length (hash table probe per access) was 1.6 with
  aggressive GC but 6.1 with normal GC settings.  But these numbers vary
  quite a lot.  (Also, with aggressive GC settings the table gets cleared
  out more often, so the comparison is a little shaky.)

* There were about 10 accesses per weak ref on average.

Comment 1

[Lars T Hansen]

Attachment: Simple profiling framework

Might as well land it, no sense in the code sitting around getting stale.

Comment 2

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

Comment on attachment 523592 [details] [diff] [review]
Simple profiling framework

Cool.

* Out of curiosity, for your commands in the bug description, what was
  the benchmark set that you used as the basis for the results
  presented?  E.g. was it just shell code, or a full player build?

  (I tested this only atop the shell on a single test program.)

* For this line:

     emptyWeakRef = new (this) GCWeakRef(NULL);      // We don't profile this one

  Why not profile it?  Because it is present on every run and has the
  same lifetime, and is therefore uninteresting?  Or because it is not
  created via the GCWeakRef::get() method, and so its simpler to not
  profile it than to accomodate the different creation control paths?

* Do you know why the existing ~GCHashtableBase destructor sets its
  int-fields to zero?  (Do you think it was some sort of accomodation,
  perhaps imagined, for the conservative collector?)  Should we be
  doing the same for the accesses and probes fields?

* For MMGC_GCHASHTABLE_PROFILER, perhaps we should track a count of
  the number of rehashes as well?  (Then again, the effort expended
  during rehash is already being tracked in the probes/accesses
  fields, since the grow() method delegates to find(), so perhaps this
  is unnecessary and/or premature.)

Suggestions:

* Move the method implementations for WeakRefAllocationSiteProfiler
  out of GCMemoryProfiler.h into GCMemoryProfiler.cpp

* To avoid confusion with C preprocessor macro, perhaps change:
    // Be sure to set MMGC_PROFILE=1.
==>
    // Be sure to set MMGC_PROFILE=1 in the runtime environment.

* nit: I'm not sure grammatical correctness is worth it in this code:

           if (numobjects == 1)
               GCLog("  1 object allocated");
           else

   especially since it also (slightly) complicate writing tools to
   post-process this data.  Most other code in GCMemoryProfiler seems
   to just use "%u objs" (though clearly there is precedent for what
   you did in DeletionProfiler::dumpObjectInfo).

Comment 3

[Lars T Hansen]

(In reply to comment #2)
> 
> * Out of curiosity, for your commands in the bug description, what was
>   the benchmark set that you used as the basis for the results
>   presented?  E.g. was it just shell code, or a full player build?

One run of abcdump.abc dumping itself.

> * For this line:
> 
>      emptyWeakRef = new (this) GCWeakRef(NULL);      // We don't profile this
> one
> 
>   Why not profile it?  Because it is present on every run and has the
>   same lifetime, and is therefore uninteresting?  Or because it is not
>   created via the GCWeakRef::get() method, and so its simpler to not
>   profile it than to accomodate the different creation control paths?

Because it's allocated before the profiler is allocated.   The comment is informative, not normative :-)

> * Do you know why the existing ~GCHashtableBase destructor sets its
>   int-fields to zero?

No.

>  (Do you think it was some sort of accomodation,
>   perhaps imagined, for the conservative collector?)  

That would be my first guess, without looking at the code.

> Should we be
>   doing the same for the accesses and probes fields?

I will have to ponder that before I land the patch.

> * For MMGC_GCHASHTABLE_PROFILER, perhaps we should track a count of
>   the number of rehashes as well?  (Then again, the effort expended
>   during rehash is already being tracked in the probes/accesses
>   fields, since the grow() method delegates to find(), so perhaps this
>   is unnecessary and/or premature.)

Maybe, but it's not something I'll worry about now.

> Suggestions:
> 
> * Move the method implementations for WeakRefAllocationSiteProfiler
>   out of GCMemoryProfiler.h into GCMemoryProfiler.cpp

Yeah... sigh.  I decided the file was far enough gone already (with the deletion profiler) that I was not damaging it further.  But you're right, of course.

> * To avoid confusion with C preprocessor macro, perhaps change:
>     // Be sure to set MMGC_PROFILE=1.
> ==>
>     // Be sure to set MMGC_PROFILE=1 in the runtime environment.

I may do that.  (I promise to do it if you unearth somebody who will state, without undue coercion, that he/she is actually confused.)

> * nit: I'm not sure grammatical correctness is worth it in this code:
> 
>            if (numobjects == 1)
>                GCLog("  1 object allocated");
>            else
> 
>    especially since it also (slightly) complicate writing tools to
>    post-process this data.  Most other code in GCMemoryProfiler seems
>    to just use "%u objs" (though clearly there is precedent for what
>    you did in DeletionProfiler::dumpObjectInfo).

Indeed, and I wrote that code too.

Comment 4

[Lars T Hansen]

I've addressed these remarks, though I did not unfix the grammar thing, nor did I add profiling for rehashes.

Comment 5

[Tamarin Bot]

changeset: 6158:5366b64229c9
user:      Lars T Hansen <[e-mail]>
summary:   Fix 647229 - Profiling framework for weak references (r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/5366b64229c9