[vm, reload] On the path for no shape changes, defer freeing the old …

…class table until the next safepoint.

Compare the reload rollback path.

Bug: dart-lang/sdk#34888
Change-Id: I5b9e7cd8392f9c9d61df31481956a9fa67435b33
Reviewed-on: https://dart-review.googlesource.com/c/81940
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

runtime/vm/isolate_reload.cc

@@ -1249,12 +1249,7 @@ void IsolateReloadContext::RollbackClasses() {
     }
   }
 
-  ClassAndSize* local_saved_class_table = saved_class_table_;
-  saved_class_table_ = NULL;
-  // Can't free this table immediately as another thread (e.g., the sweeper) may
-  // be suspended between loading the table pointer and loading the table
-  // element. Table will be freed at the next major GC or isolate shutdown.
-  class_table->AddOldTable(local_saved_class_table);
+  DiscardSavedClassTable();
 }
 
 void IsolateReloadContext::RollbackLibraries() {
@@ -1407,15 +1402,6 @@ void IsolateReloadContext::Commit() {
   TIMELINE_SCOPE(Commit);
   TIR_Print("---- COMMITTING RELOAD\n");
 
-  // Note that the object heap contains before and after instances
-  // used for morphing. It is therefore important that morphing takes
-  // place prior to any heap walking.
-  // So please keep this code at the top of Commit().
-  if (!MorphInstances()) {
-    free(saved_class_table_);
-    saved_class_table_ = NULL;
-  }
-
 #ifdef DEBUG
   VerifyMaps();
 #endif
@@ -1516,6 +1502,8 @@ void IsolateReloadContext::Commit() {
   }
 
   {
+    MorphInstancesAndApplyNewClassTable();
+
     const GrowableObjectArray& become_enum_mappings =
         GrowableObjectArray::Handle(become_enum_mappings_);
     UnorderedHashMap<BecomeMapTraits> become_map(become_map_storage_);
@@ -1638,10 +1626,17 @@ class ObjectLocator : public ObjectVisitor {
   intptr_t count_;
 };
 
-bool IsolateReloadContext::MorphInstances() {
+static bool HasNoTasks(Heap* heap) {
+  MonitorLocker ml(heap->old_space()->tasks_lock());
+  return heap->old_space()->tasks() == 0;
+}
+
+void IsolateReloadContext::MorphInstancesAndApplyNewClassTable() {
   TIMELINE_SCOPE(MorphInstances);
   if (!HasInstanceMorphers()) {
-    return false;
+    // Fast path: no class had a shape change.
+    DiscardSavedClassTable();
+    return;
   }
 
   if (FLAG_trace_reload) {
@@ -1652,52 +1647,66 @@ bool IsolateReloadContext::MorphInstances() {
     }
   }
 
-  // Find all objects that need to be morphed.
+  // Find all objects that need to be morphed (reallocated to a new size).
   ObjectLocator locator(this);
   {
     HeapIterationScope iteration(Thread::Current());
     iteration.IterateObjects(&locator);
   }
 
-  // Return if no objects are located.
   intptr_t count = locator.count();
   if (count == 0) {
-    return false;
+    // Fast path: classes with shape change have no instances.
+    DiscardSavedClassTable();
+    return;
   }
 
   TIR_Print("Found %" Pd " object%s subject to morphing.\n", count,
             (count > 1) ? "s" : "");
 
-  Array& before = Array::Handle();
-  Array& after = Array::Handle();
-  {  // Prevent GC to take place due object format confusion.
-    // Hint: More than one class share the same cid.
-    NoHeapGrowthControlScope scope;
-    for (intptr_t i = 0; i < instance_morphers_.length(); i++) {
-      instance_morphers_.At(i)->CreateMorphedCopies();
-    }
-    // Create the inputs for Become.
-    intptr_t index = 0;
-    before = Array::New(count);
-    after = Array::New(count);
-    for (intptr_t i = 0; i < instance_morphers_.length(); i++) {
-      InstanceMorpher* morpher = instance_morphers_.At(i);
-      for (intptr_t j = 0; j < morpher->before()->length(); j++) {
-        before.SetAt(index, *morpher->before()->At(j));
-        after.SetAt(index, *morpher->after()->At(j));
-        index++;
-      }
+  // While we are reallocating instances to their new size, the heap will
+  // contain a mix of instances with the old and new sizes that have the same
+  // cid. This makes the heap unwalkable until the "become" operation below
+  // replaces all the instances of the old size with forwarding corpses. Force
+  // heap growth to prevent size confusion during this period.
+  NoHeapGrowthControlScope scope;
+  // The HeapIterationScope above ensures no other GC tasks can be active.
+  ASSERT(HasNoTasks(I->heap()));
+
+  for (intptr_t i = 0; i < instance_morphers_.length(); i++) {
+    instance_morphers_.At(i)->CreateMorphedCopies();
+  }
+
+  // Create the inputs for Become.
+  intptr_t index = 0;
+  const Array& before = Array::Handle(Array::New(count));
+  const Array& after = Array::Handle(Array::New(count));
+  for (intptr_t i = 0; i < instance_morphers_.length(); i++) {
+    InstanceMorpher* morpher = instance_morphers_.At(i);
+    for (intptr_t j = 0; j < morpher->before()->length(); j++) {
+      before.SetAt(index, *morpher->before()->At(j));
+      after.SetAt(index, *morpher->after()->At(j));
+      index++;
     }
-    ASSERT(index == count);
   }
+  ASSERT(index == count);
 
-  // This is important: The saved class table (describing before objects)
-  // must be zapped to prevent the forwarding in GetClassSizeForHeapWalkAt.
-  // Instance will from now be described by the isolate's class table.
+  // Apply the new class table before "become". Become will replace all the
+  // instances of the old size with forwarding corpses, then perform a heap walk
+  // to fix references to the forwarding corpses. During this heap walk, it will
+  // encounter instances of the new size, so it requires the new class table.
+  ASSERT(HasNoTasks(I->heap()));
+#if defined(DEBUG)
+  for (intptr_t i = 0; i < saved_num_cids_; i++) {
+    saved_class_table_[i] = ClassAndSize(nullptr, -1);
+  }
+#endif
   free(saved_class_table_);
-  saved_class_table_ = NULL;
+  saved_class_table_ = nullptr;
+
   Become::ElementsForwardIdentity(before, after);
-  return true;
+  // The heap now contains only instances with the new size. Ordinary GC is safe
+  // again.
 }
 
 void IsolateReloadContext::RunNewFieldInitializers() {
@@ -1780,6 +1789,16 @@ intptr_t IsolateReloadContext::GetClassSizeForHeapWalkAt(intptr_t cid) {
   }
 }
 
+void IsolateReloadContext::DiscardSavedClassTable() {
+  ClassAndSize* local_saved_class_table = saved_class_table_;
+  saved_class_table_ = nullptr;
+  // Can't free this table immediately as another thread (e.g., concurrent
+  // marker or sweeper) may be between loading the table pointer and loading the
+  // table element. The table will be freed at the next major GC or isolate
+  // shutdown.
+  I->class_table()->AddOldTable(local_saved_class_table);
+}
+
 RawLibrary* IsolateReloadContext::saved_root_library() const {
   return saved_root_library_;
 }

runtime/vm/isolate_reload.h

@@ -173,6 +173,7 @@ class IsolateReloadContext {
   // Prefers old classes when we are in the middle of a reload.
   RawClass* GetClassForHeapWalkAt(intptr_t cid);
   intptr_t GetClassSizeForHeapWalkAt(intptr_t cid);
+  void DiscardSavedClassTable();
 
   void RegisterClass(const Class& new_cls);
 
@@ -244,9 +245,7 @@ class IsolateReloadContext {
 
   void CheckpointLibraries();
 
-  // Transforms the heap based on instance_morphers_. Return whether there was
-  // any morphing.
-  bool MorphInstances();
+  void MorphInstancesAndApplyNewClassTable();
 
   void RunNewFieldInitializers();
 

tests/language_2/language_2_kernel.status

@@ -991,9 +991,8 @@ async_star_test/01: Crash
 async_star_test/05: Crash
 
 [ $mode == debug && ($compiler == dartk || $compiler == dartkb) && ($hot_reload || $hot_reload_rollback) ]
-enum_duplicate_test/01: Pass, Crash # Issue 34606
 enum_duplicate_test/02: Crash # Issue 34606
-enum_duplicate_test/none: Crash # Issue 34606
+enum_duplicate_test/none: Pass, Crash # Issue 34606
 enum_private_test/01: Crash # Issue 34606
 enum_test: Crash # Issue 34606