Closed
Bug 1257395
Opened 8 years ago
Closed 8 years ago
GCPolicy and GCHashMap need doc update
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jimb, Assigned: jimb)
Details
Attachments
(1 file)
7.21 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
The comments for GCHashMap and GCPolicy are slightly out of date.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Comment 2•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Flags: in-testsuite-
Target Milestone: --- → mozilla48
Comment 4•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a55e30611e2c
You need to log in
before you can comment on or make changes to this bug.
Description
•