Last Comment Bug 814562 - Implement clear() on WeakMaps
: Implement clear() on WeakMaps
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla20
Assigned To: Saurabh Anand [:sawrubh]
:
Mentors:
Depends on: 1101817
Blocks: es6
  Show dependency treegraph
 
Reported: 2012-11-22 21:03 PST by Boris Zbarsky [:bz]
Modified: 2014-11-19 13:18 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (2.05 KB, patch)
2012-11-27 07:07 PST, Saurabh Anand [:sawrubh]
jwalden+bmo: feedback+
Details | Diff | Splinter Review
Patch v2 (1.90 KB, patch)
2012-12-03 12:28 PST, Saurabh Anand [:sawrubh]
jorendorff: review+
Details | Diff | Splinter Review
Test v1 (1.31 KB, patch)
2012-12-03 12:29 PST, Saurabh Anand [:sawrubh]
jorendorff: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-11-22 21:03:55 PST
Unless there's a reason we don't have that?
Comment 1 Andrew McCreight [:mccr8] 2012-11-23 11:35:59 PST
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 3 Saurabh Anand [:sawrubh] 2012-11-27 07:07:01 PST
Created attachment 685623 [details] [diff] [review]
Patch v1
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-11-27 15:11:17 PST
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] 2012-12-03 12:28:14 PST
Created attachment 687906 [details] [diff] [review]
Patch v2
Comment 6 Saurabh Anand [:sawrubh] 2012-12-03 12:29:06 PST
Created attachment 687908 [details] [diff] [review]
Test v1
Comment 7 Jason Orendorff [:jorendorff] 2012-12-06 13:42:56 PST
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] 2012-12-06 13:45:01 PST
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 11 Jean-Yves Perrier [:teoli] 2012-12-09 11:20:39 PST
I updated https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/WeakMap
and Fx 20 for developers.

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