Last Comment Bug 740673 - Remove unused template indirection from the WeakMaps
: Remove unused template indirection from the WeakMaps
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-29 17:37 PDT by Terrence Cole [:terrence]
Modified: 2012-04-03 01:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0: remove the unused template parameters. (9.71 KB, patch)
2012-03-29 17:37 PDT, Terrence Cole [:terrence]
jimb: review+
Details | Diff | Splinter Review
v1: Updated with review feedback. (9.70 KB, patch)
2012-03-30 13:53 PDT, Terrence Cole [:terrence]
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-03-29 17:37:05 PDT
Created attachment 610774 [details] [diff] [review]
v0: remove the unused template parameters.

The goal of this work was to abstract away the weakmap details from the GC and CC.  Since then, the CC has re-implemented the same logic internally.  In the face of a moving GC, we will need new primitives here anyway.  For now, we should remove the extra indirection for ease of modification, debugging, and maintenance.
Comment 1 Jim Blandy :jimb 2012-03-30 11:59:00 PDT
Comment on attachment 610774 [details] [diff] [review]
v0: remove the unused template parameters.

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

r=me, with the name issue fixed (unless I've misunderstood what's going on there)

::: js/src/jsweakmap.h
@@ +209,5 @@
> +    bool IsMarked(const HeapPtrScript&x) {
> +        return !IsAboutToBeFinalized(x);
> +    }
> +
> +    bool MarkIfMarked(JSTracer *trc, HeapValue *x) {

MarkIfMarked is not a great name. It really marks x if it's *not* marked. How about "MarkIfNeeded" or "MarkAsNeeded" or "MarkIfUnmarked"?

@@ +212,5 @@
> +
> +    bool MarkIfMarked(JSTracer *trc, HeapValue *x) {
> +        if (IsMarked(*x))
> +            return false;
> +        js::gc::MarkValue(trc, x, "WeakMap entry");

Existing issue, but: we know more than this, right? We can say "WeakMap entry value".

@@ -221,5 @@
>          bool markedAny = false;
>          for (Range r = Base::all(); !r.empty(); r.popFront()) {
> -            const Key &k = r.front().key;
> -            Value &v = r.front().value;
> -            /* If the entry is live, ensure its key and value are marked. */

Did you want to drop this comment?
Comment 2 Terrence Cole [:terrence] 2012-03-30 13:53:59 PDT
Created attachment 611019 [details] [diff] [review]
v1: Updated with review feedback.

IsMarked(obj) == !IsAboutToBeFinalized(obj)
IsAboutToBeFinalized(obj) == !obj->isMarked()

Even so, I shouldn't have gotten so turned around that I inverted the meaning of the function name.  I think I'd best throw this one at try before committing.

https://tbpl.mozilla.org/?tree=Try&rev=58d029b61f3e
Comment 3 Jim Blandy :jimb 2012-03-30 15:27:58 PDT
Comment on attachment 611019 [details] [diff] [review]
v1: Updated with review feedback.

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

::: js/src/jsweakmap.h
@@ +212,5 @@
> +
> +    bool Mark(JSTracer *trc, HeapValue *x) {
> +        if (IsMarked(*x))
> +            return false;
> +        js::gc::MarkValue(trc, x, "WeakMap entry value");

Sorry, I was unclear: I didn't mean that the names should identify the type. What I meant was that the messages could distinguish 'key' edges from 'value' edges. So "WeakMap entry value" would be the text for all three.
Comment 4 Jim Blandy :jimb 2012-03-30 15:29:12 PDT
Looks great otherwise!
Comment 5 Terrence Cole [:terrence] 2012-04-02 10:55:09 PDT
Seems reasonable.

https://hg.mozilla.org/integration/mozilla-inbound/rev/555c6be77592
Comment 6 Marco Bonardo [::mak] 2012-04-03 01:59:58 PDT
https://hg.mozilla.org/mozilla-central/rev/555c6be77592

Note You need to log in before you can comment on or make changes to this bug.