From 407ca1be87ba3f65293cf7187116914c012a43f0 Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Tue, 2 Jun 2020 21:29:03 +0000 Subject: [PATCH] [vm, api] Add Dart_UpdateExternalSize to the embedding API. Allows an embedder (or native extension) to inform the VM when external memory is released before a weak handle is finalized. Bug: https://github.com/dart-lang/sdk/issues/42078 Change-Id: Ifffd0c160e5305bc6e6752207a2315139f245e2f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/149245 Commit-Queue: Ryan Macnak Reviewed-by: Vyacheslav Egorov Reviewed-by: Martin Kustermann --- runtime/include/dart_api.h | 3 + runtime/vm/benchmark_test.cc | 14 ++-- runtime/vm/dart_api_impl.cc | 14 +++- runtime/vm/dart_api_impl_test.cc | 129 ++++++++++++++++++++++++++++-- runtime/vm/dart_api_state.h | 24 ++++-- runtime/vm/exceptions_test.cc | 3 +- runtime/vm/heap/heap.cc | 18 ++--- runtime/vm/heap/heap.h | 6 +- runtime/vm/heap/pages.cc | 15 ---- runtime/vm/heap/pages.h | 13 ++- runtime/vm/heap/scavenger.cc | 11 --- runtime/vm/heap/scavenger.h | 12 ++- runtime/vm/raw_object_snapshot.cc | 2 +- 13 files changed, 194 insertions(+), 70 deletions(-) diff --git a/runtime/include/dart_api.h b/runtime/include/dart_api.h index 425c0680c92..6a1a401e467 100644 --- a/runtime/include/dart_api.h +++ b/runtime/include/dart_api.h @@ -489,6 +489,9 @@ Dart_NewWeakPersistentHandle(Dart_Handle object, DART_EXPORT void Dart_DeleteWeakPersistentHandle( Dart_WeakPersistentHandle object); +DART_EXPORT void Dart_UpdateExternalSize(Dart_WeakPersistentHandle object, + intptr_t external_allocation_size); + /* * ========================== * Initialization and Globals diff --git a/runtime/vm/benchmark_test.cc b/runtime/vm/benchmark_test.cc index c7512c4ca28..f76f58b6c9d 100644 --- a/runtime/vm/benchmark_test.cc +++ b/runtime/vm/benchmark_test.cc @@ -280,9 +280,8 @@ BENCHMARK(UseDartApi) { " }\n" "}\n"; - Dart_Handle lib = TestCase::LoadTestScript( - kScriptChars, reinterpret_cast(bm_uda_lookup), - RESOLVED_USER_TEST_URI, false); + Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, bm_uda_lookup, + RESOLVED_USER_TEST_URI, false); Dart_Handle result = Dart_FinalizeLoading(false); EXPECT_VALID(result); @@ -381,9 +380,7 @@ BENCHMARK(KernelServiceCompileAll) { EXPECT_VALID(result); Dart_Handle service_lib = Dart_LookupLibrary(NewString("dart:vmservice_io")); ASSERT(!Dart_IsError(service_lib)); - Dart_SetNativeResolver( - service_lib, reinterpret_cast(NativeResolver), - NULL); + Dart_SetNativeResolver(service_lib, NativeResolver, NULL); result = Dart_FinalizeLoading(false); EXPECT_VALID(result); @@ -491,9 +488,8 @@ BENCHMARK(FrameLookup) { " return obj.method1(1);" " }" "}"; - Dart_Handle lib = TestCase::LoadTestScript( - kScriptChars, - reinterpret_cast(StackFrameNativeResolver)); + Dart_Handle lib = + TestCase::LoadTestScript(kScriptChars, StackFrameNativeResolver); Dart_Handle cls = Dart_GetClass(lib, NewString("StackFrameTest")); Dart_Handle result = Dart_Invoke(cls, NewString("testMain"), 0, NULL); EXPECT_VALID(result); diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc index 25b513f1f54..75e22e7d79d 100644 --- a/runtime/vm/dart_api_impl.cc +++ b/runtime/vm/dart_api_impl.cc @@ -1000,6 +1000,18 @@ Dart_NewWeakPersistentHandle(Dart_Handle object, external_allocation_size, callback); } +DART_EXPORT void Dart_UpdateExternalSize(Dart_WeakPersistentHandle object, + intptr_t external_size) { + IsolateGroup* isolate_group = IsolateGroup::Current(); + CHECK_ISOLATE_GROUP(isolate_group); + NoSafepointScope no_safepoint_scope; + ApiState* state = isolate_group->api_state(); + ASSERT(state != NULL); + ASSERT(state->IsActiveWeakPersistentHandle(object)); + auto weak_ref = FinalizablePersistentHandle::Cast(object); + weak_ref->UpdateExternalSize(external_size, isolate_group); +} + DART_EXPORT void Dart_DeletePersistentHandle(Dart_PersistentHandle object) { IsolateGroup* isolate_group = IsolateGroup::Current(); CHECK_ISOLATE_GROUP(isolate_group); @@ -1023,7 +1035,7 @@ DART_EXPORT void Dart_DeleteWeakPersistentHandle( ASSERT(state != NULL); ASSERT(state->IsActiveWeakPersistentHandle(object)); auto weak_ref = FinalizablePersistentHandle::Cast(object); - weak_ref->EnsureFreeExternal(isolate_group); + weak_ref->EnsureFreedExternal(isolate_group); state->FreeWeakPersistentHandle(weak_ref); } diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc index 65c9ea15cfb..e329d2cf9d1 100644 --- a/runtime/vm/dart_api_impl_test.cc +++ b/runtime/vm/dart_api_impl_test.cc @@ -3512,6 +3512,125 @@ TEST_CASE(DartAPI_WeakPersistentHandleExternalAllocationSizeOddReferents) { } } +#define EXAMPLE_RESOURCE_NATIVE_LIST(V) \ + V(ExampleResource_Allocate, 1) \ + V(ExampleResource_Use, 1) \ + V(ExampleResource_Dispose, 1) + +EXAMPLE_RESOURCE_NATIVE_LIST(DECLARE_FUNCTION); + +static struct NativeEntries { + const char* name_; + Dart_NativeFunction function_; + int argument_count_; +} ExampleResourceEntries[] = {EXAMPLE_RESOURCE_NATIVE_LIST(REGISTER_FUNCTION)}; + +static Dart_NativeFunction ExampleResourceNativeResolver( + Dart_Handle name, + int argument_count, + bool* auto_setup_scope) { + const char* function_name = nullptr; + Dart_Handle result = Dart_StringToCString(name, &function_name); + ASSERT(!Dart_IsError(result)); + ASSERT(function_name != nullptr); + ASSERT(auto_setup_scope != nullptr); + *auto_setup_scope = true; + int num_entries = + sizeof(ExampleResourceEntries) / sizeof(struct NativeEntries); + for (int i = 0; i < num_entries; i++) { + struct NativeEntries* entry = &(ExampleResourceEntries[i]); + if ((strcmp(function_name, entry->name_) == 0) && + (entry->argument_count_ == argument_count)) { + return reinterpret_cast(entry->function_); + } + } + return nullptr; +} + +struct ExampleResource { + Dart_WeakPersistentHandle self; + void* lots_of_memory; +}; + +void ExampleResourceFinalizer(void* isolate_peer, + Dart_WeakPersistentHandle handle, + void* peer) { + ExampleResource* resource = reinterpret_cast(peer); + free(resource->lots_of_memory); + delete resource; +} + +void FUNCTION_NAME(ExampleResource_Allocate)(Dart_NativeArguments native_args) { + Dart_Handle receiver = Dart_GetNativeArgument(native_args, 0); + intptr_t external_size = 10 * MB; + ExampleResource* resource = new ExampleResource(); + resource->lots_of_memory = malloc(external_size); + resource->self = Dart_NewWeakPersistentHandle( + receiver, resource, external_size, ExampleResourceFinalizer); + EXPECT_VALID(Dart_SetNativeInstanceField( + receiver, 0, reinterpret_cast(resource))); + // Some pretend resource initialization. + *reinterpret_cast(resource->lots_of_memory) = 123; +} + +void FUNCTION_NAME(ExampleResource_Use)(Dart_NativeArguments native_args) { + Dart_Handle receiver = Dart_GetNativeArgument(native_args, 0); + intptr_t native_field = 0; + EXPECT_VALID(Dart_GetNativeInstanceField(receiver, 0, &native_field)); + ExampleResource* resource = reinterpret_cast(native_field); + if (resource->lots_of_memory == nullptr) { + Dart_ThrowException(Dart_NewStringFromCString( + "Attempt to use a disposed ExampleResource!")); + UNREACHABLE(); + } else { + // Some pretend resource use. + EXPECT_EQ(123, *reinterpret_cast(resource->lots_of_memory)); + } +} + +void FUNCTION_NAME(ExampleResource_Dispose)(Dart_NativeArguments native_args) { + Dart_Handle receiver = Dart_GetNativeArgument(native_args, 0); + intptr_t native_field = 0; + EXPECT_VALID(Dart_GetNativeInstanceField(receiver, 0, &native_field)); + ExampleResource* resource = reinterpret_cast(native_field); + if (resource->lots_of_memory != nullptr) { + free(resource->lots_of_memory); + resource->lots_of_memory = nullptr; + Dart_UpdateExternalSize(resource->self, 0); + } +} + +TEST_CASE(DartAPI_WeakPersistentHandleUpdateSize) { + const char* kScriptChars = R"( + import "dart:nativewrappers"; + class ExampleResource extends NativeFieldWrapperClass1 { + ExampleResource() { _allocate(); } + void _allocate() native "ExampleResource_Allocate"; + void use() native "ExampleResource_Use"; + void dispose() native "ExampleResource_Dispose"; + } + main() { + var res = new ExampleResource(); + res.use(); + res.dispose(); + res.dispose(); // Idempotent + bool threw = false; + try { + res.use(); + } catch (_) { + threw = true; + } + if (!threw) { + throw "Exception expected"; + } + } + )"; + + Dart_Handle lib = + TestCase::LoadTestScript(kScriptChars, ExampleResourceNativeResolver); + EXPECT_VALID(Dart_Invoke(lib, NewString("main"), 0, NULL)); +} + static Dart_WeakPersistentHandle weak1 = NULL; static Dart_WeakPersistentHandle weak2 = NULL; static Dart_WeakPersistentHandle weak3 = NULL; @@ -5845,8 +5964,7 @@ TEST_CASE(DartAPI_ThrowException) { Dart_EnterScope(); // Start a Dart API scope for invoking API functions. // Load up a test script which extends the native wrapper class. - Dart_Handle lib = TestCase::LoadTestScript( - kScriptChars, reinterpret_cast(native_lookup)); + Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, native_lookup); // Throwing an exception here should result in an error. result = Dart_ThrowException(NewString("This doesn't work")); @@ -6032,9 +6150,7 @@ TEST_CASE(DartAPI_GetNativeArguments) { " obj2);" "}"; - Dart_Handle lib = TestCase::LoadTestScript( - kScriptChars, - reinterpret_cast(native_args_lookup)); + Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, native_args_lookup); const char* ascii_str = "string"; intptr_t ascii_str_length = strlen(ascii_str); @@ -6075,8 +6191,7 @@ TEST_CASE(DartAPI_GetNativeArgumentCount) { " return obj.method1(77, 125);" "}"; - Dart_Handle lib = TestCase::LoadTestScript( - kScriptChars, reinterpret_cast(gnac_lookup)); + Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, gnac_lookup); Dart_Handle result = Dart_Invoke(lib, NewString("testMain"), 0, NULL); EXPECT_VALID(result); diff --git a/runtime/vm/dart_api_state.h b/runtime/vm/dart_api_state.h index a986121ad92..5b22c935cf9 100644 --- a/runtime/vm/dart_api_state.h +++ b/runtime/vm/dart_api_state.h @@ -219,29 +219,39 @@ class FinalizablePersistentHandle { if (SpaceForExternal() == Heap::kNew) { SetExternalNewSpaceBit(); } - isolate_group->heap()->AllocateExternal( - raw()->GetClassIdMayBeSmi(), external_size(), SpaceForExternal()); + isolate_group->heap()->AllocatedExternal(external_size(), + SpaceForExternal()); + } + void UpdateExternalSize(intptr_t size, IsolateGroup* isolate_group) { + ASSERT(size >= 0); + intptr_t old_size = external_size(); + set_external_size(size); + if (size > old_size) { + isolate_group->heap()->AllocatedExternal(size - old_size, + SpaceForExternal()); + } else { + isolate_group->heap()->FreedExternal(old_size - size, SpaceForExternal()); + } } // Called when the referent becomes unreachable. void UpdateUnreachable(IsolateGroup* isolate_group) { - EnsureFreeExternal(isolate_group); + EnsureFreedExternal(isolate_group); Finalize(isolate_group, this); } // Called when the referent has moved, potentially between generations. void UpdateRelocated(IsolateGroup* isolate_group) { if (IsSetNewSpaceBit() && (SpaceForExternal() == Heap::kOld)) { - isolate_group->heap()->PromoteExternal(raw()->GetClassIdMayBeSmi(), - external_size()); + isolate_group->heap()->PromotedExternal(external_size()); ClearExternalNewSpaceBit(); } } // Idempotent. Called when the handle is explicitly deleted or the // referent becomes unreachable. - void EnsureFreeExternal(IsolateGroup* isolate_group) { - isolate_group->heap()->FreeExternal(external_size(), SpaceForExternal()); + void EnsureFreedExternal(IsolateGroup* isolate_group) { + isolate_group->heap()->FreedExternal(external_size(), SpaceForExternal()); set_external_size(0); } diff --git a/runtime/vm/exceptions_test.cc b/runtime/vm/exceptions_test.cc index 722ffef89e1..0e62b997d94 100644 --- a/runtime/vm/exceptions_test.cc +++ b/runtime/vm/exceptions_test.cc @@ -114,8 +114,7 @@ TEST_CASE(UnhandledExceptions) { " UnhandledExceptions.equals(2, Second.method1(1));\n" " UnhandledExceptions.equals(3, Second.method3(1));\n" "}"; - Dart_Handle lib = TestCase::LoadTestScript( - kScriptChars, reinterpret_cast(native_lookup)); + Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, native_lookup); EXPECT_VALID(Dart_Invoke(lib, NewString("testMain"), 0, NULL)); } diff --git a/runtime/vm/heap/heap.cc b/runtime/vm/heap/heap.cc index d5ec58b64ad..28a36fec940 100644 --- a/runtime/vm/heap/heap.cc +++ b/runtime/vm/heap/heap.cc @@ -158,11 +158,11 @@ uword Heap::AllocateOld(intptr_t size, OldPage::PageType type) { return 0; } -void Heap::AllocateExternal(intptr_t cid, intptr_t size, Space space) { +void Heap::AllocatedExternal(intptr_t size, Space space) { ASSERT(Thread::Current()->no_safepoint_scope_depth() == 0); if (space == kNew) { Isolate::Current()->AssertCurrentThreadIsMutator(); - new_space_.AllocateExternal(cid, size); + new_space_.AllocatedExternal(size); if (new_space_.ExternalInWords() <= (4 * new_space_.CapacityInWords())) { return; } @@ -173,7 +173,7 @@ void Heap::AllocateExternal(intptr_t cid, intptr_t size, Space space) { // space GC check. } else { ASSERT(space == kOld); - old_space_.AllocateExternal(cid, size); + old_space_.AllocatedExternal(size); } if (old_space_.NeedsGarbageCollection()) { @@ -183,18 +183,18 @@ void Heap::AllocateExternal(intptr_t cid, intptr_t size, Space space) { } } -void Heap::FreeExternal(intptr_t size, Space space) { +void Heap::FreedExternal(intptr_t size, Space space) { if (space == kNew) { - new_space_.FreeExternal(size); + new_space_.FreedExternal(size); } else { ASSERT(space == kOld); - old_space_.FreeExternal(size); + old_space_.FreedExternal(size); } } -void Heap::PromoteExternal(intptr_t cid, intptr_t size) { - new_space_.FreeExternal(size); - old_space_.PromoteExternal(cid, size); +void Heap::PromotedExternal(intptr_t size) { + new_space_.FreedExternal(size); + old_space_.AllocatedExternal(size); } bool Heap::Contains(uword addr) const { diff --git a/runtime/vm/heap/heap.h b/runtime/vm/heap/heap.h index b833d515e96..ac74209027a 100644 --- a/runtime/vm/heap/heap.h +++ b/runtime/vm/heap/heap.h @@ -95,10 +95,10 @@ class Heap { } // Track external data. - void AllocateExternal(intptr_t cid, intptr_t size, Space space); - void FreeExternal(intptr_t size, Space space); + void AllocatedExternal(intptr_t size, Space space); + void FreedExternal(intptr_t size, Space space); // Move external size from new to old space. Does not by itself trigger GC. - void PromoteExternal(intptr_t cid, intptr_t size); + void PromotedExternal(intptr_t size); // Heap contains the specified address. bool Contains(uword addr) const; diff --git a/runtime/vm/heap/pages.cc b/runtime/vm/heap/pages.cc index 796d64c2799..4a7c0bbc48b 100644 --- a/runtime/vm/heap/pages.cc +++ b/runtime/vm/heap/pages.cc @@ -569,21 +569,6 @@ void PageSpace::ReleaseLock(FreeList* freelist) { freelist->mutex()->Unlock(); } -void PageSpace::AllocateExternal(intptr_t cid, intptr_t size) { - intptr_t size_in_words = size >> kWordSizeLog2; - usage_.external_in_words += size_in_words; -} - -void PageSpace::PromoteExternal(intptr_t cid, intptr_t size) { - intptr_t size_in_words = size >> kWordSizeLog2; - usage_.external_in_words += size_in_words; -} - -void PageSpace::FreeExternal(intptr_t size) { - intptr_t size_in_words = size >> kWordSizeLog2; - usage_.external_in_words -= size_in_words; -} - class BasePageIterator : ValueObject { public: explicit BasePageIterator(const PageSpace* space) : space_(space) {} diff --git a/runtime/vm/heap/pages.h b/runtime/vm/heap/pages.h index 951e95b1b9f..3e789ea5166 100644 --- a/runtime/vm/heap/pages.h +++ b/runtime/vm/heap/pages.h @@ -409,9 +409,16 @@ class PageSpace { allocated_black_in_words_.fetch_add(size >> kWordSizeLog2); } - void AllocateExternal(intptr_t cid, intptr_t size); - void PromoteExternal(intptr_t cid, intptr_t size); - void FreeExternal(intptr_t size); + void AllocatedExternal(intptr_t size) { + ASSERT(size >= 0); + intptr_t size_in_words = size >> kWordSizeLog2; + usage_.external_in_words += size_in_words; + } + void FreedExternal(intptr_t size) { + ASSERT(size >= 0); + intptr_t size_in_words = size >> kWordSizeLog2; + usage_.external_in_words -= size_in_words; + } // Bulk data allocation. FreeList* DataFreeList(intptr_t i = 0) { diff --git a/runtime/vm/heap/scavenger.cc b/runtime/vm/heap/scavenger.cc index 489e47dd547..3c73a1e0ff1 100644 --- a/runtime/vm/heap/scavenger.cc +++ b/runtime/vm/heap/scavenger.cc @@ -1545,17 +1545,6 @@ void Scavenger::PrintToJSONObject(JSONObject* object) const { } #endif // !PRODUCT -void Scavenger::AllocateExternal(intptr_t cid, intptr_t size) { - ASSERT(size >= 0); - external_size_ += size; -} - -void Scavenger::FreeExternal(intptr_t size) { - ASSERT(size >= 0); - external_size_ -= size; - ASSERT(external_size_ >= 0); -} - void Scavenger::Evacuate() { // We need a safepoint here to prevent allocation right before or right after // the scavenge. diff --git a/runtime/vm/heap/scavenger.h b/runtime/vm/heap/scavenger.h index 098d96fe431..81db5fc921f 100644 --- a/runtime/vm/heap/scavenger.h +++ b/runtime/vm/heap/scavenger.h @@ -314,8 +314,16 @@ class Scavenger { void PrintToJSONObject(JSONObject* object) const; #endif // !PRODUCT - void AllocateExternal(intptr_t cid, intptr_t size); - void FreeExternal(intptr_t size); + void AllocatedExternal(intptr_t size) { + ASSERT(size >= 0); + external_size_ += size; + ASSERT(external_size_ >= 0); + } + void FreedExternal(intptr_t size) { + ASSERT(size >= 0); + external_size_ -= size; + ASSERT(external_size_ >= 0); + } void MakeNewSpaceIterable(); int64_t FreeSpaceInWords(Isolate* isolate) const; diff --git a/runtime/vm/raw_object_snapshot.cc b/runtime/vm/raw_object_snapshot.cc index 2f9a9a6c5b5..9eed51a1951 100644 --- a/runtime/vm/raw_object_snapshot.cc +++ b/runtime/vm/raw_object_snapshot.cc @@ -1711,7 +1711,7 @@ void TransferableTypedDataLayout::WriteTo(SnapshotWriter* writer, [](void* data, Dart_WeakPersistentHandle handle, void* peer) { TransferableTypedDataPeer* tpeer = reinterpret_cast(peer); - tpeer->handle()->EnsureFreeExternal(IsolateGroup::Current()); + tpeer->handle()->EnsureFreedExternal(IsolateGroup::Current()); tpeer->ClearData(); }); }