diff --git a/runtime/vm/class_finalizer.cc b/runtime/vm/class_finalizer.cc index 441a3b09fb8..3aa34a4f21e 100644 --- a/runtime/vm/class_finalizer.cc +++ b/runtime/vm/class_finalizer.cc @@ -994,10 +994,8 @@ void ClassFinalizer::SortClasses() { } ASSERT(next_new_cid == num_cids); RemapClassIds(old_to_new_cid.get()); - // Types use cid's as part of their hashes. - RehashTypes(); - // Const objects use cid's as part of their hashes. - IG->RehashConstants(nullptr); + RehashTypes(); // Types use cid's as part of their hashes. + IG->RehashConstants(); // Const objects use cid's as part of their hashes. } class CidRewriteVisitor : public ObjectVisitor { @@ -1244,7 +1242,7 @@ void ClassFinalizer::RehashTypes() { // The canonical constant tables use canonical hashcodes which can change // due to cid-renumbering. - IG->RehashConstants(nullptr); + IG->RehashConstants(); dict_size = Utils::RoundUpToPowerOfTwo(typeargs.Length() * 4 / 3); CanonicalTypeArgumentsSet typeargs_table( diff --git a/runtime/vm/heap/become.cc b/runtime/vm/heap/become.cc index 875798abfec..b5fc939ae4e 100644 --- a/runtime/vm/heap/become.cc +++ b/runtime/vm/heap/become.cc @@ -274,10 +274,6 @@ static void InvalidForwarding(ObjectPtr before, } void Become::Forward() { - if (pointers_.length() == 0) { - return; - } - Thread* thread = Thread::Current(); auto heap = thread->isolate_group()->heap(); diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index c3f02f6e784..71ea2259721 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -837,7 +837,7 @@ void IsolateGroup::ExitTemporaryIsolate() { Dart::ShutdownIsolate(thread); } -void IsolateGroup::RehashConstants(Become* become) { +void IsolateGroup::RehashConstants() { // Even though no individual constant contains a cycle, there can be "cycles" // between the canonical tables if some const instances of A have fields that // are const instance of B and vice versa. So set all the old tables to the @@ -872,7 +872,9 @@ void IsolateGroup::RehashConstants(Become* become) { } cls = class_table()->At(cid); + if (cls.constants() == Array::null()) continue; + old_constant_tables[cid] = &Array::Handle(zone, cls.constants()); cls.set_constants(Object::null_array()); } @@ -881,12 +883,6 @@ void IsolateGroup::RehashConstants(Become* become) { heap()->ResetCanonicalHashTable(); Instance& constant = Instance::Handle(zone); - Field& field = Field::Handle(zone); - String& name = String::Handle(zone); - Array& new_values = Array::Handle(zone); - Instance& old_value = Instance::Handle(zone); - Instance& new_value = Instance::Handle(zone); - Instance& deleted = Instance::Handle(zone); for (intptr_t cid = kInstanceCid; cid < num_cids; cid++) { Array* old_constants = old_constant_tables[cid]; if (old_constants == nullptr) continue; @@ -894,61 +890,14 @@ void IsolateGroup::RehashConstants(Become* become) { cls = class_table()->At(cid); CanonicalInstancesSet set(zone, old_constants->ptr()); CanonicalInstancesSet::Iterator it(&set); - - if (cls.is_enum_class() && (become != nullptr)) { - field = cls.LookupStaticField(Symbols::_DeletedEnumSentinel()); - deleted ^= field.StaticConstFieldValue(); - if (deleted.IsNull()) { - deleted = Instance::New(cls, Heap::kOld); - field = object_store()->enum_name_field(); - name = cls.ScrubbedName(); - name = Symbols::FromConcat(thread, Symbols::_DeletedEnumPrefix(), name); - deleted.SetField(field, name); - field = object_store()->enum_index_field(); - new_value = Smi::New(-1); - deleted.SetField(field, new_value); - // The static const field contains `Object::null()` instead of - // `Object::sentinel()` - so it's not considered an initializing store. - field.SetStaticConstFieldValue(deleted, - /*assert_initializing_store*/ false); - } - - field = cls.LookupField(Symbols::Values()); - new_values ^= field.StaticConstFieldValue(); - - field = object_store()->enum_name_field(); - while (it.MoveNext()) { - old_value ^= set.GetKey(it.Current()); - ASSERT(old_value.GetClassId() == cid); - bool found = false; - for (intptr_t j = 0; j < new_values.Length(); j++) { - new_value ^= new_values.At(j); - ASSERT(new_value.GetClassId() == cid); - if (old_value.GetField(field) == new_value.GetField(field)) { - found = true; - break; - } - } - if (!found) { - new_value = deleted.ptr(); - } - - if (old_value.ptr() != new_value.ptr()) { - become->Add(old_value, new_value); - } - if (new_value.IsCanonical()) { - cls.InsertCanonicalConstant(zone, new_value); - } - } - } else { - while (it.MoveNext()) { - constant ^= set.GetKey(it.Current()); - ASSERT(!constant.IsNull()); - // Shape changes lose the canonical bit because they may result/ in - // merging constants. E.g., [x1, y1], [x1, y2] -> [x1]. - DEBUG_ASSERT(constant.IsCanonical() || HasAttemptedReload()); - cls.InsertCanonicalConstant(zone, constant); - } + while (it.MoveNext()) { + constant ^= set.GetKey(it.Current()); + ASSERT(!constant.IsNull()); + // Shape changes lose the canonical bit because they may result/ in + // merging constants. E.g., [x1, y1], [x1, y2] -> [x1]. + DEBUG_ASSERT(constant.IsCanonical() || + IsolateGroup::Current()->HasAttemptedReload()); + cls.InsertCanonicalConstant(zone, constant); } set.Release(); } diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index d00245998d3..e3a8b81f5a5 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -285,7 +285,7 @@ class IsolateGroup : public IntrusiveDListEntry { Dart_IsolateFlags api_flags); ~IsolateGroup(); - void RehashConstants(Become* become); + void RehashConstants(); #if defined(DEBUG) void ValidateClassTable(); #endif diff --git a/runtime/vm/isolate_reload.cc b/runtime/vm/isolate_reload.cc index 62cb3073e45..bc24d502701 100644 --- a/runtime/vm/isolate_reload.cc +++ b/runtime/vm/isolate_reload.cc @@ -237,6 +237,111 @@ void InstanceMorpher::AddObject(ObjectPtr object) { } void InstanceMorpher::CreateMorphedCopies(Become* become) { + // Enum migriation needs to run in same phase as instance morphing since it + // may also involve a shape change. + if (old_class_.is_enum_class()) { + Field& field = Field::Handle(Z); + String& name = String::Handle(Z); + Array& old_values = Array::Handle(Z); + Array& new_values = Array::Handle(Z); + Instance& old_value = Instance::Handle(Z); + Instance& new_value = Instance::Handle(Z); + Instance& old_deleted = Instance::Handle(Z); + Instance& new_deleted = Instance::Handle(Z); + + field = old_class_.LookupStaticField(Symbols::_DeletedEnumSentinel()); + old_deleted ^= field.StaticConstFieldValue(); + + new_deleted = Instance::New(new_class_, Heap::kOld); + Thread* thread = Thread::Current(); + field = thread->isolate_group()->object_store()->enum_name_field(); + name = new_class_.ScrubbedName(); + name = Symbols::FromConcat(thread, Symbols::_DeletedEnumPrefix(), name); + new_deleted.SetField(field, name); + field = thread->isolate_group()->object_store()->enum_index_field(); + new_value = Smi::New(-1); + new_deleted.SetField(field, new_value); + field = new_class_.LookupStaticField(Symbols::_DeletedEnumSentinel()); + // The static const field contains `Object::null()` instead of + // `Object::sentinel()` - so it's not considered an initializing store. + field.SetStaticConstFieldValue(new_deleted, + /*assert_initializing_store*/ false); + + field = old_class_.LookupField(Symbols::Values()); + old_values ^= field.StaticConstFieldValue(); + + field = new_class_.LookupField(Symbols::Values()); + new_values ^= field.StaticConstFieldValue(); + + field = thread->isolate_group()->object_store()->enum_name_field(); + for (intptr_t i = 0; i < old_values.Length(); i++) { + old_value ^= old_values.At(i); + ASSERT(old_value.GetClassId() == cid_); + bool found = false; + for (intptr_t j = 0; j < new_values.Length(); j++) { + new_value ^= new_values.At(j); + ASSERT(new_value.GetClassId() == cid_); + if (old_value.GetField(field) == new_value.GetField(field)) { + found = true; + break; + } + } + if (!found) { + new_value = new_deleted.ptr(); + } + + if (old_value.ptr() == new_value.ptr()) { + // This probably shouldn't happen and means canonicalization is mixing + // before and after. At any rate, don't submit a self-fowarding to + // become. + RELEASE_ASSERT(old_value.ptr()->untag()->HeapSize() == + new_class_.host_instance_size()); + } else { + // Convert the old instance into a filler object. We will switch to the + // new class table before the next heap walk, so there must be no + // instances of any class with the old size. + Become::MakeDummyObject(old_value); + + become->Add(old_value, new_value); + } + } + + // The deleted sentinel is not part of Enum.values. It doesn't exist yet for + // the first reload. + if (!old_deleted.IsNull()) { + // Convert the old instance into a filler object. We will switch to the + // new class table before the next heap walk, so there must be no + // instances of any class with the old size. + Become::MakeDummyObject(old_deleted); + + become->Add(old_deleted, new_deleted); + } + + // We also forward Enum.values. No filler is needed because arrays never + // change shape. + if (old_value.ptr() == new_value.ptr()) { + // This probably shouldn't happen and means canonicalization is mixing + // before and after. At any rate, don't submit a self-fowarding to + // become. + RELEASE_ASSERT(old_class_.host_instance_size() == + new_class_.host_instance_size()); + } else { + become->Add(old_values, new_values); + } + +#if defined(DEBUG) + for (intptr_t i = 0; i < before_.length(); i++) { + const Instance& before = *before_.At(i); + // All instances are accounted for. + ASSERT((before.GetClassId() == kForwardingCorpse) || + (before.ptr()->untag()->HeapSize() == + new_class_.host_instance_size())); + } +#endif + + return; + } + Instance& after = Instance::Handle(Z); Object& value = Object::Handle(Z); for (intptr_t i = 0; i < before_.length(); i++) { @@ -874,7 +979,6 @@ bool IsolateGroupReloadContext::Reload(bool force_reload, // we have accepted the compilation to clear some state in the incremental // compiler. if (did_kernel_compilation) { - TIMELINE_SCOPE(AcceptCompilation); const auto& result = Object::Handle(Z, AcceptCompilation(thread)); if (result.IsError()) { const auto& error = Error::Cast(result); @@ -891,8 +995,7 @@ bool IsolateGroupReloadContext::Reload(bool force_reload, // layout). ObjectLocator locator(this); { - TIMELINE_SCOPE(CollectInstances); - HeapIterationScope iteration(thread); + HeapIterationScope iteration(Thread::Current()); iteration.IterateObjects(&locator); } @@ -1263,7 +1366,9 @@ void ProgramReloadContext::RegisterClass(const Class& new_cls) { VTIR_Print("Registering class: %s\n", new_cls.ToCString()); new_cls.set_id(old_cls.id()); IG->class_table()->SetAt(old_cls.id(), new_cls.ptr()); - new_cls.CopyCanonicalConstants(old_cls); + if (!old_cls.is_enum_class()) { + new_cls.CopyCanonicalConstants(old_cls); + } new_cls.CopyDeclarationType(old_cls); AddBecomeMapping(old_cls, new_cls); AddClassMapping(new_cls, old_cls); @@ -1741,11 +1846,7 @@ void ProgramReloadContext::CommitAfterInstanceMorphing() { // content may have changed from fields being added or removed. { TIMELINE_SCOPE(RehashConstants); - IG->RehashConstants(&become_); - } - { - TIMELINE_SCOPE(ForwardEnums); - become_.Forward(); + IG->RehashConstants(); } if (FLAG_identity_reload) { @@ -1997,7 +2098,6 @@ void ProgramReloadContext::RunInvalidationVisitors() { GrowableArray instances(4 * KB); { - TIMELINE_SCOPE(CollectInvalidations); HeapIterationScope iteration(thread); InvalidationCollector visitor(zone, &functions, &kernel_infos, &fields, &suspend_states, &instances); diff --git a/runtime/vm/isolate_reload_test.cc b/runtime/vm/isolate_reload_test.cc index e8fb4ba08b0..34247f447f6 100644 --- a/runtime/vm/isolate_reload_test.cc +++ b/runtime/vm/isolate_reload_test.cc @@ -2367,57 +2367,27 @@ TEST_CASE(IsolateReload_EnumShapeChangeRemove) { SimpleInvokeStr(lib, "main")); } -TEST_CASE(IsolateReload_EnumReferentShapeChangeAdd) { +TEST_CASE(IsolateReload_EnumShapeChangeValues) { const char* kScript = - "class Box {\n" - " final x;\n" - " const Box(this.x);\n" - "}\n" - "enum Fruit {\n" - " Apple('Apple', const Box('A')),\n" - " Banana('Banana', const Box('B')),\n" - " Cherry('Cherry', const Box('C')),\n" - " Durian('Durian', const Box('D')),\n" - " Elderberry('Elderberry', const Box('E')),\n" - " Fig('Fig', const Box('F')),\n" - " Grape('Grape', const Box('G')),\n" - " Huckleberry('Huckleberry', const Box('H')),\n" - " Jackfruit('Jackfruit', const Box('J'));\n" - " const Fruit(this.name, this.initial);\n" - " final String name;\n" - " final Box initial;\n" - "}\n" + "enum Fruit { Apple, Banana }\n" "var retained;\n" "main() {\n" - " retained = Fruit.Apple;\n" + " retained = Fruit.values;\n" " return retained.toString();\n" "}\n"; Dart_Handle lib = TestCase::LoadTestScript(kScript, nullptr); EXPECT_VALID(lib); - EXPECT_STREQ("Fruit.Apple", SimpleInvokeStr(lib, "main")); + EXPECT_STREQ("[Fruit.Apple, Fruit.Banana]", SimpleInvokeStr(lib, "main")); const char* kReloadScript = - "class Box {\n" - " final x;\n" - " final y;\n" - " final z;\n" - " const Box(this.x, this.y, this.z);\n" - "}\n" "enum Fruit {\n" - " Apple('Apple', const Box('A', 0, 0)),\n" - " Banana('Banana', const Box('B', 0, 0)),\n" - " Cherry('Cherry', const Box('C', 0, 0)),\n" - " Durian('Durian', const Box('D', 0, 0)),\n" - " Elderberry('Elderberry', const Box('E', 0, 0)),\n" - " Fig('Fig', const Box('F', 0, 0)),\n" - " Grape('Grape', const Box('G', 0, 0)),\n" - " Huckleberry('Huckleberry', const Box('H', 0, 0)),\n" - " Jackfruit('Jackfruit', const Box('J', 0, 0)),\n" - " Lemon('Lemon', const Box('L', 0, 0));\n" + " Apple('Apple', 'A'),\n" + " Banana('Banana', 'B'),\n" + " Cherry('Cherry', 'C');\n" " const Fruit(this.name, this.initial);\n" " final String name;\n" - " final Box initial;\n" + " final String initial;\n" "}\n" "var retained;\n" "main() {\n" @@ -2426,7 +2396,8 @@ TEST_CASE(IsolateReload_EnumReferentShapeChangeAdd) { lib = TestCase::ReloadTestScript(kReloadScript); EXPECT_VALID(lib); - EXPECT_STREQ("Fruit.Apple", SimpleInvokeStr(lib, "main")); + EXPECT_STREQ("[Fruit.Apple, Fruit.Banana, Fruit.Cherry]", + SimpleInvokeStr(lib, "main")); } TEST_CASE(IsolateReload_ConstantIdentical) { diff --git a/runtime/vm/object_reload.cc b/runtime/vm/object_reload.cc index 7762eaa3dd1..60af532036e 100644 --- a/runtime/vm/object_reload.cc +++ b/runtime/vm/object_reload.cc @@ -227,6 +227,11 @@ void Class::CopyStaticFieldValues(ProgramReloadContext* reload_context, } void Class::CopyCanonicalConstants(const Class& old_cls) const { + if (is_enum_class()) { + // We do not copy enum classes's canonical constants because we explicitly + // become the old enum values to the new enum values. + return; + } #if defined(DEBUG) { // Class has no canonical constants allocated.