Implement clear() on WeakMaps

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bz, Assigned: sawrubh)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla20
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Unless there's a reason we don't have that?
I think we just don't have it because it is a fairly recent addition. Should be very easy for anybody who understand spec-ese.
Blocks: 694100
Code at http://mxr.mozilla.org/mozilla-central/source/js/src/jsweakmap.cpp
Spec at http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.15.5.2

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 3

5 years ago
Created attachment 685623 [details] [diff] [review]
Patch v1
Assignee: general → saurabhanandiit
Status: NEW → ASSIGNED
Attachment #685623 - Flags: feedback?(jwalden+bmo)
Comment on attachment 685623 [details] [diff] [review]
Patch v1

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

Looks pretty good to me.  This needs a few tests before it can land, tho -- make sure clearing an already-empty WeakMap works, make sure clearing one with a live key works, make sure clearing one with a key that's turned to garbage works, and make sure clearing one with a key that's turned to garbage *and* we've done a GC works.

jorendorff might have more ideas for tests here, too.  Plus, since I'm not super-familiar with the WeakMap code, I'm slightly hesitant to take final responsibility for an actual review here -- ask him for that, please.  :-)

::: js/src/jsweakmap.cpp
@@ +153,5 @@
> +    Rooted<JSObject*> thisObj(cx, &args.thisv().toObject());
> +    if (ObjectValueMap *map = GetObjectMap(thisObj)) {
> +        js_delete(map);
> +        thisObj->setPrivate(NULL);
> +        return true;

Erm, I don't think you need to return at all here, just fall through -- indeed it'd be wrong to return true here, because then you won't be setting the right return value.

@@ +365,5 @@
>  };
>  
>  static JSFunctionSpec weak_map_methods[] = {
>      JS_FN("has",    WeakMap_has, 1, 0),
> +    JS_FN("clear",    WeakMap_clear, 0, 0),

Misaligned.
Attachment #685623 - Flags: feedback?(jwalden+bmo) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 687906 [details] [diff] [review]
Patch v2
Attachment #685623 - Attachment is obsolete: true
Attachment #687906 - Flags: review?(jorendorff)
(Assignee)

Comment 6

5 years ago
Created attachment 687908 [details] [diff] [review]
Test v1
Attachment #687908 - Flags: review?(jorendorff)
Comment on attachment 687906 [details] [diff] [review]
Patch v2

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

Check in code and tests in a single changeset, please.

::: js/src/jsweakmap.cpp
@@ +177,5 @@
> +    JS_ASSERT(IsWeakMap(args.thisv()));
> +
> +    if (ObjectValueMap *map = GetObjectMap(&args.thisv().toObject())) {
> +        map->clear();
> +    }

Please leave a comment explaining why we can't just js_delete(map). It's crazy enough to be worth explaining, I think.

@@ +390,5 @@
>  };
>  
>  static JSFunctionSpec weak_map_methods[] = {
>      JS_FN("has",    WeakMap_has, 1, 0),
> +    JS_FN("clear",  WeakMap_clear, 0, 0),

The order doesn't matter at all, but I wonder why "clear" fits in between "has" and "get" (the two non-mutating methods). I guess I would put it last--but it's fine where it is.
Attachment #687906 - Flags: review?(jorendorff) → review+
Comment on attachment 687908 [details] [diff] [review]
Test v1

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

::: js/src/jit-test/tests/collections/WeakMap-clear.js
@@ +13,5 @@
> +assertEq(wm.has(key), false);
> +
> +// Clearing a WeakMap with keys turned to garbage
> +for (var i = 0; i < 10; i++)
> +    wm.set(key, {});

This key never becomes garbage. It is directly referenced from a global variable. Instead, use:

    wm.set(key, {});
    for (var i = 0; i < 10; i++)
        wm.set({}, {});  // these keys become garbage immediately

@@ +20,5 @@
> +assertEq(wm.has(key), false);
> +
> +// Clearing a WeakMap with keys turned to garbage and after doing a GC
> +for (var i = 0; i < 10; i++)
> +    wm.set(key, {});

Same thing here.
Attachment #687908 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fa6a857c35
https://hg.mozilla.org/mozilla-central/rev/b5fa6a857c35
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
I updated https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/WeakMap
and Fx 20 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1101817
You need to log in before you can comment on or make changes to this bug.