Closed Bug 1257395 Opened 8 years ago Closed 8 years ago

GCPolicy and GCHashMap need doc update

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jimb, Assigned: jimb)

Details

Attachments

(1 file)

The comments for GCHashMap and GCPolicy are slightly out of date.
I hope I wasn't over-aggressive here. I think it's better to spell out exactly what methods do, rather than describe their role:

    Trace the edge |*tp|, ...

rather than:

    Tells the GC how to traverse the edge.

It also seems to me like the aggregates don't specialize GCPolicy, but rather *use* GCPolicy to handle their contained values correctly. The language is unclear about this.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Attachment #8731498 - Flags: review?(terrence)
Comment on attachment 8731498 [details] [diff] [review]
Update comments for GCHashTable and GCPolicy.

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

This is very nice. While we've updated the comments quite a bit, they were drafted initially before we had any real grasp of the full scope of their interaction.

Looking closer at the C++ spec, I misused "aggregate". It's not just about having a statically defined number of elements, but also requires POD members. This isn't quite what I meant, so lets curtail usage. I've left inline comments in all the places I found.

::: js/public/GCHashTable.h
@@ +48,5 @@
> +// sweeping, currently it requires an explicit call to <map>.sweep().
> +//
> +// If tracing your keys can change them, use GCRekeyableHashMap, which rehashes
> +// keys when their hash value has changed. However, prefer to use
> +// MovableCellHasher for pointers to things that can be tenured and compacted.

I'd avoid even mentioning GCRekeyableHashMap. Our usage is almost at zero and ideally we'll be able to make it an implementation detail of the unique id system.

::: js/public/GCPolicyAPI.h
@@ +18,4 @@
>  //
>  //   static void trace(JSTracer, T* tp, const char* name)
> +//       - Trace the edge |*tp|, calling the edge |name|. Aggregates like
> +//         GCHashMap and GCHashSet use this method to trace their children.

Let's not use "aggregate" here. Maybe "Containers use it to trace their contained sets."

@@ +22,4 @@
>  //
>  //   static bool needsSweep(T* tp)
> +//       - Return true if |*tp| is about to be finalized. Otherwise, update the
> +//         edge for moving GC, and return false. Aggregates like GCHashMap and

Containers like...

@@ +23,5 @@
>  //   static bool needsSweep(T* tp)
> +//       - Return true if |*tp| is about to be finalized. Otherwise, update the
> +//         edge for moving GC, and return false. Aggregates like GCHashMap and
> +//         GCHashSet use this method to decide when to remove an entry: if this
> +//         function returns true on a key/value/member/etc, its entry its

"its entry its" needs to lose the third t.

@@ +24,5 @@
> +//       - Return true if |*tp| is about to be finalized. Otherwise, update the
> +//         edge for moving GC, and return false. Aggregates like GCHashMap and
> +//         GCHashSet use this method to decide when to remove an entry: if this
> +//         function returns true on a key/value/member/etc, its entry its
> +//         dropped from the aggregate. Specializing this method is the standard

aggregate -> container
Attachment #8731498 - Flags: review?(terrence) → review+
Flags: in-testsuite-
Target Milestone: --- → mozilla48
https://hg.mozilla.org/mozilla-central/rev/a55e30611e2c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.