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#oldcode26
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#oldcode44
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#newcode76
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#newcode80
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#newcode95
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#newcode103
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#newcode210
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#newcode16298
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-hash.js
File test/mjsunit/global-hash.js (right):
https://codereview.chromium.org/1149863005/diff/40001/test/mjsunit/global-hash.js#newcode1
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-hash.js#newcode41
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".
https://codereview.chromium.org/1149863005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
