Remove unused template indirection from the WeakMaps

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #610774 - Flags: review?(jimb)

Comment 1

5 years ago
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?
Attachment #610774 - Flags: review?(jimb) → review+
(Assignee)

Comment 2

5 years ago
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
Attachment #610774 - Attachment is obsolete: true
Attachment #611019 - Flags: review+

Comment 3

5 years ago
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

5 years ago
Looks great otherwise!
(Assignee)

Comment 5

5 years ago
Seems reasonable.

https://hg.mozilla.org/integration/mozilla-inbound/rev/555c6be77592
https://hg.mozilla.org/mozilla-central/rev/555c6be77592
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.