From 86d1261dff279acf84bc699c6c116189955b0076 Mon Sep 17 00:00:00 2001 From: Tess Strickland Date: Wed, 12 Jul 2023 07:37:16 +0000 Subject: [PATCH] [vm] Clean up New() methods of Dart classes. Goals of this CL, a followup to 2f63acea22: * Ensure that X::New() depends on the initialization guarantees of Object::Allocate(...): * Ptr fields are guaranteed to be initialized to Object::null(). * Non-Ptr fields are guaranteed to be zero-initialized. In cases where the only uses of the allocated object before return was to perform unnecessary field assignments, X::New() just simply returns the result of Object::Allocate(...). Otherwise, the old now-unnecessary assignments have been changed into ASSERTs so that they will be checked in DEBUG mode. * Ensure that NoSafepointScopes are entered in X::New() only when necessary (e.g., to ensure fields used to calculate to(...) are properly set before being seen by pointer visitors as the GC may run when outside a NoSafepointScope). In particular, the often occurring pattern: auto& result = X::Handle(); { auto raw = Object::Allocate(...); NoSafepointScope no_safepoint; result = raw; } ... has been replaced with: const auto& result = X::Handle(Object::Allocate(...)); * If a handle was allocated, the only uses of that handle before returning must be performed under a NoSafepointScope, and the same operations can be done directly on the object pointer, then do so and remove the unnecessary handle allocation. Notable changes outside the above: * Swapped ObjectPool::EntryType::{kImmediate,kTaggedObject} so that kImmediate has value 0, since Object::Allocate(len) zero-initializes the payload and without this change, ObjectPool::New() must set the entry types manually. * Removed the old static ArrayPtr cached_array_ field on SubtypeTestCache as well as SubtypeTestCache::{Init,Cleanup} and instead added Object::empty_subtype_test_cache_array(). * Removed the no-arg Closure::New() method, which is never used. * Inlined the no-arg Script::New() method into its only caller, one of the other Script::New() overloads. TEST=ci Issue: https://github.com/dart-lang/sdk/issues/52876 Change-Id: I079b4c9f73c7d2c0146c30cf2cd570b91a1ecf36 Cq-Include-Trybots: luci.dart.try:vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-linux-release-x64-try,vm-aot-linux-product-x64-try,vm-aot-linux-release-x64-try,vm-aot-linux-debug-x64-try,vm-ffi-qemu-linux-release-riscv64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/313120 Reviewed-by: Ryan Macnak Commit-Queue: Tess Strickland --- runtime/vm/app_snapshot.cc | 6 +- .../compiler/assembler/object_pool_builder.h | 4 +- runtime/vm/dart.cc | 2 - runtime/vm/object.cc | 748 ++++++------------ runtime/vm/object.h | 16 +- 5 files changed, 262 insertions(+), 514 deletions(-) diff --git a/runtime/vm/app_snapshot.cc b/runtime/vm/app_snapshot.cc index 28abb88525f..f6ad759f7fe 100644 --- a/runtime/vm/app_snapshot.cc +++ b/runtime/vm/app_snapshot.cc @@ -6051,6 +6051,8 @@ class VMSerializationRoots : public SerializationRoots { s->AddBaseObject(Object::empty_array().ptr(), "Array", ""); s->AddBaseObject(Object::empty_instantiations_cache_array().ptr(), "Array", ""); + s->AddBaseObject(Object::empty_subtype_test_cache_array().ptr(), "Array", + ""); s->AddBaseObject(Object::dynamic_type().ptr(), "Type", ""); s->AddBaseObject(Object::void_type().ptr(), "Type", ""); s->AddBaseObject(Object::empty_type_arguments().ptr(), "TypeArguments", @@ -6086,8 +6088,6 @@ class VMSerializationRoots : public SerializationRoots { s->AddBaseObject(ICData::cached_icdata_arrays_[i], "Array", ""); } - s->AddBaseObject(SubtypeTestCache::cached_array_, "Array", - ""); ClassTable* table = s->isolate_group()->class_table(); for (intptr_t cid = kFirstInternalOnlyCid; cid <= kLastInternalOnlyCid; @@ -6174,6 +6174,7 @@ class VMDeserializationRoots : public DeserializationRoots { d->AddBaseObject(Object::optimized_out().ptr()); d->AddBaseObject(Object::empty_array().ptr()); d->AddBaseObject(Object::empty_instantiations_cache_array().ptr()); + d->AddBaseObject(Object::empty_subtype_test_cache_array().ptr()); d->AddBaseObject(Object::dynamic_type().ptr()); d->AddBaseObject(Object::void_type().ptr()); d->AddBaseObject(Object::empty_type_arguments().ptr()); @@ -6197,7 +6198,6 @@ class VMDeserializationRoots : public DeserializationRoots { for (intptr_t i = 0; i < ICData::kCachedICDataArrayCount; i++) { d->AddBaseObject(ICData::cached_icdata_arrays_[i]); } - d->AddBaseObject(SubtypeTestCache::cached_array_); ClassTable* table = d->isolate_group()->class_table(); for (intptr_t cid = kFirstInternalOnlyCid; cid <= kLastInternalOnlyCid; diff --git a/runtime/vm/compiler/assembler/object_pool_builder.h b/runtime/vm/compiler/assembler/object_pool_builder.h index 98247ab5e7b..76c0c802ace 100644 --- a/runtime/vm/compiler/assembler/object_pool_builder.h +++ b/runtime/vm/compiler/assembler/object_pool_builder.h @@ -21,13 +21,13 @@ bool IsSameObject(const Object& a, const Object& b); struct ObjectPoolBuilderEntry { enum Patchability { - kPatchable, + kPatchable = 0, kNotPatchable, }; enum EntryType { + kImmediate = 0, kTaggedObject, - kImmediate, kNativeFunction, // Used only during AOT snapshot serialization/deserialization. diff --git a/runtime/vm/dart.cc b/runtime/vm/dart.cc index 0535b778e18..9d2191522cf 100644 --- a/runtime/vm/dart.cc +++ b/runtime/vm/dart.cc @@ -400,7 +400,6 @@ char* Dart::DartInit(const Dart_InitializeParams* params) { OffsetsTable::Init(); ArgumentsDescriptor::Init(); ICData::Init(); - SubtypeTestCache::Init(); if (params->vm_snapshot_data != nullptr) { #if defined(SUPPORT_TIMELINE) TimelineBeginEndScope tbes(Timeline::GetVMStream(), "ReadVMSnapshot"); @@ -747,7 +746,6 @@ char* Dart::Cleanup() { UserTags::Cleanup(); IsolateGroup::Cleanup(); ICData::Cleanup(); - SubtypeTestCache::Cleanup(); ArgumentsDescriptor::Cleanup(); OffsetsTable::Cleanup(); FfiCallbackMetadata::Cleanup(); diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index d2d5e14db08..ed966bda539 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -119,8 +119,6 @@ static const intptr_t kInitPrefixLength = strlen(kInitPrefix); // A cache of VM heap allocated preinitialized empty ic data entry arrays. ArrayPtr ICData::cached_icdata_arrays_[kCachedICDataArrayCount]; -// A VM heap allocated preinitialized empty subtype entry array. -ArrayPtr SubtypeTestCache::cached_array_; cpp_vtable Object::builtin_vtables_[kNumPredefinedCids] = {}; @@ -757,6 +755,7 @@ void Object::Init(IsolateGroup* isolate_group) { // allocated (RAW_NULL is not available). *empty_array_ = Array::null(); *empty_instantiations_cache_array_ = Array::null(); + *empty_subtype_test_cache_array_ = Array::null(); Class& cls = Class::Handle(); @@ -1038,6 +1037,28 @@ void Object::Init(IsolateGroup* isolate_group) { empty_instantiations_cache_array_->SetCanonical(); } + // Allocate and initialize the empty subtype test cache array instance, + // which contains a single unoccupied entry. + { + const intptr_t array_size = SubtypeTestCache::kTestEntryLength; + uword address = + heap->Allocate(thread, Array::InstanceSize(array_size), Heap::kOld); + InitializeObjectVariant(address, kImmutableArrayCid, array_size); + Array::initializeHandle(empty_subtype_test_cache_array_, + static_cast(address + kHeapObjectTag)); + empty_subtype_test_cache_array_->untag()->set_length(Smi::New(array_size)); + // Make the first (and only) entry unoccupied by setting its first element + // to the null value. + empty_subtype_test_cache_array_->SetAt( + SubtypeTestCache::kInstanceCidOrSignature, Object::null_object()); + smi = TypeArguments::Cache::Sentinel(); + SubtypeTestCacheTable table(*empty_subtype_test_cache_array_); + table.At(0).Set( + Object::null_object()); + // The other contents of the array are immaterial. + empty_subtype_test_cache_array_->SetCanonical(); + } + // Allocate and initialize the canonical empty context scope object. { uword address = @@ -1272,6 +1293,8 @@ void Object::Init(IsolateGroup* isolate_group) { ASSERT(empty_array_->IsArray()); ASSERT(!empty_instantiations_cache_array_->IsSmi()); ASSERT(empty_instantiations_cache_array_->IsArray()); + ASSERT(!empty_subtype_test_cache_array_->IsSmi()); + ASSERT(empty_subtype_test_cache_array_->IsArray()); ASSERT(!empty_type_arguments_->IsSmi()); ASSERT(empty_type_arguments_->IsTypeArguments()); ASSERT(!empty_context_scope_->IsSmi()); @@ -3101,12 +3124,7 @@ TypePtr Class::RareType() const { template ClassPtr Class::New(IsolateGroup* isolate_group, bool register_class) { ASSERT(Object::class_class() != Class::null()); - Class& result = Class::Handle(); - { - auto raw = Object::Allocate(Heap::kOld); - NoSafepointScope no_safepoint; - result = raw; - } + const auto& result = Class::Handle(Object::Allocate(Heap::kOld)); Object::VerifyBuiltinVtable(FakeObject::kClassId); NOT_IN_PRECOMPILED(result.set_token_pos(TokenPosition::kNoSource)); NOT_IN_PRECOMPILED(result.set_end_token_pos(TokenPosition::kNoSource)); @@ -3202,7 +3220,7 @@ void Class::set_can_be_future(bool value) const { } // Initialize class fields of type Array with empty array. -void Class::InitEmptyFields() { +void Class::InitEmptyFields() const { if (Object::empty_array().ptr() == Array::null()) { // The empty array has not been initialized yet. return; @@ -5014,12 +5032,7 @@ bool Class::InjectCIDFields() const { template ClassPtr Class::NewCommon(intptr_t index) { ASSERT(Object::class_class() != Class::null()); - Class& result = Class::Handle(); - { - auto raw = Object::Allocate(Heap::kOld); - NoSafepointScope no_safepoint; - result = raw; - } + const auto& result = Class::Handle(Object::Allocate(Heap::kOld)); // Here kIllegalCid means not-yet-assigned. Object::VerifyBuiltinVtable(index == kIllegalCid ? kInstanceCid : index); @@ -11471,9 +11484,10 @@ void FfiTrampolineData::set_callback_kind(FfiCallbackKind kind) const { FfiTrampolineDataPtr FfiTrampolineData::New() { ASSERT(Object::ffi_trampoline_data_class() != Class::null()); - auto data = Object::Allocate(Heap::kOld); - data->untag()->callback_id_ = -1; - return data; + const auto& data = FfiTrampolineData::Handle( + Object::Allocate(Heap::kOld)); + data.set_callback_id(-1); + return data.ptr(); } const char* FfiTrampolineData::ToCString() const { @@ -13200,11 +13214,6 @@ StringPtr Script::GetSnippet(intptr_t from_line, return String::SubString(src, start, end - start); } -ScriptPtr Script::New() { - ASSERT(Object::script_class() != Class::null()); - return Object::Allocate