Expose the content of Maps and WeakMaps through MapMirror.

Expose the content of Maps and WeakMaps through MapMirror.

BUG=v8:3291
LOG=N
R=aandrey@chromium.org, yangguo@chromium.org

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

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

[rossberg, 2014-07-16 09:51:07]

https://codereview.chromium.org/398513005/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/398513005/diff/1/src/runtime.cc#newcode1709
src/runtime.cc:1709: RUNTIME_FUNCTION(Runtime_GetMapEntries) {
Is this necessary? You can easily get the array of entries of an ordinary map
through its public JS interface, so we shouldn't need to add an extra runtime
function for that.

[Alexandra Mikhaylova, 2014-07-16 10:21:14]

https://codereview.chromium.org/398513005/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/398513005/diff/1/src/runtime.cc#newcode1709
src/runtime.cc:1709: RUNTIME_FUNCTION(Runtime_GetMapEntries) {
On 2014/07/16 09:51:07, rossberg wrote:
> Is this necessary? You can easily get the array of entries of an ordinary map
> through its public JS interface, so we shouldn't need to add an extra runtime
> function for that.

Thanks, I'll use the map iterator to get entries then.

[Yang, 2014-07-16 12:10:58]

I agree with Andreas' comment, and also have some of my own. We probably should
add a comment to the definition of MapMirror.entries stating that it will keep
keys alive for WeakMaps.

https://codereview.chromium.org/398513005/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/398513005/diff/1/src/runtime.cc#newcode1742
src/runtime.cc:1742: Handle<Object> key(table->KeyAt(i), isolate);
Indentation size is weird. We use 4 spaces for line break and 2 spaces for
scopes. You could also use "git cl format" to have it formatted for you.

https://codereview.chromium.org/398513005/diff/1/test/mjsunit/harmony/mirror-...
File test/mjsunit/harmony/mirror-collections.js (right):

https://codereview.chromium.org/398513005/diff/1/test/mjsunit/harmony/mirror-...
test/mjsunit/harmony/mirror-collections.js:9: var serializer =
debug.MakeMirrorSerializer();
Same here. 2 space indentation please.

https://codereview.chromium.org/398513005/diff/1/test/mjsunit/harmony/mirror-...
test/mjsunit/harmony/mirror-collections.js:51: assertEquals(44, entries[5]);
Having MapMirror.entries() contain both keys and value is somewhat confusing. It
would be nicer to have an array of key/value pairs. That way, entries.length
will also not be twice the actual number of added mappings.

On the other hand that would add some more overhead when preparing that array.

We could have methods on MapMirror that would abstract away from direct access
to entries, but then it becomes unclear when to refresh the entries array.

:/

[Alexandra Mikhaylova, 2014-07-16 17:00:38]

https://codereview.chromium.org/398513005/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/398513005/diff/1/src/runtime.cc#newcode1742
src/runtime.cc:1742: Handle<Object> key(table->KeyAt(i), isolate);
On 2014/07/16 12:10:58, Yang wrote:
> Indentation size is weird. We use 4 spaces for line break and 2 spaces for
> scopes. You could also use "git cl format" to have it formatted for you.

Done.

https://codereview.chromium.org/398513005/diff/1/test/mjsunit/harmony/mirror-...
File test/mjsunit/harmony/mirror-collections.js (right):

https://codereview.chromium.org/398513005/diff/1/test/mjsunit/harmony/mirror-...
test/mjsunit/harmony/mirror-collections.js:9: var serializer =
debug.MakeMirrorSerializer();
On 2014/07/16 12:10:58, Yang wrote:
> Same here. 2 space indentation please.

Done.

https://codereview.chromium.org/398513005/diff/1/test/mjsunit/harmony/mirror-...
test/mjsunit/harmony/mirror-collections.js:51: assertEquals(44, entries[5]);
On 2014/07/16 12:10:58, Yang wrote:
> Having MapMirror.entries() contain both keys and value is somewhat confusing.
It
> would be nicer to have an array of key/value pairs. That way, entries.length
> will also not be twice the actual number of added mappings.
> 
> On the other hand that would add some more overhead when preparing that array.
> 
> We could have methods on MapMirror that would abstract away from direct access
> to entries, but then it becomes unclear when to refresh the entries array.
> 
> :/

I've changed the return value of MapMirror.entries() to the array of key/value
pairs (objects).

I don't quite get the second part of your comment, however. We'd just like to be
able to pull all entries from a map at the given moment. So, it's kind of a
snapshot of the map. Could there be any issues with it?

[arv (Not doing code reviews), 2014-07-16 19:17:20]

FYI

https://codereview.chromium.org/398513005/diff/10001/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/398513005/diff/10001/src/mirror-debugger.js#n...
src/mirror-debugger.js:1295: var iter = this.value_.entries();
Do we need to ensure we are calling the original entries function here?

[Yang, 2014-07-17 08:49:30]

looking good.

https://codereview.chromium.org/398513005/diff/10001/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/398513005/diff/10001/src/mirror-debugger.js#n...
src/mirror-debugger.js:1295: var iter = this.value_.entries();
On 2014/07/16 19:17:20, arv wrote:
> Do we need to ensure we are calling the original entries function here?

That is right. We need to use %_CallFunction(this.value_, builtins.MapEntries)
to make sure we do not call a monkey-patched version.

[Alexandra Mikhaylova, 2014-07-17 09:16:21]

https://codereview.chromium.org/398513005/diff/10001/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/398513005/diff/10001/src/mirror-debugger.js#n...
src/mirror-debugger.js:1295: var iter = this.value_.entries();
On 2014/07/17 08:49:29, Yang wrote:
> On 2014/07/16 19:17:20, arv wrote:
> > Do we need to ensure we are calling the original entries function here?
> 
> That is right. We need to use %_CallFunction(this.value_, builtins.MapEntries)
> to make sure we do not call a monkey-patched version.

Thanks, fixed it.

[aandrey, 2014-07-17 09:42:29]

https://codereview.chromium.org/398513005/diff/10001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/398513005/diff/10001/src/runtime.cc#newcode1722
src/runtime.cc:1722: entries->set(number_of_non_hole_elements * 2, *key);
maybe get rid of multiplications?
entries->set(index++, ...);
entries->set(index++, ...);

https://codereview.chromium.org/398513005/diff/10001/src/runtime.cc#newcode1728
src/runtime.cc:1728: return
*isolate->factory()->NewJSArrayWithElements(entries);
is it guaranteed that (table->NumberOfElements() * 2 ==
number_of_non_hole_elements) here? maybe an ASSERT?

https://codereview.chromium.org/398513005/diff/10001/test/mjsunit/harmony/mir...
File test/mjsunit/harmony/mirror-collections.js (right):

https://codereview.chromium.org/398513005/diff/10001/test/mjsunit/harmony/mir...
test/mjsunit/harmony/mirror-collections.js:61: function
testWeakMapMirrorBeforeGC(mirror) {
could you extract a method out of these two?

[Alexandra Mikhaylova, 2014-07-17 10:21:18]

https://codereview.chromium.org/398513005/diff/10001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/398513005/diff/10001/src/runtime.cc#newcode1722
src/runtime.cc:1722: entries->set(number_of_non_hole_elements * 2, *key);
On 2014/07/17 09:42:28, aandrey wrote:
> maybe get rid of multiplications?
> entries->set(index++, ...);
> entries->set(index++, ...);

Done.

https://codereview.chromium.org/398513005/diff/10001/src/runtime.cc#newcode1728
src/runtime.cc:1728: return
*isolate->factory()->NewJSArrayWithElements(entries);
On 2014/07/17 09:42:28, aandrey wrote:
> is it guaranteed that (table->NumberOfElements() * 2 ==
> number_of_non_hole_elements) here? maybe an ASSERT?

It should be, added an ASSERT.

https://codereview.chromium.org/398513005/diff/10001/test/mjsunit/harmony/mir...
File test/mjsunit/harmony/mirror-collections.js (right):

https://codereview.chromium.org/398513005/diff/10001/test/mjsunit/harmony/mir...
test/mjsunit/harmony/mirror-collections.js:61: function
testWeakMapMirrorBeforeGC(mirror) {
On 2014/07/17 09:42:28, aandrey wrote:
> could you extract a method out of these two?

Done.

[aandrey, 2014-07-17 12:06:31]

lgtm

[Yang, 2014-07-17 14:10:31]

On 2014/07/17 12:06:31, aandrey wrote:
> lgtm

lgtm.

[Alexandra Mikhaylova, 2014-07-17 14:27:26]

On 2014/07/17 14:10:31, Yang wrote:
> On 2014/07/17 12:06:31, aandrey wrote:
> > lgtm
> 
> lgtm.

Yang, could you please land it then?

[Yang, 2014-07-17 15:08:11]

Committed patchset #4 manually as r22452 (presubmit successful).

[Alexandra Mikhaylova, 2014-07-18 15:50:42]

Fixed the test, the problem was in GC being called before it was expected in the
test.

[Yang, 2014-07-21 07:56:23]

On 2014/07/18 15:50:42, Alexandra Mikhaylova wrote:
> Fixed the test, the problem was in GC being called before it was expected in
the
> test.

lgtm.

[Yang, 2014-07-21 08:07:13]

Committed patchset #6 manually as r22490 (presubmit successful).