Use a private own symbol instead of a hidden property for hash codes

Use a private own symbol instead of a hidden property for hash codes

R=adamk@chromium.org, verwaest@chromium.org
BUG=

[Erik Corry, 2015-05-14 12:00:56]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2015-05-14 12:00:56]

On my microbenchmark this doubles the speed of common operations on ES6 Map.  I
think it's a simplification too.

Before (times in ms):

Read 514
Update 1083
Set/delete 2075

After:

Read 231
Update 484
Set/delete 1619



The microbenchmark:

function MyKey() {
  this.x = "foo";
}

function setup(key) {
  var m = new Map();
  m.set(key, 42);
  for (var i = 0; i < 20; i++) {
    m.set(new MyKey(), 4);
  }
  return m;
}

function read(map, key) {
  for (var i = 0; i < 10000000; i++) {
    if (map.get(key) != 42) {
      blowup();
    }
  }
}

function update(map, key) {
  for (var i = 0; i < 5000000; i++) {
    map.set(key, 41);
    if (map.get(key) != 41) {
      blowup();
    }
    map.set(key, 42);
    if (map.get(key) != 42) {
      blowup();
    }
  }
}

function setdelete(map, key) {
  for (var i = 0; i < 5000000; i++) {
    map.delete(key, 41);
    if (map.get(key) != undefined) {
      blowup();
    }
    map.set(key, 42);
    if (map.get(key) != 42) {
      blowup();
    }
  }
}

var key = new MyKey();
var map = setup(key);

var start = new Date();
read(map, key);
var end = new Date();
print("Read " + (end - start));

var start = new Date();
update(map, key);
var end = new Date();
print("Update " + (end - start));

var start = new Date();
setdelete(map, key);
var end = new Date();
print("Set/delete " + (end - start));

[arv (Not doing code reviews), 2015-05-14 13:16:17]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-05-14 13:16:17]

High level comments:

1. We have a perf test for maps. We should add a new case for objects as keys.
2. I assume the change to Isolate::IsInternallyUsedPropertyName makes us able to
add this property to non extensible objects? (Needs tests?)
3. One drawback with this is that adding an object to a map now causes a Map
transition and your previously monomorphic code becomes polymorphic.

[Erik Corry, 2015-05-14 13:17:59]

Surely it always caused a map transition. How else can you add the hidden
property?

The change was to make an already existing test pass, but I can add another.

[adamk, 2015-05-14 15:01:28]

This looks great, I didn't realize how easy it was to add properties to a
non-extensible object.

I'm on vacation through Monday, so won't get back to this until next week. The
only thing that I'm worried about is the question about the global below.

https://codereview.chromium.org/1142493002/diff/1/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/1142493002/diff/1/src/collection.js#newcode85
src/collection.js:85: if (IS_SPEC_OBJECT(key)) {
You might need a TODO here for handling proxies appropriately.

Also, will this get the wrong value for the JSGlobalProxy?

https://codereview.chromium.org/1142493002/diff/1/src/collection.js#newcode86
src/collection.js:86: var hash = key[hash_code_symbol];
We've been using a GET_PRIVATE() for this as a matter of style.

https://codereview.chromium.org/1142493002/diff/1/src/collection.js#newcode87
src/collection.js:87: if (hash == void 0) {
IS_UNDEFINED(hash)

https://codereview.chromium.org/1142493002/diff/1/src/collection.js#newcode89
src/collection.js:89: key[hash_code_symbol] = hash;
SET_PRIVATE()

[Erik Corry, 2015-05-14 15:42:19]

https://codereview.chromium.org/1142493002/diff/1/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/1142493002/diff/1/src/collection.js#newcode85
src/collection.js:85: if (IS_SPEC_OBJECT(key)) {
On 2015/05/14 15:01:27, adamk wrote:
> You might need a TODO here for handling proxies appropriately.
> 
> Also, will this get the wrong value for the JSGlobalProxy?

Probably.  What's the deal with the JSGlobalProxy?

[arv (Not doing code reviews), 2015-05-14 19:58:54]

https://codereview.chromium.org/1142493002/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1142493002/diff/1/src/isolate.cc#newcode791
src/isolate.cc:791: return name == heap()->hidden_string();
Does this one need to be updated too?

[Toon Verwaest, 2015-05-15 12:24:10]

https://codereview.chromium.org/1142493002/diff/1/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/1142493002/diff/1/src/collection.js#newcode85
src/collection.js:85: if (IS_SPEC_OBJECT(key)) {
On 2015/05/14 15:42:19, Erik Corry wrote:
> On 2015/05/14 15:01:27, adamk wrote:
> > You might need a TODO here for handling proxies appropriately.
> > 
> > Also, will this get the wrong value for the JSGlobalProxy?
> 
> Probably.  What's the deal with the JSGlobalProxy?

Due to split-object the global proxy points to the current global object, which
is a single object from the POV of the user. The global proxy represents an
iframe, whereas the global object represents a page in the iframe. If the iframe
navigates, the page changes, and hence the link from the global proxy to the
global object.

Access to the global proxy always relays the access to the global object. So if
you do window[hash_code_symbol], you'll get the hash-code from the current page.
That doesn't remain stable across navigation.

After navigation you won't be able to retrieve values associated with the iframe
anymore (unless there's a hash collision between the 2 pages).

[adamk, 2015-05-19 17:39:38]

https://codereview.chromium.org/1142493002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1142493002/diff/1/src/objects.cc#newcode5039
src/objects.cc:5039: return JSGlobalProxy::cast(this)->hash();
To expand slightly on Toon's explanation, here's the codepath we used to go down
for the identity hash of the JSGlobalProxy.

[Erik Corry Chromium.org, 2015-05-21 11:13:43]

On 2015/05/19 17:39:38, adamk wrote:
> https://codereview.chromium.org/1142493002/diff/1/src/objects.cc
> File src/objects.cc (right):
> 
> https://codereview.chromium.org/1142493002/diff/1/src/objects.cc#newcode5039
> src/objects.cc:5039: return JSGlobalProxy::cast(this)->hash();
> To expand slightly on Toon's explanation, here's the codepath we used to go
down
> for the identity hash of the JSGlobalProxy.

Moved this to a new issue, sorry: https://codereview.chromium.org/1149863005/

[Toon Verwaest, 2015-12-08 20:34:04]

Description was changed from

==========
Use a private own symbol instead of a hidden property for hash codes

R=adamk@chromium.org, verwaest@chromium.org
BUG=
==========

to

==========
Use a private own symbol instead of a hidden property for hash codes

R=adamk@chromium.org, verwaest@chromium.org
BUG=
==========