Move hash code from hidden string to a private symbol

Move hash code from hidden string to a private symbol

* Hash code is now just done with a private own symbol instead of the hidden string, which predates symbols.
* In the long run we should do all hidden properties this way and get rid of the
hidden magic 0-length string with the zero hash code.  The advantages include
less complexity and being able to do things from JS in a natural way.
* Initially, the performance of weak set regressed, because it's a little harder
to do the lookup in C++.  Instead of heroics in C++ to make things faster I
moved some functionality into JS and got the performance back. JS is supposed to be good at looking up named properties on objects.
* This also changes hash codes of Smis so that they are always Smis.

Performance figures are in the comments to the code review.  Summary: Most of js-perf-test/Collections is neutral.  Set and Map with object keys are 40-50% better.  WeakMap is -5% and WeakSet is +9%.  After the measurements, I fixed global proxies, which cost 1% on most tests and 5% on the weak ones :-(.

In the code review comments is a patch with an example of the heroics we could do in C++ to make lookup faster (I hope we don't have to do this.  Instead of checking for the property, then doing a new lookup to insert it, we could do one lookup and handle the addition immediately).  With the current benchmarks above this buys us nothing, but if we go back to doing more lookups in C++ instead of in stubs and JS then it's a win.

In a similar vein we could give the magic zero hash code to the hash code
symbol.  Then when we look up the hash code we would sometimes see the table
with all the hidden properties.  This dual use of the field for either the hash
code or the table with all hidden properties and the hash code is rather ugly,
and this CL gets rid of it.  I'd be loath to bring it back.  On the benchmarks quoted above it's slightly slower than moving the hash code lookup to JS like in this CL.

One worry is that the benchmark results above are more monomorphic than real
world code, so may be overstating the performance benefits of moving to JS.  I
think this is part of a general issue we have with handling polymorphic code in
JS and any solutions there will benefit this solution, which boils down to
regular property access. Any improvement there will lift all boats.

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

Committed: https://crrev.com/eca5b5d7abc0a9028cb9832087fbf2ed59dadf92
Cr-Commit-Position: refs/heads/master@{#28622}

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

erikcorry@chromium.org changed reviewers:
+ verwaest@chromium.org
- toon@chromium.org

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

Here is a new attempt, based on the previous one.  Summary:

Global proxy is still unresolved, but ES6 proxies are no more broken than they
were.

* Hash code is just done with a hidden own symbol
* In the long run we should do all hidden properties this way and get rid of the
hidden magic 0-length string with the zero hash code.  The advantages include
less complexity and being able to do things from JS in a natural way.  We
actually hide foo[symbol] = bar behind PRIVATE_SET(foo, symbol, bar), which I
think makes it look more mysterious than it is.
* Initially, the performance of weak set regressed, because it's a little harder
to do the lookup in C++.  Instead of heroics in C++ to make things faster I
moved some functionality into JS and got the performance back. JS is supposed to
be good at looking up named properties on objects.

Before:
Map-Smi-Collections(Score): 343457
Map-String-Collections(Score): 304504
Map-Object-Collections(Score): 117353
Map-Iteration-Collections(Score): 212760
Set-Smi-Collections(Score): 369738
Set-String-Collections(Score): 336437
Set-Object-Collections(Score): 132722
Set-Iteration-Collections(Score): 229770
WeakMap-Collections(Score): 97736
WeakSet-Collections(Score): 106390

After:
Map-Smi-Collections(Score): 342210         0%
Map-String-Collections(Score): 303118      0%
Map-Object-Collections(Score): 186080     +59%
Map-Iteration-Collections(Score): 217530  +2%
Set-Smi-Collections(Score): 377493           +2%
Set-String-Collections(Score): 344410        +2%
Set-Object-Collections(Score): 182470       +38%
Set-Iteration-Collections(Score): 222360    - 3%
WeakMap-Collections(Score): 92364           -5%
WeakSet-Collections(Score): 115826           +9%

Below is a patch with an example of the heroics we could do in C++ to make
lookup faster (I hope we don't have to do this.  Instead of checking for the
property, then doing a new lookup to insert it, we could do one lookup and
handle the addition immediately).  With the current benchmarks above this buys
us nothing, but if we go back to doing more lookups in C++ instead of in stubs
and JS then it's a win.

@@ -5069,12 +5069,23 @@ Handle<Smi>
JSObject::GetOrCreateIdentityHash(Handle<JSObject> object) {
 
   Isolate* isolate = object->GetIsolate();
 
-  Handle<Object> maybe_hash(object->GetIdentityHash(), isolate);
-  if (maybe_hash->IsSmi()) return Handle<Smi>::cast(maybe_hash);
+  Handle<Name> name(isolate->heap()->hash_code_symbol(), isolate);
+  LookupIterator it(object, name, LookupIterator::OWN_SKIP_INTERCEPTOR);
+  if (it.state() != LookupIterator::NOT_FOUND) {
+    return Handle<Smi>::cast(it.GetDataValue());
+  }
 
   Handle<Smi> hash(GenerateIdentityHash(isolate), isolate);
-  Handle<Name> hash_code_symbol(isolate->heap()->hash_code_symbol());
-  JSObject::AddProperty(object, hash_code_symbol, hash, NONE);
+  it.PrepareTransitionToDataProperty(hash, NONE,
CERTAINLY_NOT_STORE_FROM_KEYED);
+  DCHECK (it.state() == LookupIterator::TRANSITION);
+  it.ApplyTransitionToDataProperty();
+
+  if (object->map()->is_dictionary_map()) {
+    JSObject::AddSlowProperty(object, name, hash, NONE);
+  } else {
+    it.WriteDataValue(hash);
+  }
+
   return hash;
 }
 
In a similar vein we could give the magic zero hash code to the hash code
symbol.  Then when we look up the hash code we would sometimes see the table
with all the hidden properties.  This dual use of the field for either the hash
code or the table with all hidden properties and the hash code is rather ugly,
and this CL gets rid of it.  I'd be loath to bring it back.  I do have the code
for this in a branch.  On the benchmarks quoted above it's slightly slower than
this solution.

One worry is that the benchmark results above are more monomorphic than real
world code, so may be overstating the performance benefits of moving to JS.  I
think this is part of a general issue we have with handling polymorphic code in
JS and any solutions there will benefit this solution, which boils down to
regular property access. Any improvement there will lift all boats.

[adamk, 2015-05-21 19:11:33]

Thanks for the detailed writeup, can you move the important bits of that into
the CL description for posterity?

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js
File src/collection.js (left):

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#oldco...
src/collection.js:26: for (var entry = HashToEntry(table, hash, numBuckets);
Why did this get rewritten as a while loop? From my inspection the only
difference I see is extra calls to KEY_AT and extra comparisons.

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#oldco...
src/collection.js:44: for (var entry = HashToEntry(table, hash, numBuckets);
Same question down here.

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newcode5
src/collection.js:5: var $gethash;
Rename to $getHash

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newcode6
src/collection.js:6: var $getexistinghash;
and $getExistingHash

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newco...
src/collection.js:76: return hash & 0x3fffffff;
Why the extra &? This used to be equivalent to ComputeIntegerHash() in utils.h.

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newco...
src/collection.js:80: var hash_code_symbol = GLOBAL_PRIVATE("hash_code_symbol");
Please use JS style here ("hashCodeSymbol").

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newco...
src/collection.js:95: return %GenericHash(key);
%GenericHash calls GetOrCreateHash, which seems wrong for this case. Maybe add a
new runtime function that calls GetHash() instead (though maybe we don't care
much about this case).

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newco...
src/collection.js:103: if (IS_GLOBAL(key)) {
I think this branch is currently unreachable due to the above.

https://codereview.chromium.org/1149863005/diff/40001/src/math.js
File src/math.js (right):

https://codereview.chromium.org/1149863005/diff/40001/src/math.js#newcode374
src/math.js:374: $intrandom = MathRandomRaw;
One more camelCase nit here...$intRandom?

https://codereview.chromium.org/1149863005/diff/40001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/1149863005/diff/40001/src/object-observe.js#n...
src/object-observe.js:210: return
%WeakCollectionGet(GetObservationStateJS().objectInfoMap, object,
$gethash(object));
Style nit: over 80 characters here and below.

https://codereview.chromium.org/1149863005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1149863005/diff/40001/src/objects.cc#newcode5106
src/objects.cc:5106: DCHECK(!inline_value->IsSmi());
I don't think this DCHECK will make much sense in future. If the slow check
inside ObjectHashTable::cast() isn't sufficient, you could make this

DCHECK(inline_value->IsUndefined() || inline_value->IsObjectHashTable());

https://codereview.chromium.org/1149863005/diff/40001/src/objects.cc#newcode5166
src/objects.cc:5166: DCHECK(!inline_value->IsSmi());
Same thing here, this DCHECK will be weird in the future.

https://codereview.chromium.org/1149863005/diff/40001/src/objects.cc#newcode1...
src/objects.cc:16298: int entry = FindEntry(isolate, key,
Smi::cast(hash)->value());
Can you just delegate to the two-arg form of Lookup() to avoid duplication here
(as you do for Put and Remove below)?

https://codereview.chromium.org/1149863005/diff/40001/test/mjsunit/global-has...
File test/mjsunit/global-hash.js (right):

https://codereview.chromium.org/1149863005/diff/40001/test/mjsunit/global-has...
test/mjsunit/global-hash.js:1: // Copyright 2015 the V8 project authors. All
rights reserved.
Please use the short license header for new tests.

https://codereview.chromium.org/1149863005/diff/40001/test/mjsunit/global-has...
test/mjsunit/global-hash.js:41: // Hopefully still findable using the JS hash
code.
This test is good, but can you also add a cctest that detaches the proxy and
attaches to a new global to show that the hashcode remains the same across
"navigations".

[Erik Corry, 2015-05-21 20:19:01]

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

[Erik Corry, 2015-05-21 20:19:02]

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js
File src/collection.js (left):

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#oldco...
src/collection.js:26: for (var entry = HashToEntry(table, hash, numBuckets);
On 2015/05/21 19:11:32, adamk wrote:
> Why did this get rewritten as a while loop? From my inspection the only
> difference I see is extra calls to KEY_AT and extra comparisons.

The idea is to avoid testing the key for Nan-ness in the case where the first
probe fails or hits.

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#oldco...
src/collection.js:44: for (var entry = HashToEntry(table, hash, numBuckets);
On 2015/05/21 19:11:32, adamk wrote:
> Same question down here.

Same answer :-)

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newco...
src/collection.js:95: return %GenericHash(key);
On 2015/05/21 19:11:32, adamk wrote:
> %GenericHash calls GetOrCreateHash, which seems wrong for this case. Maybe add
a
> new runtime function that calls GetHash() instead (though maybe we don't care
> much about this case).

I think we don't care much about this case, it's only proxies.  And symbols, but
they should be born with a hash code (and it would be nice to handle them above
in the string clause, which would then be a name clause).

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newco...
src/collection.js:103: if (IS_GLOBAL(key)) {
On 2015/05/21 19:11:32, adamk wrote:
> I think this branch is currently unreachable due to the above.

Good point.

I think a better solution would be for global objects to be born with a hash
code, and it should always be 1 (or 42).  That way we don't have to keep track
of them specially, and that would give us back our 5% on the weak maps.

https://codereview.chromium.org/1149863005/diff/40001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/1149863005/diff/40001/src/object-observe.js#n...
src/object-observe.js:210: return
%WeakCollectionGet(GetObservationStateJS().objectInfoMap, object,
$gethash(object));
On 2015/05/21 19:11:32, adamk wrote:
> Style nit: over 80 characters here and below.

I've come to rely too much on git cl format.

[Erik Corry, 2015-05-22 06:20:25]

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newcode5
src/collection.js:5: var $gethash;
On 2015/05/21 19:11:32, adamk wrote:
> Rename to $getHash

Done.

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newcode6
src/collection.js:6: var $getexistinghash;
On 2015/05/21 19:11:32, adamk wrote:
> and $getExistingHash

Done.

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newco...
src/collection.js:76: return hash & 0x3fffffff;
On 2015/05/21 19:11:32, adamk wrote:
> Why the extra &? This used to be equivalent to ComputeIntegerHash() in
utils.h.

To make sure it's always a Smi.  I changed ComputeIntegerHash to match.

https://codereview.chromium.org/1149863005/diff/40001/src/collection.js#newco...
src/collection.js:80: var hash_code_symbol = GLOBAL_PRIVATE("hash_code_symbol");
On 2015/05/21 19:11:32, adamk wrote:
> Please use JS style here ("hashCodeSymbol").

Done.

https://codereview.chromium.org/1149863005/diff/40001/src/math.js
File src/math.js (right):

https://codereview.chromium.org/1149863005/diff/40001/src/math.js#newcode374
src/math.js:374: $intrandom = MathRandomRaw;
On 2015/05/21 19:11:32, adamk wrote:
> One more camelCase nit here...$intRandom?

Done.

https://codereview.chromium.org/1149863005/diff/40001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/1149863005/diff/40001/src/object-observe.js#n...
src/object-observe.js:210: return
%WeakCollectionGet(GetObservationStateJS().objectInfoMap, object,
$gethash(object));
On 2015/05/21 20:19:02, Erik Corry wrote:
> On 2015/05/21 19:11:32, adamk wrote:
> > Style nit: over 80 characters here and below.
> 
> I've come to rely too much on git cl format.

Done.

https://codereview.chromium.org/1149863005/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1149863005/diff/40001/src/objects.cc#newcode5106
src/objects.cc:5106: DCHECK(!inline_value->IsSmi());
On 2015/05/21 19:11:32, adamk wrote:
> I don't think this DCHECK will make much sense in future. If the slow check
> inside ObjectHashTable::cast() isn't sufficient, you could make this
> 
> DCHECK(inline_value->IsUndefined() || inline_value->IsObjectHashTable());

The cast is sufficient, removed.

https://codereview.chromium.org/1149863005/diff/40001/src/objects.cc#newcode5166
src/objects.cc:5166: DCHECK(!inline_value->IsSmi());
On 2015/05/21 19:11:32, adamk wrote:
> Same thing here, this DCHECK will be weird in the future.

Removed.

https://codereview.chromium.org/1149863005/diff/40001/src/objects.cc#newcode1...
src/objects.cc:16298: int entry = FindEntry(isolate, key,
Smi::cast(hash)->value());
On 2015/05/21 19:11:32, adamk wrote:
> Can you just delegate to the two-arg form of Lookup() to avoid duplication
here
> (as you do for Put and Remove below)?

Created a three-argument form to avoid looking up the isolate yet again.

https://codereview.chromium.org/1149863005/diff/40001/test/mjsunit/global-has...
File test/mjsunit/global-hash.js (right):

https://codereview.chromium.org/1149863005/diff/40001/test/mjsunit/global-has...
test/mjsunit/global-hash.js:1: // Copyright 2015 the V8 project authors. All
rights reserved.
On 2015/05/21 19:11:32, adamk wrote:
> Please use the short license header for new tests.

Done.

https://codereview.chromium.org/1149863005/diff/40001/test/mjsunit/global-has...
test/mjsunit/global-hash.js:41: // Hopefully still findable using the JS hash
code.
On 2015/05/21 19:11:32, adamk wrote:
> This test is good, but can you also add a cctest that detaches the proxy and
> attaches to a new global to show that the hashcode remains the same across
> "navigations".

We already had this in test-api.cc, but it added the hash code from C++ so it
didn't test the case where we added from JS.  FIxed to do both.

[Erik Corry Chromium.org, 2015-05-22 07:37:38]

The CQ bit was checked by erikcorry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-22 07:37:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149863005/80001

[commit-bot: I haz the power, 2015-05-22 07:50:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-22 07:50:22]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[Erik Corry Chromium.org, 2015-05-22 09:54:23]

The CQ bit was checked by erikcorry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-22 09:54:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149863005/100001

[commit-bot: I haz the power, 2015-05-22 10:25:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-22 10:25:19]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-05-22 17:58:49]

lgtm

Still think it'd be good for Toon to sign off as well.

[paul.l..., 2015-05-25 05:42:06]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-05-25 05:42:07]

MIPS code lgtm, can be written using macro-asm as And(reg0, reg0,
Operand(0x3fffffff)); for clarity. Same code can be used for MIPS64.

[paul.l..., 2015-05-25 05:45:00]

adding v8-mips-ports group, since i am out of office.

[Erik Corry Chromium.org, 2015-05-26 07:36:14]

The CQ bit was checked by erikcorry@chromium.org to run a CQ dry run

[Erik Corry Chromium.org, 2015-05-26 07:36:15]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1149863005/#ps120001
(title: "Fix MIPS")

[commit-bot: I haz the power, 2015-05-26 07:36:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149863005/120001

[commit-bot: I haz the power, 2015-05-26 07:38:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-26 07:38:22]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/709)

[Toon Verwaest, 2015-05-26 09:06:27]

https://codereview.chromium.org/1149863005/diff/120001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/1149863005/diff/120001/src/collection.js#newc...
src/collection.js:86: function GetExistingHash(key) {
That's a pretty deceiving name since it does compute a new hash for
non-jsobjects (and global proxies)

https://codereview.chromium.org/1149863005/diff/120001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1149863005/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:3189:
JSObject::SetProperty(Handle<JSObject>::cast(private_intern_table),  \
You probably want to use AddProperty here

https://codereview.chromium.org/1149863005/diff/120001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1149863005/diff/120001/src/isolate.cc#newcode...
src/isolate.cc:2529: JSObject::NormalizeProperties(obj,
KEEP_INOBJECT_PROPERTIES, 8,
Why do you want to keep inobject properties?

https://codereview.chromium.org/1149863005/diff/120001/src/isolate.cc#newcode...
src/isolate.cc:2531: JSObject::SetProperty(registry, name, obj,
STRICT).Assert();
AddProperty

https://codereview.chromium.org/1149863005/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1149863005/diff/120001/src/objects.cc#newcode...
src/objects.cc:5044: void JSObject::SetIdentityHash(Handle<JSObject> object,
Handle<Smi> hash) {
Perhaps you should rename this to AddIdentityHash to avoid confusion. Or
InitializeIdentityHash or so.

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
File src/runtime/runtime-symbol.cc (right):

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
src/runtime/runtime-symbol.cc:40: Handle<Symbol> symbol =
isolate->factory()->NewPrivateOwnSymbol(name);
return *isolate-> ...

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
src/runtime/runtime-symbol.cc:53: isolate, privates,
Object::GetPropertyOrElement(registry, part));
GetProperty, part is not an element.

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
src/runtime/runtime-symbol.cc:56: isolate, symbol,
Object::GetPropertyOrElement(privates, name));
Same here. Given that SetProperty is used below, it wouldn't even work if we'd
look up "name" as an element here, but store it as a named property with
SetProperty (instead of SetElement).

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
src/runtime/runtime-symbol.cc:60:
JSObject::SetProperty(Handle<JSObject>::cast(privates), name, symbol,
AddProperty

https://codereview.chromium.org/1149863005/diff/120001/test/mjsunit/global-ha...
File test/mjsunit/global-hash.js (right):

https://codereview.chromium.org/1149863005/diff/120001/test/mjsunit/global-ha...
test/mjsunit/global-hash.js:7: assertTrue(typeof(global) === "object");  //
Global object.
assertEquals("object", typeof global);

[Erik Corry Chromium.org, 2015-05-26 09:51:32]

https://codereview.chromium.org/1149863005/diff/120001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/1149863005/diff/120001/src/collection.js#newc...
src/collection.js:86: function GetExistingHash(key) {
On 2015/05/26 09:06:26, Toon Verwaest wrote:
> That's a pretty deceiving name since it does compute a new hash for
> non-jsobjects (and global proxies)

Left as is after offline discussion.

https://codereview.chromium.org/1149863005/diff/120001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1149863005/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:3189:
JSObject::SetProperty(Handle<JSObject>::cast(private_intern_table),  \
On 2015/05/26 09:06:26, Toon Verwaest wrote:
> You probably want to use AddProperty here

Done.

https://codereview.chromium.org/1149863005/diff/120001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1149863005/diff/120001/src/isolate.cc#newcode...
src/isolate.cc:2529: JSObject::NormalizeProperties(obj,
KEEP_INOBJECT_PROPERTIES, 8,
On 2015/05/26 09:06:26, Toon Verwaest wrote:
> Why do you want to keep inobject properties?

Done.

https://codereview.chromium.org/1149863005/diff/120001/src/isolate.cc#newcode...
src/isolate.cc:2531: JSObject::SetProperty(registry, name, obj,
STRICT).Assert();
On 2015/05/26 09:06:26, Toon Verwaest wrote:
> AddProperty

Done.

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
File src/runtime/runtime-symbol.cc (right):

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
src/runtime/runtime-symbol.cc:40: Handle<Symbol> symbol =
isolate->factory()->NewPrivateOwnSymbol(name);
On 2015/05/26 09:06:27, Toon Verwaest wrote:
> return *isolate-> ...

Done.

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
src/runtime/runtime-symbol.cc:53: isolate, privates,
Object::GetPropertyOrElement(registry, part));
On 2015/05/26 09:06:27, Toon Verwaest wrote:
> GetProperty, part is not an element.

Done.

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
src/runtime/runtime-symbol.cc:56: isolate, symbol,
Object::GetPropertyOrElement(privates, name));
On 2015/05/26 09:06:27, Toon Verwaest wrote:
> Same here. Given that SetProperty is used below, it wouldn't even work if we'd
> look up "name" as an element here, but store it as a named property with
> SetProperty (instead of SetElement).

Done.

https://codereview.chromium.org/1149863005/diff/120001/src/runtime/runtime-sy...
src/runtime/runtime-symbol.cc:60:
JSObject::SetProperty(Handle<JSObject>::cast(privates), name, symbol,
On 2015/05/26 09:06:27, Toon Verwaest wrote:
> AddProperty

Done.

https://codereview.chromium.org/1149863005/diff/120001/test/mjsunit/global-ha...
File test/mjsunit/global-hash.js (right):

https://codereview.chromium.org/1149863005/diff/120001/test/mjsunit/global-ha...
test/mjsunit/global-hash.js:7: assertTrue(typeof(global) === "object");  //
Global object.
On 2015/05/26 09:06:27, Toon Verwaest wrote:
> assertEquals("object", typeof global);

Done.

[Erik Corry Chromium.org, 2015-05-26 10:00:02]

The CQ bit was checked by erikcorry@chromium.org to run a CQ dry run

[Erik Corry Chromium.org, 2015-05-26 10:00:02]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1149863005/#ps140001
(title: "Feedback from Toon")

[commit-bot: I haz the power, 2015-05-26 10:00:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149863005/140001

[commit-bot: I haz the power, 2015-05-26 10:01:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-26 10:01:56]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/721)

[Erik Corry Chromium.org, 2015-05-26 10:26:11]

The CQ bit was checked by erikcorry@chromium.org to run a CQ dry run

[Erik Corry Chromium.org, 2015-05-26 10:26:11]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1149863005/#ps180001
(title: "Fix merge")

[commit-bot: I haz the power, 2015-05-26 10:26:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149863005/180001

[commit-bot: I haz the power, 2015-05-26 10:53:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-26 10:53:26]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2015-05-26 11:22:54]

lgtm

[Erik Corry Chromium.org, 2015-05-26 11:23:59]

The CQ bit was checked by erikcorry@chromium.org

[commit-bot: I haz the power, 2015-05-26 11:24:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149863005/180001

[commit-bot: I haz the power, 2015-05-26 11:26:35]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-05-26 11:26:49]

Patchset 10 (id:??) landed as
https://crrev.com/eca5b5d7abc0a9028cb9832087fbf2ed59dadf92
Cr-Commit-Position: refs/heads/master@{#28622}