From 26bc8bb4013a984d9e7a3e8feff8b1058458f349 Mon Sep 17 00:00:00 2001 From: Leszek Swirski Date: Wed, 23 Nov 2022 15:06:55 +0100 Subject: [PATCH] [ext-code-space] Make process-wide code range leaky Make the process-wide code range a once-initialised leaky object, rather than having a global weak_ptr + per-heap shared pointers and allowing it to be collected when all Isolates die. These weak pointers add locking overhead when accessing the code range, which shows up in GC and deoptimization traces when attempting to calculate Code objects from PCs. The process-wide pointer compression cage is already leaky, so it makes sense for the code range to be similar. Bug: v8:11460 Change-Id: Ibebd468ebad9eafe8aec49f575cdbf604e4b6cc0 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4051201 Reviewed-by: Igor Sheludko Reviewed-by: Michael Lippautz Commit-Queue: Leszek Swirski Cr-Commit-Position: refs/heads/main@{#84462} --- src/execution/isolate.cc | 3 +- src/heap/code-range.cc | 61 ++++++++++++-------------- src/heap/code-range.h | 7 +-- src/heap/heap.cc | 28 ++++++------ src/heap/heap.h | 14 +++++- src/init/isolate-allocator.cc | 3 +- src/objects/code.cc | 2 +- src/snapshot/embedded/embedded-data.cc | 2 +- src/snapshot/embedded/embedded-data.h | 2 +- 9 files changed, 64 insertions(+), 58 deletions(-) diff --git a/src/execution/isolate.cc b/src/execution/isolate.cc index b5a70d8dc7..7143c51cfd 100644 --- a/src/execution/isolate.cc +++ b/src/execution/isolate.cc @@ -4277,8 +4277,7 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data, // Additionally, enable if there is already a process-wide CodeRange that // has re-embedded builtins. if (COMPRESS_POINTERS_IN_SHARED_CAGE_BOOL) { - std::shared_ptr code_range = - CodeRange::GetProcessWideCodeRange(); + CodeRange* code_range = CodeRange::GetProcessWideCodeRange(); if (code_range && code_range->embedded_blob_code_copy() != nullptr) { is_short_builtin_calls_enabled_ = true; } diff --git a/src/heap/code-range.cc b/src/heap/code-range.cc index 0b663a0c41..4a4ea1f28f 100644 --- a/src/heap/code-range.cc +++ b/src/heap/code-range.cc @@ -6,6 +6,7 @@ #include "src/base/bits.h" #include "src/base/lazy-instance.h" +#include "src/base/once.h" #include "src/codegen/constants-arch.h" #include "src/common/globals.h" #include "src/flags/flags.h" @@ -17,19 +18,10 @@ namespace internal { namespace { -// Mutex for creating process_wide_code_range_. -base::LazyMutex process_wide_code_range_creation_mutex_ = - LAZY_MUTEX_INITIALIZER; - -// Weak pointer holding the process-wide CodeRange, if one has been created. All -// Heaps hold a std::shared_ptr to this, so this is destroyed when no Heaps -// remain. -base::LazyInstance>::type process_wide_code_range_ = - LAZY_INSTANCE_INITIALIZER; - DEFINE_LAZY_LEAKY_OBJECT_GETTER(CodeRangeAddressHint, GetCodeRangeAddressHint) void FunctionInStaticBinaryForAddressHint() {} + } // anonymous namespace Address CodeRangeAddressHint::GetAddressHint(size_t code_range_size, @@ -298,35 +290,40 @@ uint8_t* CodeRange::RemapEmbeddedBuiltins(Isolate* isolate, return embedded_blob_code_copy; } -// static -std::shared_ptr CodeRange::EnsureProcessWideCodeRange( - v8::PageAllocator* page_allocator, size_t requested_size) { - base::MutexGuard guard(process_wide_code_range_creation_mutex_.Pointer()); - std::shared_ptr code_range = process_wide_code_range_.Get().lock(); - if (!code_range) { - code_range = std::make_shared(); - if (!code_range->InitReservation(page_allocator, requested_size)) { - V8::FatalProcessOutOfMemory( - nullptr, "Failed to reserve virtual memory for CodeRange"); - } - *process_wide_code_range_.Pointer() = code_range; +namespace { + +CodeRange* process_wide_code_range_ = nullptr; + +V8_DECLARE_ONCE(init_code_range_once); +void InitProcessWideCodeRange(v8::PageAllocator* page_allocator, + size_t requested_size) { + CodeRange* code_range = new CodeRange(); + if (!code_range->InitReservation(page_allocator, requested_size)) { + V8::FatalProcessOutOfMemory( + nullptr, "Failed to reserve virtual memory for CodeRange"); + } + process_wide_code_range_ = code_range; #ifdef V8_EXTERNAL_CODE_SPACE #ifdef V8_COMPRESS_POINTERS_IN_SHARED_CAGE - // This might be not the first time we create a code range because previous - // code range instance could have been deleted if it wasn't kept alive by an - // active Isolate. Let the base be initialized from scratch once again. - ExternalCodeCompressionScheme::InitBase( - ExternalCodeCompressionScheme::PrepareCageBaseAddress( - code_range->base())); + ExternalCodeCompressionScheme::InitBase( + ExternalCodeCompressionScheme::PrepareCageBaseAddress( + code_range->base())); #endif // V8_COMPRESS_POINTERS_IN_SHARED_CAGE #endif // V8_EXTERNAL_CODE_SPACE - } - return code_range; +} +} // namespace + +// static +CodeRange* CodeRange::EnsureProcessWideCodeRange( + v8::PageAllocator* page_allocator, size_t requested_size) { + base::CallOnce(&init_code_range_once, InitProcessWideCodeRange, + page_allocator, requested_size); + return process_wide_code_range_; } // static -std::shared_ptr CodeRange::GetProcessWideCodeRange() { - return process_wide_code_range_.Get().lock(); +CodeRange* CodeRange::GetProcessWideCodeRange() { + return process_wide_code_range_; } } // namespace internal diff --git a/src/heap/code-range.h b/src/heap/code-range.h index 4fcea5f26f..e783ce27fb 100644 --- a/src/heap/code-range.h +++ b/src/heap/code-range.h @@ -11,6 +11,7 @@ #include "src/base/platform/mutex.h" #include "src/common/globals.h" #include "src/utils/allocation.h" +#include "v8-internal.h" namespace v8 { namespace internal { @@ -129,12 +130,12 @@ class CodeRange final : public VirtualMemoryCage { const uint8_t* embedded_blob_code, size_t embedded_blob_code_size); - static std::shared_ptr EnsureProcessWideCodeRange( + static CodeRange* EnsureProcessWideCodeRange( v8::PageAllocator* page_allocator, size_t requested_size); // If InitializeProcessWideCodeRangeOnce has been called, returns the - // initialized CodeRange. Otherwise returns an empty std::shared_ptr. - V8_EXPORT_PRIVATE static std::shared_ptr GetProcessWideCodeRange(); + // initialized CodeRange. Otherwise returns a null pointer. + V8_EXPORT_PRIVATE static CodeRange* GetProcessWideCodeRange(); private: // Used when short builtin calls are enabled, where embedded builtins are diff --git a/src/heap/heap.cc b/src/heap/heap.cc index 4c06cd98ee..6e78b5946e 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -5445,20 +5445,20 @@ void Heap::SetUp(LocalHeap* main_thread_local_heap) { // When a target requires the code range feature, we put all code objects in // a contiguous range of virtual address space, so that they can call each // other with near calls. - if (COMPRESS_POINTERS_IN_SHARED_CAGE_BOOL) { - // When sharing a pointer cage among Isolates, also share the - // CodeRange. isolate_->page_allocator() is the process-wide pointer - // compression cage's PageAllocator. - code_range_ = CodeRange::EnsureProcessWideCodeRange( - isolate_->page_allocator(), requested_size); - } else { - code_range_ = std::make_shared(); - if (!code_range_->InitReservation(isolate_->page_allocator(), - requested_size)) { - V8::FatalProcessOutOfMemory( - isolate_, "Failed to reserve virtual memory for CodeRange"); - } +#ifdef V8_COMPRESS_POINTERS_IN_SHARED_CAGE + // When sharing a pointer cage among Isolates, also share the + // CodeRange. isolate_->page_allocator() is the process-wide pointer + // compression cage's PageAllocator. + code_range_ = CodeRange::EnsureProcessWideCodeRange( + isolate_->page_allocator(), requested_size); +#else + code_range_ = std::make_unique(); + if (!code_range_->InitReservation(isolate_->page_allocator(), + requested_size)) { + V8::FatalProcessOutOfMemory( + isolate_, "Failed to reserve virtual memory for CodeRange"); } +#endif // V8_COMPRESS_POINTERS_IN_SHARED_CAGE LOG(isolate_, NewEvent("CodeRange", @@ -6964,7 +6964,7 @@ CodeLookupResult Heap::GcSafeFindCodeForInnerPointer( // Put useful info on the stack for debugging and crash the process. // TODO(1241665): Remove once the issue is solved. - std::shared_ptr code_range = CodeRange::GetProcessWideCodeRange(); + CodeRange* code_range = CodeRange::GetProcessWideCodeRange(); void* code_range_embedded_blob_code_copy = code_range ? code_range->embedded_blob_code_copy() : nullptr; Address flags = (isolate()->is_short_builtin_calls_enabled() ? 1 : 0) | diff --git a/src/heap/heap.h b/src/heap/heap.h index 187aedaa8b..4e63aa046f 100644 --- a/src/heap/heap.h +++ b/src/heap/heap.h @@ -893,7 +893,13 @@ class Heap { // range if it exists or empty region otherwise. const base::AddressRegion& code_region(); - CodeRange* code_range() { return code_range_.get(); } + CodeRange* code_range() { +#if V8_COMPRESS_POINTERS_IN_SHARED_CAGE + return code_range_; +#else + return code_range_.get(); +#endif + } // The base of the code range if it exists or null address. inline Address code_range_base(); @@ -2311,7 +2317,11 @@ class Heap { // // Owned by the heap when !V8_COMPRESS_POINTERS_IN_SHARED_CAGE, otherwise is // process-wide. - std::shared_ptr code_range_; +#if V8_COMPRESS_POINTERS_IN_SHARED_CAGE + CodeRange* code_range_ = nullptr; +#else + std::unique_ptr code_range_; +#endif // The embedder owns the C++ heap. v8::CppHeap* cpp_heap_ = nullptr; diff --git a/src/init/isolate-allocator.cc b/src/init/isolate-allocator.cc index 4e62a644f5..435c020487 100644 --- a/src/init/isolate-allocator.cc +++ b/src/init/isolate-allocator.cc @@ -64,8 +64,7 @@ DEFINE_LAZY_LEAKY_OBJECT_GETTER(VirtualMemoryCage, GetProcessWidePtrComprCage) // static void IsolateAllocator::FreeProcessWidePtrComprCageForTesting() { - if (std::shared_ptr code_range = - CodeRange::GetProcessWideCodeRange()) { + if (CodeRange* code_range = CodeRange::GetProcessWideCodeRange()) { code_range->Free(); } GetProcessWidePtrComprCage()->Free(); diff --git a/src/objects/code.cc b/src/objects/code.cc index 46d9edb471..a95ae9b9ee 100644 --- a/src/objects/code.cc +++ b/src/objects/code.cc @@ -49,7 +49,7 @@ inline EmbeddedData EmbeddedDataWithMaybeRemappedEmbeddedBuiltins( // copy of the re-embedded builtins in the shared CodeRange, so use that if // it's present. if (v8_flags.jitless) return EmbeddedData::FromBlob(); - CodeRange* code_range = CodeRange::GetProcessWideCodeRange().get(); + CodeRange* code_range = CodeRange::GetProcessWideCodeRange(); return (code_range && code_range->embedded_blob_code_copy() != nullptr) ? EmbeddedData::FromBlob(code_range) : EmbeddedData::FromBlob(); diff --git a/src/snapshot/embedded/embedded-data.cc b/src/snapshot/embedded/embedded-data.cc index 1260f1a642..f7c8944329 100644 --- a/src/snapshot/embedded/embedded-data.cc +++ b/src/snapshot/embedded/embedded-data.cc @@ -101,7 +101,7 @@ Builtin OffHeapInstructionStream::TryLookupCode(Isolate* isolate, // isolate uses it or knows about it or not (see // Code::OffHeapInstructionStart()). // So, this blob has to be checked too. - CodeRange* code_range = CodeRange::GetProcessWideCodeRange().get(); + CodeRange* code_range = CodeRange::GetProcessWideCodeRange(); if (code_range && code_range->embedded_blob_code_copy() != nullptr) { builtin = i::TryLookupCode(EmbeddedData::FromBlob(code_range), address); } diff --git a/src/snapshot/embedded/embedded-data.h b/src/snapshot/embedded/embedded-data.h index 4c5a1f998a..90add5d87e 100644 --- a/src/snapshot/embedded/embedded-data.h +++ b/src/snapshot/embedded/embedded-data.h @@ -108,7 +108,7 @@ class EmbeddedData final { // isolate uses it or knows about it or not (see // Code::OffHeapInstructionStart()). // So, this blob has to be checked too. - CodeRange* code_range = CodeRange::GetProcessWideCodeRange().get(); + CodeRange* code_range = CodeRange::GetProcessWideCodeRange(); if (code_range && code_range->embedded_blob_code_copy() != nullptr) { EmbeddedData remapped_d = EmbeddedData::FromBlob(code_range); // If the pc does not belong to the embedded code blob we should be