Skip to content

Commit eca5b5d

Browse files
erikcorryCommit bot
erikcorry
authored and
Commit bot
committed
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= Review URL: https://codereview.chromium.org/1149863005 Cr-Commit-Position: refs/heads/master@{#28622}
1 parent b53c35a commit eca5b5d

30 files changed

Lines changed: 340 additions & 181 deletions

‎src/api.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2540,7 +2540,8 @@ void NativeWeakMap::Set(Handle<Value> v8_key, Handle<Value> v8_value) {
25402540
DCHECK(false);
25412541
return;
25422542
}
2543-
i::Runtime::WeakCollectionSet(weak_collection, key, value);
2543+
int32_t hash = i::Object::GetOrCreateHash(isolate, key)->value();
2544+
i::Runtime::WeakCollectionSet(weak_collection, key, value, hash);
25442545
}
25452546

25462547

‎src/arm/macro-assembler-arm.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1559,6 +1559,7 @@ void MacroAssembler::GetNumberHash(Register t0, Register scratch) {
15591559
add(t0, t0, scratch);
15601560
// hash = hash ^ (hash >> 16);
15611561
eor(t0, t0, Operand(t0, LSR, 16));
1562+
bic(t0, t0, Operand(0xc0000000u));
15621563
}
15631564

15641565

‎src/arm64/macro-assembler-arm64.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3928,6 +3928,7 @@ void MacroAssembler::GetNumberHash(Register key, Register scratch) {
39283928
Add(key, key, scratch);
39293929
// hash = hash ^ (hash >> 16);
39303930
Eor(key, key, Operand(key, LSR, 16));
3931+
Bic(key, key, Operand(0xc0000000u));
39313932
}
39323933

39333934

‎src/collection.js‎

Lines changed: 54 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22
// Use of this source code is governed by a BSD-style license that can be
33
// found in the LICENSE file.
44

5-
(function(global, utils) {
5+
var $getHash;
6+
var $getExistingHash;
67

8+
(function(global, utils) {
79
"use strict";
810

911
%CheckIsBootstrapping();
@@ -14,6 +16,11 @@
1416
var GlobalMap = global.Map;
1517
var GlobalObject = global.Object;
1618
var GlobalSet = global.Set;
19+
var IntRandom;
20+
21+
utils.Import(function(from) {
22+
IntRandom = from.IntRandom;
23+
});
1724

1825
var NumberIsNaN;
1926

@@ -31,35 +38,39 @@ function HashToEntry(table, hash, numBuckets) {
3138

3239

3340
function SetFindEntry(table, numBuckets, key, hash) {
41+
var entry = HashToEntry(table, hash, numBuckets);
42+
if (entry === NOT_FOUND) return entry;
43+
var candidate = ORDERED_HASH_SET_KEY_AT(table, entry, numBuckets);
44+
if (key === candidate) return entry;
3445
var keyIsNaN = NumberIsNaN(key);
35-
for (var entry = HashToEntry(table, hash, numBuckets);
36-
entry !== NOT_FOUND;
37-
entry = ORDERED_HASH_SET_CHAIN_AT(table, entry, numBuckets)) {
38-
var candidate = ORDERED_HASH_SET_KEY_AT(table, entry, numBuckets);
39-
if (key === candidate) {
40-
return entry;
41-
}
46+
while (true) {
4247
if (keyIsNaN && NumberIsNaN(candidate)) {
4348
return entry;
4449
}
50+
entry = ORDERED_HASH_SET_CHAIN_AT(table, entry, numBuckets);
51+
if (entry === NOT_FOUND) return entry;
52+
candidate = ORDERED_HASH_SET_KEY_AT(table, entry, numBuckets);
53+
if (key === candidate) return entry;
4554
}
4655
return NOT_FOUND;
4756
}
4857
%SetForceInlineFlag(SetFindEntry);
4958

5059

5160
function MapFindEntry(table, numBuckets, key, hash) {
61+
var entry = HashToEntry(table, hash, numBuckets);
62+
if (entry === NOT_FOUND) return entry;
63+
var candidate = ORDERED_HASH_MAP_KEY_AT(table, entry, numBuckets);
64+
if (key === candidate) return entry;
5265
var keyIsNaN = NumberIsNaN(key);
53-
for (var entry = HashToEntry(table, hash, numBuckets);
54-
entry !== NOT_FOUND;
55-
entry = ORDERED_HASH_MAP_CHAIN_AT(table, entry, numBuckets)) {
56-
var candidate = ORDERED_HASH_MAP_KEY_AT(table, entry, numBuckets);
57-
if (key === candidate) {
58-
return entry;
59-
}
66+
while (true) {
6067
if (keyIsNaN && NumberIsNaN(candidate)) {
6168
return entry;
6269
}
70+
entry = ORDERED_HASH_MAP_CHAIN_AT(table, entry, numBuckets);
71+
if (entry === NOT_FOUND) return entry;
72+
candidate = ORDERED_HASH_MAP_KEY_AT(table, entry, numBuckets);
73+
if (key === candidate) return entry;
6374
}
6475
return NOT_FOUND;
6576
}
@@ -75,12 +86,13 @@ function ComputeIntegerHash(key, seed) {
7586
hash = hash ^ (hash >>> 4);
7687
hash = (hash * 2057) | 0; // hash = (hash + (hash << 3)) + (hash << 11);
7788
hash = hash ^ (hash >>> 16);
78-
return hash;
89+
return hash & 0x3fffffff;
7990
}
8091
%SetForceInlineFlag(ComputeIntegerHash);
8192

93+
var hashCodeSymbol = GLOBAL_PRIVATE("hash_code_symbol");
8294

83-
function GetHash(key) {
95+
function GetExistingHash(key) {
8496
if (%_IsSmi(key)) {
8597
return ComputeIntegerHash(key, 0);
8698
}
@@ -89,9 +101,24 @@ function GetHash(key) {
89101
if ((field & 1 /* Name::kHashNotComputedMask */) === 0) {
90102
return field >>> 2 /* Name::kHashShift */;
91103
}
104+
} else if (IS_SPEC_OBJECT(key) && !%_IsJSProxy(key) && !IS_GLOBAL(key)) {
105+
var hash = GET_PRIVATE(key, hashCodeSymbol);
106+
return hash;
92107
}
93108
return %GenericHash(key);
94109
}
110+
%SetForceInlineFlag(GetExistingHash);
111+
112+
113+
function GetHash(key) {
114+
var hash = GetExistingHash(key);
115+
if (IS_UNDEFINED(hash)) {
116+
hash = IntRandom() | 0;
117+
if (hash === 0) hash = 1;
118+
SET_PRIVATE(key, hashCodeSymbol, hash);
119+
}
120+
return hash;
121+
}
95122
%SetForceInlineFlag(GetHash);
96123

97124

@@ -164,7 +191,8 @@ function SetHas(key) {
164191
}
165192
var table = %_JSCollectionGetTable(this);
166193
var numBuckets = ORDERED_HASH_TABLE_BUCKET_COUNT(table);
167-
var hash = GetHash(key);
194+
var hash = GetExistingHash(key);
195+
if (IS_UNDEFINED(hash)) return false;
168196
return SetFindEntry(table, numBuckets, key, hash) !== NOT_FOUND;
169197
}
170198

@@ -176,7 +204,8 @@ function SetDelete(key) {
176204
}
177205
var table = %_JSCollectionGetTable(this);
178206
var numBuckets = ORDERED_HASH_TABLE_BUCKET_COUNT(table);
179-
var hash = GetHash(key);
207+
var hash = GetExistingHash(key);
208+
if (IS_UNDEFINED(hash)) return false;
180209
var entry = SetFindEntry(table, numBuckets, key, hash);
181210
if (entry === NOT_FOUND) return false;
182211

@@ -291,7 +320,8 @@ function MapGet(key) {
291320
}
292321
var table = %_JSCollectionGetTable(this);
293322
var numBuckets = ORDERED_HASH_TABLE_BUCKET_COUNT(table);
294-
var hash = GetHash(key);
323+
var hash = GetExistingHash(key);
324+
if (IS_UNDEFINED(hash)) return UNDEFINED;
295325
var entry = MapFindEntry(table, numBuckets, key, hash);
296326
if (entry === NOT_FOUND) return UNDEFINED;
297327
return ORDERED_HASH_MAP_VALUE_AT(table, entry, numBuckets);
@@ -446,4 +476,8 @@ utils.InstallFunctions(GlobalMap.prototype, DONT_ENUM, [
446476
"forEach", MapForEach
447477
]);
448478

479+
// Expose to the global scope.
480+
$getHash = GetHash;
481+
$getExistingHash = GetExistingHash;
482+
449483
})

‎src/factory.cc‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -705,10 +705,15 @@ Handle<Symbol> Factory::NewPrivateSymbol() {
705705
}
706706

707707

708-
Handle<Symbol> Factory::NewPrivateOwnSymbol() {
708+
Handle<Symbol> Factory::NewPrivateOwnSymbol(Handle<Object> name) {
709709
Handle<Symbol> symbol = NewSymbol();
710710
symbol->set_is_private(true);
711711
symbol->set_is_own(true);
712+
if (name->IsString()) {
713+
symbol->set_name(*name);
714+
} else {
715+
DCHECK(name->IsUndefined());
716+
}
712717
return symbol;
713718
}
714719

‎src/factory.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ class Factory final {
233233
// Create a symbol.
234234
Handle<Symbol> NewSymbol();
235235
Handle<Symbol> NewPrivateSymbol();
236-
Handle<Symbol> NewPrivateOwnSymbol();
236+
Handle<Symbol> NewPrivateOwnSymbol(Handle<Object> name);
237237

238238
// Create a global (but otherwise uninitialized) context.
239239
Handle<Context> NewNativeContext();

‎src/heap/heap.cc‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3060,9 +3060,12 @@ void Heap::CreateInitialObjects() {
30603060

30613061
{
30623062
HandleScope scope(isolate());
3063-
#define SYMBOL_INIT(name) \
3064-
Handle<Symbol> name = factory->NewPrivateOwnSymbol(); \
3065-
roots_[k##name##RootIndex] = *name;
3063+
#define SYMBOL_INIT(name) \
3064+
{ \
3065+
Handle<String> name##d = factory->NewStringFromStaticChars(#name); \
3066+
Handle<Object> symbol(isolate()->factory()->NewPrivateOwnSymbol(name##d)); \
3067+
roots_[k##name##RootIndex] = *symbol; \
3068+
}
30663069
PRIVATE_SYMBOL_LIST(SYMBOL_INIT)
30673070
#undef SYMBOL_INIT
30683071
}
@@ -3171,6 +3174,19 @@ void Heap::CreateInitialObjects() {
31713174
}
31723175

31733176

3177+
void Heap::AddPrivateGlobalSymbols(Handle<Object> private_intern_table) {
3178+
#define ADD_SYMBOL_TO_PRIVATE_INTERN_TABLE(name_arg) \
3179+
{ \
3180+
Handle<Symbol> symbol(Symbol::cast(roots_[k##name_arg##RootIndex])); \
3181+
Handle<String> name_arg##d(String::cast(symbol->name())); \
3182+
JSObject::AddProperty(Handle<JSObject>::cast(private_intern_table), \
3183+
name_arg##d, symbol, NONE); \
3184+
}
3185+
PRIVATE_SYMBOL_LIST(ADD_SYMBOL_TO_PRIVATE_INTERN_TABLE)
3186+
#undef ADD_SYMBOL_TO_PRIVATE_INTERN_TABLE
3187+
}
3188+
3189+
31743190
bool Heap::RootCanBeWrittenAfterInitialization(Heap::RootListIndex root_index) {
31753191
switch (root_index) {
31763192
case kStoreBufferTopRootIndex:

‎src/heap/heap.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,6 @@ namespace internal {
271271
V(illegal_access_string, "illegal access") \
272272
V(cell_value_string, "%cell_value") \
273273
V(illegal_argument_string, "illegal argument") \
274-
V(identity_hash_string, "v8::IdentityHash") \
275274
V(closure_string, "(closure)") \
276275
V(dot_string, ".") \
277276
V(compare_ic_string, "==") \
@@ -294,6 +293,7 @@ namespace internal {
294293
#define PRIVATE_SYMBOL_LIST(V) \
295294
V(nonextensible_symbol) \
296295
V(sealed_symbol) \
296+
V(hash_code_symbol) \
297297
V(frozen_symbol) \
298298
V(nonexistent_symbol) \
299299
V(elements_transition_symbol) \
@@ -1754,6 +1754,8 @@ class Heap {
17541754
// any string when looked up in properties.
17551755
String* hidden_string_;
17561756

1757+
void AddPrivateGlobalSymbols(Handle<Object> private_intern_table);
1758+
17571759
// GC callback function, called before and after mark-compact GC.
17581760
// Allocations in the callback function are disallowed.
17591761
struct GCPrologueCallbackPair {

‎src/ia32/macro-assembler-ia32.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1150,6 +1150,7 @@ void MacroAssembler::GetNumberHash(Register r0, Register scratch) {
11501150
mov(scratch, r0);
11511151
shr(scratch, 16);
11521152
xor_(r0, scratch);
1153+
and_(r0, 0x3fffffff);
11531154
}
11541155

11551156

‎src/isolate.cc‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2522,21 +2522,29 @@ ISOLATE_INIT_ARRAY_LIST(ISOLATE_FIELD_OFFSET)
25222522
#endif
25232523

25242524

2525+
Handle<JSObject> Isolate::SetUpSubregistry(Handle<JSObject> registry,
2526+
Handle<Map> map, const char* cname) {
2527+
Handle<String> name = factory()->InternalizeUtf8String(cname);
2528+
Handle<JSObject> obj = factory()->NewJSObjectFromMap(map);
2529+
JSObject::NormalizeProperties(obj, CLEAR_INOBJECT_PROPERTIES, 0,
2530+
"SetupSymbolRegistry");
2531+
JSObject::AddProperty(registry, name, obj, NONE);
2532+
return obj;
2533+
}
2534+
2535+
25252536
Handle<JSObject> Isolate::GetSymbolRegistry() {
25262537
if (heap()->symbol_registry()->IsSmi()) {
25272538
Handle<Map> map = factory()->NewMap(JS_OBJECT_TYPE, JSObject::kHeaderSize);
25282539
Handle<JSObject> registry = factory()->NewJSObjectFromMap(map);
25292540
heap()->set_symbol_registry(*registry);
25302541

2531-
static const char* nested[] = {"for", "for_api", "keyFor", "private_api",
2532-
"private_intern"};
2533-
for (unsigned i = 0; i < arraysize(nested); ++i) {
2534-
Handle<String> name = factory()->InternalizeUtf8String(nested[i]);
2535-
Handle<JSObject> obj = factory()->NewJSObjectFromMap(map);
2536-
JSObject::NormalizeProperties(obj, KEEP_INOBJECT_PROPERTIES, 8,
2537-
"SetupSymbolRegistry");
2538-
JSObject::SetProperty(registry, name, obj, STRICT).Assert();
2539-
}
2542+
SetUpSubregistry(registry, map, "for");
2543+
SetUpSubregistry(registry, map, "for_api");
2544+
SetUpSubregistry(registry, map, "keyFor");
2545+
SetUpSubregistry(registry, map, "private_api");
2546+
heap()->AddPrivateGlobalSymbols(
2547+
SetUpSubregistry(registry, map, "private_intern"));
25402548
}
25412549
return Handle<JSObject>::cast(factory()->symbol_registry());
25422550
}

0 commit comments

Comments
 (0)