Reland "[snapshot] rehash JSMap and JSSet during deserialization"

joyeecheung · Commit Bot · commit 22014de00115 · 2020-05-27T08:49:38.000Z
This is a reland of 8374fee. Fixed rehashing of global proxy keys by creating its identity hash early, before the deserialization of the context snapshot. Original change's description: > [snapshot] rehash JSMap and JSSet during deserialization > > To rehash JSMap and JSSet, we simply replace the backing store > with a new one created with the new hash. > > Bug: v8:9187 > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983 > Commit-Queue: Joyee Cheung <joyee@igalia.com> > Reviewed-by: Jakob Gruber <jgruber@chromium.org> > Reviewed-by: Camillo Bruni <cbruni@chromium.org> > Cr-Commit-Position: refs/heads/master@{#67663} Bug: v8:9187, v8:10523 Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085 Reviewed-by: Leszek Swirski <leszeks@chromium.org> Reviewed-by: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/master@{#67999}
diff --git a/src/heap/factory.cc b/src/heap/factory.cc
@@ -2819,8 +2819,12 @@ Handle<JSGlobalProxy> Factory::NewUninitializedJSGlobalProxy(int size) {
   map->set_is_access_check_needed(true);
   map->set_may_have_interesting_symbols(true);
   LOG(isolate(), MapDetails(*map));
-  return Handle<JSGlobalProxy>::cast(
+  Handle<JSGlobalProxy> proxy = Handle<JSGlobalProxy>::cast(
       NewJSObjectFromMap(map, AllocationType::kYoung));
+  // Create identity hash early in case there is any JS collection containing
+  // a global proxy key and needs to be rehashed after deserialization.
+  proxy->GetOrCreateIdentityHash(isolate());
+  return proxy;
 }
 
 void Factory::ReinitializeJSGlobalProxy(Handle<JSGlobalProxy> object,
diff --git a/src/objects/js-collection.h b/src/objects/js-collection.h
@@ -30,6 +30,7 @@ class JSSet : public TorqueGeneratedJSSet<JSSet, JSCollection> {
  public:
   static void Initialize(Handle<JSSet> set, Isolate* isolate);
   static void Clear(Isolate* isolate, Handle<JSSet> set);
+  void Rehash(Isolate* isolate);
 
   // Dispatched behavior.
   DECL_PRINTER(JSSet)
@@ -56,6 +57,7 @@ class JSMap : public TorqueGeneratedJSMap<JSMap, JSCollection> {
  public:
   static void Initialize(Handle<JSMap> map, Isolate* isolate);
   static void Clear(Isolate* isolate, Handle<JSMap> map);
+  void Rehash(Isolate* isolate);
 
   // Dispatched behavior.
   DECL_PRINTER(JSMap)
diff --git a/src/objects/objects.cc b/src/objects/objects.cc
@@ -2306,9 +2306,8 @@ bool HeapObject::NeedsRehashing() const {
     case TRANSITION_ARRAY_TYPE:
       return TransitionArray::cast(*this).number_of_entries() > 1;
     case ORDERED_HASH_MAP_TYPE:
-      return OrderedHashMap::cast(*this).NumberOfElements() > 0;
     case ORDERED_HASH_SET_TYPE:
-      return OrderedHashSet::cast(*this).NumberOfElements() > 0;
+      return false;  // We'll rehash from the JSMap or JSSet referencing them.
     case NAME_DICTIONARY_TYPE:
     case GLOBAL_DICTIONARY_TYPE:
     case NUMBER_DICTIONARY_TYPE:
@@ -2318,6 +2317,8 @@ bool HeapObject::NeedsRehashing() const {
     case SMALL_ORDERED_HASH_MAP_TYPE:
     case SMALL_ORDERED_HASH_SET_TYPE:
     case SMALL_ORDERED_NAME_DICTIONARY_TYPE:
+    case JS_MAP_TYPE:
+    case JS_SET_TYPE:
       return true;
     default:
       return false;
@@ -2327,10 +2328,13 @@ bool HeapObject::NeedsRehashing() const {
 bool HeapObject::CanBeRehashed() const {
   DCHECK(NeedsRehashing());
   switch (map().instance_type()) {
+    case JS_MAP_TYPE:
+    case JS_SET_TYPE:
+      return true;
     case ORDERED_HASH_MAP_TYPE:
     case ORDERED_HASH_SET_TYPE:
+      UNREACHABLE();  // We'll rehash from the JSMap or JSSet referencing them.
     case ORDERED_NAME_DICTIONARY_TYPE:
-      // TODO(yangguo): actually support rehashing OrderedHash{Map,Set}.
       return false;
     case NAME_DICTIONARY_TYPE:
     case GLOBAL_DICTIONARY_TYPE:
@@ -2387,6 +2391,19 @@ void HeapObject::RehashBasedOnMap(LocalIsolateWrapper isolate) {
     case SMALL_ORDERED_HASH_SET_TYPE:
       DCHECK_EQ(0, SmallOrderedHashSet::cast(*this).NumberOfElements());
       break;
+    case ORDERED_HASH_MAP_TYPE:
+    case ORDERED_HASH_SET_TYPE:
+      UNREACHABLE();  // We'll rehash from the JSMap or JSSet referencing them.
+    case JS_MAP_TYPE: {
+      DCHECK(isolate.is_main_thread());
+      JSMap::cast(*this).Rehash(isolate.main_thread());
+      break;
+    }
+    case JS_SET_TYPE: {
+      DCHECK(isolate.is_main_thread());
+      JSSet::cast(*this).Rehash(isolate.main_thread());
+      break;
+    }
     case SMALL_ORDERED_NAME_DICTIONARY_TYPE:
       DCHECK_EQ(0, SmallOrderedNameDictionary::cast(*this).NumberOfElements());
       break;
@@ -7863,6 +7880,13 @@ void JSSet::Clear(Isolate* isolate, Handle<JSSet> set) {
   set->set_table(*table);
 }
 
+void JSSet::Rehash(Isolate* isolate) {
+  Handle<OrderedHashSet> table_handle(OrderedHashSet::cast(table()), isolate);
+  Handle<OrderedHashSet> new_table =
+      OrderedHashSet::Rehash(isolate, table_handle).ToHandleChecked();
+  set_table(*new_table);
+}
+
 void JSMap::Initialize(Handle<JSMap> map, Isolate* isolate) {
   Handle<OrderedHashMap> table = isolate->factory()->NewOrderedHashMap();
   map->set_table(*table);
@@ -7874,6 +7898,13 @@ void JSMap::Clear(Isolate* isolate, Handle<JSMap> map) {
   map->set_table(*table);
 }
 
+void JSMap::Rehash(Isolate* isolate) {
+  Handle<OrderedHashMap> table_handle(OrderedHashMap::cast(table()), isolate);
+  Handle<OrderedHashMap> new_table =
+      OrderedHashMap::Rehash(isolate, table_handle).ToHandleChecked();
+  set_table(*new_table);
+}
+
 void JSWeakCollection::Initialize(Handle<JSWeakCollection> weak_collection,
                                   Isolate* isolate) {
   Handle<EphemeronHashTable> table = EphemeronHashTable::New(isolate, 0);
diff --git a/src/objects/ordered-hash-table.cc b/src/objects/ordered-hash-table.cc
@@ -194,6 +194,13 @@ HeapObject OrderedHashMap::GetEmpty(ReadOnlyRoots ro_roots) {
   return ro_roots.empty_ordered_hash_map();
 }
 
+template <class Derived, int entrysize>
+MaybeHandle<Derived> OrderedHashTable<Derived, entrysize>::Rehash(
+    Isolate* isolate, Handle<Derived> table) {
+  return OrderedHashTable<Derived, entrysize>::Rehash(isolate, table,
+                                                      table->Capacity());
+}
+
 template <class Derived, int entrysize>
 MaybeHandle<Derived> OrderedHashTable<Derived, entrysize>::Rehash(
     Isolate* isolate, Handle<Derived> table, int new_capacity) {
@@ -250,6 +257,20 @@ MaybeHandle<OrderedHashSet> OrderedHashSet::Rehash(Isolate* isolate,
                                                      new_capacity);
 }
 
+MaybeHandle<OrderedHashSet> OrderedHashSet::Rehash(
+    Isolate* isolate, Handle<OrderedHashSet> table) {
+  return OrderedHashTable<
+      OrderedHashSet, OrderedHashSet::kEntrySizeWithoutChain>::Rehash(isolate,
+                                                                      table);
+}
+
+MaybeHandle<OrderedHashMap> OrderedHashMap::Rehash(
+    Isolate* isolate, Handle<OrderedHashMap> table) {
+  return OrderedHashTable<
+      OrderedHashMap, OrderedHashMap::kEntrySizeWithoutChain>::Rehash(isolate,
+                                                                      table);
+}
+
 MaybeHandle<OrderedHashMap> OrderedHashMap::Rehash(Isolate* isolate,
                                                    Handle<OrderedHashMap> table,
                                                    int new_capacity) {
diff --git a/src/objects/ordered-hash-table.h b/src/objects/ordered-hash-table.h
@@ -138,6 +138,7 @@ class OrderedHashTable : public FixedArray {
 
   // The extra +1 is for linking the bucket chains together.
   static const int kEntrySize = entrysize + 1;
+  static const int kEntrySizeWithoutChain = entrysize;
   static const int kChainOffset = entrysize;
 
   static const int kNotFound = -1;
@@ -200,6 +201,8 @@ class OrderedHashTable : public FixedArray {
   static MaybeHandle<Derived> Allocate(
       Isolate* isolate, int capacity,
       AllocationType allocation = AllocationType::kYoung);
+
+  static MaybeHandle<Derived> Rehash(Isolate* isolate, Handle<Derived> table);
   static MaybeHandle<Derived> Rehash(Isolate* isolate, Handle<Derived> table,
                                      int new_capacity);
 
@@ -244,6 +247,8 @@ class V8_EXPORT_PRIVATE OrderedHashSet
   static MaybeHandle<OrderedHashSet> Rehash(Isolate* isolate,
                                             Handle<OrderedHashSet> table,
                                             int new_capacity);
+  static MaybeHandle<OrderedHashSet> Rehash(Isolate* isolate,
+                                            Handle<OrderedHashSet> table);
   static MaybeHandle<OrderedHashSet> Allocate(
       Isolate* isolate, int capacity,
       AllocationType allocation = AllocationType::kYoung);
@@ -273,6 +278,8 @@ class V8_EXPORT_PRIVATE OrderedHashMap
   static MaybeHandle<OrderedHashMap> Rehash(Isolate* isolate,
                                             Handle<OrderedHashMap> table,
                                             int new_capacity);
+  static MaybeHandle<OrderedHashMap> Rehash(Isolate* isolate,
+                                            Handle<OrderedHashMap> table);
   Object ValueAt(int entry);
 
   // This takes and returns raw Address values containing tagged Object
diff --git a/src/snapshot/context-deserializer.cc b/src/snapshot/context-deserializer.cc
@@ -59,12 +59,12 @@ MaybeHandle<Object> ContextDeserializer::Deserialize(
     // new code, which also has to be flushed from instruction cache.
     CHECK_EQ(start_address, code_space->top());
 
-    if (FLAG_rehash_snapshot && can_rehash()) Rehash();
     LogNewMapEvents();
 
     result = handle(root, isolate);
   }
 
+  if (FLAG_rehash_snapshot && can_rehash()) Rehash();
   SetupOffHeapArrayBufferBackingStores();
 
   return result;
diff --git a/src/snapshot/deserializer.cc b/src/snapshot/deserializer.cc
@@ -147,6 +147,14 @@ void Deserializer::DeserializeDeferredObjects() {
       }
     }
   }
+
+  // When the deserialization of maps are deferred, they will be created
+  // as filler maps, and we postpone the post processing until the maps
+  // are also deserialized.
+  for (const auto& pair : fillers_to_post_process_) {
+    DCHECK(!pair.first.IsFiller());
+    PostProcessNewObject(pair.first, pair.second);
+  }
 }
 
 void Deserializer::LogNewMapEvents() {
@@ -208,7 +216,11 @@ HeapObject Deserializer::PostProcessNewObject(HeapObject obj,
   DisallowHeapAllocation no_gc;
 
   if ((FLAG_rehash_snapshot && can_rehash_) || deserializing_user_code()) {
-    if (obj.IsString()) {
+    if (obj.IsFiller()) {
+      DCHECK_EQ(fillers_to_post_process_.find(obj),
+                fillers_to_post_process_.end());
+      fillers_to_post_process_.insert({obj, space});
+    } else if (obj.IsString()) {
       // Uninitialize hash field as we need to recompute the hash.
       String string = String::cast(obj);
       string.set_hash_field(String::kEmptyHashField);
diff --git a/src/snapshot/deserializer.h b/src/snapshot/deserializer.h
@@ -204,6 +204,11 @@ class V8_EXPORT_PRIVATE Deserializer : public SerializerDeserializer {
   // TODO(6593): generalize rehashing, and remove this flag.
   bool can_rehash_;
   std::vector<HeapObject> to_rehash_;
+  // Store the objects whose maps are deferred and thus initialized as filler
+  // maps during deserialization, so that they can be processed later when the
+  // maps become available.
+  std::unordered_map<HeapObject, SnapshotSpace, Object::Hasher>
+      fillers_to_post_process_;
 
 #ifdef DEBUG
   uint32_t num_api_references_;
diff --git a/src/snapshot/object-deserializer.cc b/src/snapshot/object-deserializer.cc
@@ -67,11 +67,12 @@ MaybeHandle<HeapObject> ObjectDeserializer::Deserialize(
       LogNewMapEvents();
     }
     result = handle(HeapObject::cast(root), local_isolate);
-    Rehash();
     if (is_main_thread()) {
       allocator()->RegisterDeserializedObjectsForBlackAllocation();
     }
   }
+
+  Rehash();
   CommitPostProcessedObjects();
   return scope.CloseAndEscape(result);
 }
diff --git a/test/cctest/test-serialize.cc b/test/cctest/test-serialize.cc
@@ -3755,7 +3755,7 @@ UNINITIALIZED_TEST(SnapshotCreatorIncludeGlobalProxy) {
   FreeCurrentEmbeddedBlob();
 }
 
-UNINITIALIZED_TEST(ReinitializeHashSeedNotRehashable) {
+UNINITIALIZED_TEST(ReinitializeHashSeedJSCollectionRehashable) {
   DisableAlwaysOpt();
   i::FLAG_rehash_snapshot = true;
   i::FLAG_hash_seed = 42;
@@ -3773,13 +3773,18 @@ UNINITIALIZED_TEST(ReinitializeHashSeedNotRehashable) {
       CompileRun(
           "var m = new Map();"
           "m.set('a', 1);"
-          "m.set('b', 2);");
+          "m.set('b', 2);"
+          "var s = new Set();"
+          "s.add(1);"
+          "s.add(globalThis);");
       ExpectInt32("m.get('b')", 2);
+      ExpectTrue("s.has(1)");
+      ExpectTrue("s.has(globalThis)");
       creator.SetDefaultContext(context);
     }
     blob =
         creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear);
-    CHECK(!blob.CanBeRehashed());
+    CHECK(blob.CanBeRehashed());
   }
 
   i::FLAG_hash_seed = 1337;
@@ -3788,15 +3793,17 @@ UNINITIALIZED_TEST(ReinitializeHashSeedNotRehashable) {
   create_params.snapshot_blob = &blob;
   v8::Isolate* isolate = v8::Isolate::New(create_params);
   {
-    // Check that no rehashing has been performed.
-    CHECK_EQ(static_cast<uint64_t>(42),
+    // Check that rehashing has been performed.
+    CHECK_EQ(static_cast<uint64_t>(1337),
              HashSeed(reinterpret_cast<i::Isolate*>(isolate)));
     v8::Isolate::Scope isolate_scope(isolate);
     v8::HandleScope handle_scope(isolate);
     v8::Local<v8::Context> context = v8::Context::New(isolate);
     CHECK(!context.IsEmpty());
     v8::Context::Scope context_scope(context);
     ExpectInt32("m.get('b')", 2);
+    ExpectTrue("s.has(1)");
+    ExpectTrue("s.has(globalThis)");
   }
   isolate->Dispose();
   delete[] blob.data;

Original file line number	Diff line number	Diff line change
`@@ -67,11 +67,12 @@ MaybeHandle<HeapObject> ObjectDeserializer::Deserialize(`
`67`	`67`	`LogNewMapEvents();`
`68`	`68`	`}`
`69`	`69`	`result = handle(HeapObject::cast(root), local_isolate);`
`70`		`- Rehash();`
`71`	`70`	`if (is_main_thread()) {`
`72`	`71`	`allocator()->RegisterDeserializedObjectsForBlackAllocation();`
`73`	`72`	`}`
`74`	`73`	`}`
	`74`	`+`
	`75`	`+ Rehash();`
`75`	`76`	`CommitPostProcessedObjects();`
`76`	`77`	`return scope.CloseAndEscape(result);`
`77`	`78`	`}`