814562 - Implement clear() on WeakMaps

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Unless there's a reason we don't have that?

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 3

[Saurabh Anand [:sawrubh]]

Attachment: Patch v1

Comment 4

[Jeff Walden [:Waldo]]

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.

Comment 5

[Saurabh Anand [:sawrubh]]

Attachment: Patch v2

Comment 6

[Saurabh Anand [:sawrubh]]

Attachment: Test v1

Comment 7

[Jason Orendorff [: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.

Comment 8

[Jason Orendorff [:jorendorff]]

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.

Comment 9

[Saurabh Anand [:sawrubh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fa6a857c35

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b5fa6a857c35

Comment 11

[Jean-Yves Perrier [:teoli]]

I updated https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/WeakMap
and Fx 20 for developers.