740673 - Remove unused template indirection from the WeakMaps

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 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]

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]]

Attachment: 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]

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]

Looks great otherwise!

Comment 5

[Terrence Cole [:terrence]]

Seems reasonable.

https://hg.mozilla.org/integration/mozilla-inbound/rev/555c6be77592

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/555c6be77592