[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: https://github.com/dart-lang/sdk/issues/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>
diff --git a/runtime/vm/isolate_reload.cc b/runtime/vm/isolate_reload.cc
index 231aa10..892a7ed 100644
--- a/runtime/vm/isolate_reload.cc
+++ b/runtime/vm/isolate_reload.cc
@@ -1258,12 +1258,7 @@
}
}
- 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() {
@@ -1416,15 +1411,6 @@
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
@@ -1525,6 +1511,8 @@
}
{
+ MorphInstancesAndApplyNewClassTable();
+
const GrowableObjectArray& become_enum_mappings =
GrowableObjectArray::Handle(become_enum_mappings_);
UnorderedHashMap<BecomeMapTraits> become_map(become_map_storage_);
@@ -1647,10 +1635,17 @@
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) {
@@ -1661,52 +1656,66 @@
}
}
- // 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++;
- }
- }
- ASSERT(index == count);
+ // 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();
}
- // 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.
+ // 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);
+
+ // 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() {
@@ -1789,6 +1798,16 @@
}
}
+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_;
}
diff --git a/runtime/vm/isolate_reload.h b/runtime/vm/isolate_reload.h
index 04a2d5e..082bc48 100644
--- a/runtime/vm/isolate_reload.h
+++ b/runtime/vm/isolate_reload.h
@@ -173,6 +173,7 @@
// 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 @@
void CheckpointLibraries();
- // Transforms the heap based on instance_morphers_. Return whether there was
- // any morphing.
- bool MorphInstances();
+ void MorphInstancesAndApplyNewClassTable();
void RunNewFieldInitializers();
diff --git a/tests/language_2/language_2_kernel.status b/tests/language_2/language_2_kernel.status
index d715621..75d0043 100644
--- a/tests/language_2/language_2_kernel.status
+++ b/tests/language_2/language_2_kernel.status
@@ -968,9 +968,8 @@
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
