Preliminary Harmony weak maps API implementation.

Preliminary Harmony weak maps API implementation.

R=rossberg@chromium.org,danno@chromium.org
BUG=v8:1565
TEST=mjsunit/harmony/weakmaps

Committed: http://code.google.com/p/v8/source/detail?r=8811

[danno, 2011-07-29 08:17:23]

You're heading in the right direction!

There are a bunch of places in the runtime that specialize behavior depending on
the subclass of JSObject, using the instance_type to distinguish them (as a
random example, take a look at JSObject::GetHeaderSize). You will need to ensure
that your new weak map subclass is properly handled in all those cases. As a
start, just grep for JS_OBJECT_TYPE and in the source (and maybe JS_PROXY_TYPE,
since Andreas also did something similar when he added proxy support).

You're also missing some GC support that is needed for handling evacuation from
new heap and proper handing of the mark-and-compact heap. Some object-debugging
boilerplate is also missing. 

Take a look at http://codereview.chromium.org/7089002/ which also introduced a
new subclass of Object (but not JSObject) for an example of some of the things
you will have to add. Take a look at mark-compact.cc and heap.cc.

Please also add some GC tests (gc() calls) to your. js test to actually trigger
the GC support. You can make the GC API available to the test case by using
--expose-gc in the header. You can find examples of the usage here:

test/mjsunit/unbox-double-arrays.js

One last not about testing: please try to create cases in your .js test that
trigger the runtime code paths that you add... it's not always possible, but
worth a try :-)

That will keep you busy for a while. :-)

http://codereview.chromium.org/7529007/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7529007/diff/1/src/objects.h#newcode335
src/objects.h:335: V(JS_WEAKMAP_TYPE)                                           
               \
nit: WEAK_MAP is more consistent

[danno, 2011-07-29 08:52:37]

Ah, it looks like you're aware of the needed mark-compact support from your
email. However, even if you want to land the "strong WeakMap" support
temporarily, you will still have to add the visitor.

[Michael Starzinger, 2011-07-29 19:46:36]

On 2011/07/29 08:17:23, danno wrote:
> You're heading in the right direction!

Thanks for the confirmation. It is good to know that I am not missing the point
completely. :)

> There are a bunch of places in the runtime that specialize behavior depending
on
> the subclass of JSObject, using the instance_type to distinguish them (as a
> random example, take a look at JSObject::GetHeaderSize). You will need to
ensure
> that your new weak map subclass is properly handled in all those cases. As a
> start, just grep for JS_OBJECT_TYPE and in the source (and maybe
JS_PROXY_TYPE,
> since Andreas also did something similar when he added proxy support).

I grepped through the code and found several such occurrences. But I think I am
close to tracking them all down by now. Especially the one in
JSObject::GetHeaderSize is easy to miss, because it is only called by the native
API. Will upload an updated CL later today.

> You're also missing some GC support that is needed for handling evacuation
from
> new heap and proper handing of the mark-and-compact heap. Some
object-debugging
> boilerplate is also missing. 
> 
> Take a look at http://codereview.chromium.org/7089002/ which also introduced a
> new subclass of Object (but not JSObject) for an example of some of the things
> you will have to add. Take a look at mark-compact.cc and heap.cc.

I think for the "strong WeakMap" it is sufficient to reuse the visitor for
normal JSObjects, because the backing hash table is referenced by the header
which all visitors will pick up. In my opinion objects-visiting.cc:118 should
take care of that. Or am I missing something here?

Yes, once the GC "learns" about the special behavior of WeakMaps, we will need a
separate visitor id and a new entry in the visitor dispatch table. But I wanted
to make this change in a separate CL because it is not necessary for "strong
WeakMaps".

> Please also add some GC tests (gc() calls) to your. js test to actually
trigger
> the GC support. You can make the GC API available to the test case by using
> --expose-gc in the header. You can find examples of the usage here:
> 
> test/mjsunit/unbox-double-arrays.js

Fixed. Inserted two explicit invocations of gc() into the test case. Thanks for
the hint about --expose-gc, that will be really helpful!

> One last not about testing: please try to create cases in your .js test that
> trigger the runtime code paths that you add... it's not always possible, but
> worth a try :-)

Yes, especially JSObject::GetHeaderSize is giving me a hard time. I think it is
not possible to the that from within JS. Should I add an extra cctest for that?

> That will keep you busy for a while. :-)
> 
> http://codereview.chromium.org/7529007/diff/1/src/objects.h
> File src/objects.h (right):
> 
> http://codereview.chromium.org/7529007/diff/1/src/objects.h#newcode335
> src/objects.h:335: V(JS_WEAKMAP_TYPE)                                         
 
>                \
> nit: WEAK_MAP is more consistent

Fixed.

[rossberg, 2011-08-02 12:33:38]

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc#newcode2180
src/bootstrapper.cc:2180: if (FLAG_harmony_weakmaps) {  // -- W e a k M a p
Can you perhaps move this into a separate function InitializeExperimentalGlobal?

http://codereview.chromium.org/7529007/diff/4001/src/flag-definitions.h
File src/flag-definitions.h (right):

http://codereview.chromium.org/7529007/diff/4001/src/flag-definitions.h#newco...
src/flag-definitions.h:101: DEFINE_bool(harmony_weakmaps, false, "enable harmony
weak-maps")
Nit: use space instead of hyphen.

http://codereview.chromium.org/7529007/diff/4001/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/7529007/diff/4001/src/messages.js#newcode204
src/messages.js:204: invalid_weakmap_key:          ["Invalid value used as
weak-map key"],
Nit: space instead of hyphen.

http://codereview.chromium.org/7529007/diff/4001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7529007/diff/4001/src/runtime.cc#newcode638
src/runtime.cc:638: RUNTIME_FUNCTION(MaybeObject*, Runtime_WeakMapCreate) {
Since unlike the others, this function does not actually create the object, but
only initializes it, I would prefer calling it WeakMapInitialize or
WeakMapConstruct.

http://codereview.chromium.org/7529007/diff/4001/src/weakmap.js
File src/weakmap.js (right):

http://codereview.chromium.org/7529007/diff/4001/src/weakmap.js#newcode43
src/weakmap.js:43: if (!IS_OBJECT(key)) {
You probably want IS_SPEC_OBJECT here (and below).

http://codereview.chromium.org/7529007/diff/4001/src/weakmap.js#newcode61
src/weakmap.js:61: //%SetProperty($WeakMap.prototype, "constructor", $WeakMap,
DONT_ENUM);
Is there a reason that this is commented out?

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
File test/mjsunit/harmony/weakmaps.js (right):

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
test/mjsunit/harmony/weakmaps.js:55: TestMapping(m, new Object, new Object);
Does this work with proxies as keys?

[danno, 2011-08-02 12:48:02]

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc#newcode2185
src/bootstrapper.cc:2185:
//global_context()->set_weakmap_function(*weakmap_fun);
Should this really be commented out? If the code isn't used, it's better to
remove it.

http://codereview.chromium.org/7529007/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7529007/diff/4001/src/objects.h#newcode6648
src/objects.h:6648: DISALLOW_IMPLICIT_CONSTRUCTORS(JSWeakMap);
I wonder, do you really properly handle in-line properties here? Or do you
accidentally overwrite them?

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
File test/mjsunit/harmony/weakmaps.js (right):

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
test/mjsunit/harmony/weakmaps.js:96: assertTrue(WeakMap.prototype.get instanceof
Function)
add test cases that set and get arbitrary properties and elements on a weak map,
especially ones designed to trigger fast in-object properties.

[Michael Starzinger, 2011-08-02 14:05:22]

Updated patch set.

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc#newcode2180
src/bootstrapper.cc:2180: if (FLAG_harmony_weakmaps) {  // -- W e a k M a p
On 2011/08/02 12:33:38, rossberg wrote:
> Can you perhaps move this into a separate function
InitializeExperimentalGlobal?

Done. I suppose you meant InstallExperimentalNatives, right?

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc#newcode2185
src/bootstrapper.cc:2185:
//global_context()->set_weakmap_function(*weakmap_fun);
On 2011/08/02 12:48:02, danno wrote:
> Should this really be commented out? If the code isn't used, it's better to
> remove it.

Done.

http://codereview.chromium.org/7529007/diff/4001/src/flag-definitions.h
File src/flag-definitions.h (right):

http://codereview.chromium.org/7529007/diff/4001/src/flag-definitions.h#newco...
src/flag-definitions.h:101: DEFINE_bool(harmony_weakmaps, false, "enable harmony
weak-maps")
On 2011/08/02 12:33:38, rossberg wrote:
> Nit: use space instead of hyphen.

Done.

http://codereview.chromium.org/7529007/diff/4001/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/7529007/diff/4001/src/messages.js#newcode204
src/messages.js:204: invalid_weakmap_key:          ["Invalid value used as
weak-map key"],
On 2011/08/02 12:33:38, rossberg wrote:
> Nit: space instead of hyphen.

Done

http://codereview.chromium.org/7529007/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7529007/diff/4001/src/objects.h#newcode6648
src/objects.h:6648: DISALLOW_IMPLICIT_CONSTRUCTORS(JSWeakMap);
On 2011/08/02 12:48:02, danno wrote:
> I wonder, do you really properly handle in-line properties here? Or do you
> accidentally overwrite them?

Yes, any in-object properties would overwrite the inlined "table" field in this
case. I just verified that weakmap objects have zero in-object properties and
added the following assertion to the initialization method:

ASSERT(weakmap->map()->inobject_properties() == 0);

http://codereview.chromium.org/7529007/diff/4001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7529007/diff/4001/src/runtime.cc#newcode638
src/runtime.cc:638: RUNTIME_FUNCTION(MaybeObject*, Runtime_WeakMapCreate) {
On 2011/08/02 12:33:38, rossberg wrote:
> Since unlike the others, this function does not actually create the object,
but
> only initializes it, I would prefer calling it WeakMapInitialize or
> WeakMapConstruct.

Done.

http://codereview.chromium.org/7529007/diff/4001/src/weakmap.js
File src/weakmap.js (right):

http://codereview.chromium.org/7529007/diff/4001/src/weakmap.js#newcode43
src/weakmap.js:43: if (!IS_OBJECT(key)) {
On 2011/08/02 12:33:38, rossberg wrote:
> You probably want IS_SPEC_OBJECT here (and below).

Done.

http://codereview.chromium.org/7529007/diff/4001/src/weakmap.js#newcode61
src/weakmap.js:61: //%SetProperty($WeakMap.prototype, "constructor", $WeakMap,
DONT_ENUM);
On 2011/08/02 12:33:38, rossberg wrote:
> Is there a reason that this is commented out?

Done.

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
File test/mjsunit/harmony/weakmaps.js (right):

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
test/mjsunit/harmony/weakmaps.js:55: TestMapping(m, new Object, new Object);
On 2011/08/02 12:33:38, rossberg wrote:
> Does this work with proxies as keys?

No, actually it doesn't. Unfortunately the whole concept of identity hash codes
is implemented on JSObject, but a JSProxy cannot be cast to a JSObject. So it
will require quite some effort to make it work.

Does the Harmony proposal for proxies say anything about identity hash codes for
proxies?

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
test/mjsunit/harmony/weakmaps.js:96: assertTrue(WeakMap.prototype.get instanceof
Function)
On 2011/08/02 12:48:02, danno wrote:
> add test cases that set and get arbitrary properties and elements on a weak
map,
> especially ones designed to trigger fast in-object properties. 

Done.

[Michael Starzinger, 2011-08-02 14:37:47]

Added Slava as reviewer. Slava, could you please take a look? This is only the
API (no GC implementation yet).

[rossberg, 2011-08-02 15:32:04]

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc#newcode2180
src/bootstrapper.cc:2180: if (FLAG_harmony_weakmaps) {  // -- W e a k M a p
On 2011/08/02 14:05:22, Michael Starzinger wrote:
> On 2011/08/02 12:33:38, rossberg wrote:
> > Can you perhaps move this into a separate function
> InitializeExperimentalGlobal?
> 
> Done. I suppose you meant InstallExperimentalNatives, right?

Actually, no, I had in mind a new function that mirrors the role of
InitializeGlobal, but for the experimental stuff.

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
File test/mjsunit/harmony/weakmaps.js (right):

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
test/mjsunit/harmony/weakmaps.js:55: TestMapping(m, new Object, new Object);
On 2011/08/02 14:05:22, Michael Starzinger wrote:
> On 2011/08/02 12:33:38, rossberg wrote:
> > Does this work with proxies as keys?
> 
> No, actually it doesn't. Unfortunately the whole concept of identity hash
codes
> is implemented on JSObject, but a JSProxy cannot be cast to a JSObject. So it
> will require quite some effort to make it work.

In that case, some respective TODO in here would probably be good to have.

> Does the Harmony proposal for proxies say anything about identity hash codes
for
> proxies?

No, what specifically would you expect it to say? AFAICS, the spec never talks
about hashing or hash codes, nor should it.

[Michael Starzinger, 2011-08-02 16:04:51]

Added new patch set.

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc#newcode2180
src/bootstrapper.cc:2180: if (FLAG_harmony_weakmaps) {  // -- W e a k M a p
On 2011/08/02 15:32:04, rossberg wrote:
> On 2011/08/02 14:05:22, Michael Starzinger wrote:
> > On 2011/08/02 12:33:38, rossberg wrote:
> > > Can you perhaps move this into a separate function
> > InitializeExperimentalGlobal?
> > 
> > Done. I suppose you meant InstallExperimentalNatives, right?
> 
> Actually, no, I had in mind a new function that mirrors the role of
> InitializeGlobal, but for the experimental stuff.

Done.

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
File test/mjsunit/harmony/weakmaps.js (right):

http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmap...
test/mjsunit/harmony/weakmaps.js:55: TestMapping(m, new Object, new Object);
On 2011/08/02 15:32:04, rossberg wrote:
> On 2011/08/02 14:05:22, Michael Starzinger wrote:
> > On 2011/08/02 12:33:38, rossberg wrote:
> > > Does this work with proxies as keys?
> > 
> > No, actually it doesn't. Unfortunately the whole concept of identity hash
> codes
> > is implemented on JSObject, but a JSProxy cannot be cast to a JSObject. So
it
> > will require quite some effort to make it work.
> 
> In that case, some respective TODO in here would probably be good to have.

Done. Added comment to runtime.cc, that's were the assertion will trigger in
case you pass JSProxy objects.

> > Does the Harmony proposal for proxies say anything about identity hash codes
> for
> > proxies?
> 
> No, what specifically would you expect it to say? AFAICS, the spec never talks
> about hashing or hash codes, nor should it.

Hmm, I am so used to identity hashing, can't imagine an OO-language without it.
:)

[Vyacheslav Egorov (Chromium), 2011-08-02 18:37:32]

I did a quick pass over and everything looks ok to me.

[danno, 2011-08-03 07:12:41]

LGTM