ES6: Implement WeakSet

ES6: Implement WeakSet

WeakSets work similar to ordinary Sets but the value (which must be an
object) is held weakly.

This is implemented using a WeakMap internally.

This is available under --harmony-collections

BUG=v8:2785

[Michael Starzinger, 2013-07-17 09:46:32]

I'll do a technical review. Adding Andreas and Dmitry for a procedural aspects
of adding/launching new features.

[Dmitry Lomov (no reviews), 2013-07-17 10:58:51]

Re launching new features, I think this is fine. WeakSet is in July 16th ES6
draft, and we are already implementing other collections from ES6 draft under
the same flag. Therefore intent-to-implement for this feature has already been
approved. 

On 2013/07/17 09:46:32, Michael Starzinger wrote:
> I'll do a technical review. Adding Andreas and Dmitry for a procedural aspects
> of adding/launching new features.

[Michael Starzinger, 2013-07-17 12:56:41]

https://codereview.chromium.org/19429002/diff/2001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode333
src/collection.js:333: var weaksetDataSymbol = %CreateSymbol(void 0);
This requires that Symbols can model private state, which currently holds for
our implementation but will probably not hold in ES6, so it would be good to not
introduce this requirement here. Since we already need modifications to the
bootstrapper (see comment below), adding a new instance type (i.e.
JS_WEAK_SET_TYPE) is trivial.

As for the runtime functions (i.e. Runtime_WeakMapHas and friends) we could
either duplicate them in runtime.cc for WeakSet or introduce a common super-type
in the type hirarchy in objects.h, both approaches sound equally fine to Andreas
and me.

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode336
src/collection.js:336: function WeakSet() {
nit: s/WeakSet/WeakSetConstructor/ for consistency.

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode345
src/collection.js:345: var $WeakSet = global.WeakSet = WeakSet;
This way of adding "global.WeakSet" unfortunately makes this property
enumerable. While I generally support the move to bootstrap as much as possible
in JavaScript, I am afraid this part of the initialization needs to go into
bootstrapper.cc similar to e.g. WeakMap. Once we can move InstallFunction() into
JavaScript, we should do so for all global properties at once. This can be
checked using either of the following code in global scope:

Object.getOwnPropertyDescriptor(this, "WeakSet");
Object.keys(this).indexOf("WeakSet");

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode349
src/collection.js:349: var weaksetData = this[weaksetDataSymbol];
It would be nice to get rid of this additional indirection. See comment at the
top if this file.

[arv (Not doing code reviews), 2013-07-17 15:26:26]

Moving things to C++ would make this a lot more complex. However, it does seem
doable.

https://codereview.chromium.org/19429002/diff/2001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode333
src/collection.js:333: var weaksetDataSymbol = %CreateSymbol(void 0);
On 2013/07/17 12:56:41, Michael Starzinger wrote:
> This requires that Symbols can model private state, which currently holds for
> our implementation but will probably not hold in ES6, so it would be good to
not
> introduce this requirement here. Since we already need modifications to the
> bootstrapper (see comment below), adding a new instance type (i.e.
> JS_WEAK_SET_TYPE) is trivial.

We already depend on private symbols in a few other places. We can change to use
WeakMap in those few places if I fail to convince TC39 to have private symbols.

> As for the runtime functions (i.e. Runtime_WeakMapHas and friends) we could
> either duplicate them in runtime.cc for WeakSet or introduce a common
super-type
> in the type hirarchy in objects.h, both approaches sound equally fine to
Andreas
> and me.

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode336
src/collection.js:336: function WeakSet() {
On 2013/07/17 12:56:41, Michael Starzinger wrote:
> nit: s/WeakSet/WeakSetConstructor/ for consistency.

No, then the name property would be wrong.

Adding test.

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode345
src/collection.js:345: var $WeakSet = global.WeakSet = WeakSet;
On 2013/07/17 12:56:41, Michael Starzinger wrote:
> This way of adding "global.WeakSet" unfortunately makes this property
> enumerable. While I generally support the move to bootstrap as much as
possible
> in JavaScript, I am afraid this part of the initialization needs to go into
> bootstrapper.cc similar to e.g. WeakMap. Once we can move InstallFunction()
into
> JavaScript, we should do so for all global properties at once. This can be
> checked using either of the following code in global scope:
> 
> Object.getOwnPropertyDescriptor(this, "WeakSet");
> Object.keys(this).indexOf("WeakSet");

Easy to fix without C++ bootstrapping.

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode349
src/collection.js:349: var weaksetData = this[weaksetDataSymbol];
On 2013/07/17 12:56:41, Michael Starzinger wrote:
> It would be nice to get rid of this additional indirection. See comment at the
> top if this file.

Instead of this indirection we would need to check that there is a
[[WeakSetData]] or the traditional way of checking [[Class]]. I assume checking
the [[Class]] is faster.

[Michael Starzinger, 2013-07-18 08:33:18]

I could be convinced to land the current approach (without adding a separate
instance type in C++) for now and leave that for a follow-up CL to cleanup.
Looping in Andreas about using Symbols as private state though.

https://codereview.chromium.org/19429002/diff/2001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode333
src/collection.js:333: var weaksetDataSymbol = %CreateSymbol(void 0);
On 2013/07/17 15:26:26, arv wrote:
> On 2013/07/17 12:56:41, Michael Starzinger wrote:
> > This requires that Symbols can model private state, which currently holds
for
> > our implementation but will probably not hold in ES6, so it would be good to
> not
> > introduce this requirement here. Since we already need modifications to the
> > bootstrapper (see comment below), adding a new instance type (i.e.
> > JS_WEAK_SET_TYPE) is trivial.
> 
> We already depend on private symbols in a few other places. We can change to
use
> WeakMap in those few places if I fail to convince TC39 to have private
symbols.
> 
> > As for the runtime functions (i.e. Runtime_WeakMapHas and friends) we could
> > either duplicate them in runtime.cc for WeakSet or introduce a common
> super-type
> > in the type hirarchy in objects.h, both approaches sound equally fine to
> Andreas
> > and me.
> 

Andreas, what is your opinion about using a Synbol here to model private state?
I could be convinced to land the CL with this current approach. But I'll leave
the final say to Andreas as he is the expert on our Symbol implementation.

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode336
src/collection.js:336: function WeakSet() {
On 2013/07/17 15:26:26, arv wrote:
> On 2013/07/17 12:56:41, Michael Starzinger wrote:
> > nit: s/WeakSet/WeakSetConstructor/ for consistency.
> 
> No, then the name property would be wrong.
> 
> Adding test.

Good point, thanks for adding the tests. But I would still use
%FunctionSetName() to set the correct "name" property on that function instead
of forcing the property name on the builtins global object to have the same name
(or alternatively address the next comment).

Then we can s/WeakSet/WeakSetConstructor/ here.

https://codereview.chromium.org/19429002/diff/2001/src/collection.js#newcode345
src/collection.js:345: var $WeakSet = global.WeakSet = WeakSet;
On 2013/07/17 15:26:26, arv wrote:
> On 2013/07/17 12:56:41, Michael Starzinger wrote:
> > This way of adding "global.WeakSet" unfortunately makes this property
> > enumerable. While I generally support the move to bootstrap as much as
> possible
> > in JavaScript, I am afraid this part of the initialization needs to go into
> > bootstrapper.cc similar to e.g. WeakMap. Once we can move InstallFunction()
> into
> > JavaScript, we should do so for all global properties at once. This can be
> > checked using either of the following code in global scope:
> > 
> > Object.getOwnPropertyDescriptor(this, "WeakSet");
> > Object.keys(this).indexOf("WeakSet");
> 
> Easy to fix without C++ bootstrapping.

This issue might be easy to fix without C++, but it introduces disparity in the
into bootstrapping process, that's what I meant with "we should do so for all
global properties at once" in my original comment.

To give one example of where that will bite us: The experimental builtins (e.g.
WeakMap, Symbol, ...) all have their prototype property (e.g. WeakMap.prototype,
...) be writable which is wrong according to the spec draft. We intend to fix
this soon and I would have no clue how to fix this in plain JavaScript.

So I would kindly ask you to install "global.WeakSet" in C++ for the sake of
consistency and maintainability.

[rossberg, 2013-07-18 09:22:09]

On 2013/07/18 08:33:18, Michael Starzinger wrote:
> I could be convinced to land the current approach (without adding a separate
> instance type in C++) for now and leave that for a follow-up CL to cleanup.
> Looping in Andreas about using Symbols as private state though.

I don't worry about the symbol as such too much, because we have to find a
solution for that problem anyway and then fix all uses we already have (which
aren't many yet, though).

But I still wouldn't be happy about landing the current approach. It's not how
we want this to be implemented ultimately. On the other hand, doing the
implementation right isn't very difficult (you can largely follow
JS_WEAK_MAP_TYPE), so I see no good reason to iterate on a temporary solution.

To clarify, we just ask to implement a new instance type, with corresponding
runtime functions. It can still use the lower-level ObjectHashTable to realise
the set.

[arv (Not doing code reviews), 2013-07-23 17:11:38]

Committed as https://code.google.com/p/v8/source/detail?r=15792

[arv (Not doing code reviews), 2013-07-23 17:11:54]

On 2013/07/23 17:11:38, arv wrote:
> Committed as https://code.google.com/p/v8/source/detail?r=15792

Using a different approach...