From 4ebc9b7b0d3cba55b7e49fa1cd349f697d37a780 Mon Sep 17 00:00:00 2001 From: Victor Gomes Date: Mon, 17 Jan 2022 16:40:04 +0100 Subject: [PATCH] Reland "[runtime] Adds LocalNameIterator" This is a reland of f605d77822df392b83dd26c59d31dfdde188687c Adds a GC safe (using handles) and unsafe versions of the iterator. V8HeapExplorer needs an unsafe one, since it does not allow the creation of handles. Original change's description: > [runtime] Adds LocalNameIterator > > ScopeInfo will contain either inlined (array) local names or > a hash table (names => index) containing the local names. > > We abstract iteration with LocalNameIterator and remove > ContextLocalName since accessing a local name by index in > the hash table would be expensive. > > This CL only implements the iterator for the array. > > Bug: v8:12315 > Change-Id: I2c62802652fca1cf47815ce8768a3f7487f2c39f > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3386603 > Reviewed-by: Toon Verwaest > Commit-Queue: Victor Gomes > Cr-Commit-Position: refs/heads/main@{#78623} Bug: v8:12315 Change-Id: I6288a08b9c342cd3a9cabcb621c40bb44c08c9c4 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3394706 Reviewed-by: Camillo Bruni Reviewed-by: Toon Verwaest Commit-Queue: Victor Gomes Cr-Commit-Position: refs/heads/main@{#78653} --- src/ast/scopes.cc | 3 +- src/debug/debug-interface.cc | 20 ++++----- src/debug/debug-scopes.cc | 6 +-- src/execution/execution.cc | 6 +-- src/execution/frames.cc | 6 +-- src/objects/scope-info-inl.h | 60 +++++++++++++++++++++++++ src/objects/scope-info.cc | 5 ++- src/objects/scope-info.h | 14 +++++- src/profiler/heap-snapshot-generator.cc | 9 ++-- 9 files changed, 98 insertions(+), 31 deletions(-) diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc index e9e3f9a313..3d5d92ae8b 100644 --- a/src/ast/scopes.cc +++ b/src/ast/scopes.cc @@ -470,7 +470,8 @@ Scope* Scope::DeserializeScopeChain(IsolateT* isolate, Zone* zone, DCHECK_EQ(scope_info.ContextLocalCount(), 1); DCHECK_EQ(scope_info.ContextLocalMode(0), VariableMode::kVar); DCHECK_EQ(scope_info.ContextLocalInitFlag(0), kCreatedInitialized); - String name = scope_info.ContextLocalName(0); + DCHECK(scope_info.HasInlinedLocalNames()); + String name = scope_info.ContextInlinedLocalName(0); MaybeAssignedFlag maybe_assigned = scope_info.ContextLocalMaybeAssignedFlag(0); outer_scope = diff --git a/src/debug/debug-interface.cc b/src/debug/debug-interface.cc index 1b9f19f424..684347b0f0 100644 --- a/src/debug/debug-interface.cc +++ b/src/debug/debug-interface.cc @@ -111,17 +111,15 @@ void CollectPrivateMethodsAndAccessorsFromContext( i::IsStaticFlag is_static_flag, std::vector>* names_out, std::vector>* values_out) { i::Handle scope_info(context->scope_info(), isolate); - int local_count = scope_info->ContextLocalCount(); - for (int j = 0; j < local_count; ++j) { - i::VariableMode mode = scope_info->ContextLocalMode(j); - i::IsStaticFlag flag = scope_info->ContextLocalIsStaticFlag(j); + for (auto it : i::ScopeInfo::IterateLocalNames(scope_info)) { + i::Handle name(it->name(), isolate); + i::VariableMode mode = scope_info->ContextLocalMode(it->index()); + i::IsStaticFlag flag = scope_info->ContextLocalIsStaticFlag(it->index()); if (!i::IsPrivateMethodOrAccessorVariableMode(mode) || flag != is_static_flag) { continue; } - - i::Handle name(scope_info->ContextLocalName(j), isolate); - int context_index = scope_info->ContextHeaderLength() + j; + int context_index = scope_info->ContextHeaderLength() + it->index(); i::Handle slot_value(context->get(context_index), isolate); DCHECK_IMPLIES(mode == i::VariableMode::kPrivateMethod, slot_value->IsJSFunction()); @@ -1001,11 +999,9 @@ void GlobalLexicalScopeNames(v8::Local v8_context, i::ScriptContextTable::GetContext(isolate, table, i); DCHECK(script_context->IsScriptContext()); i::Handle scope_info(script_context->scope_info(), isolate); - int local_count = scope_info->ContextLocalCount(); - for (int j = 0; j < local_count; ++j) { - i::String name = scope_info->ContextLocalName(j); - if (i::ScopeInfo::VariableIsSynthetic(name)) continue; - names->Append(Utils::ToLocal(handle(name, isolate))); + for (auto it : i::ScopeInfo::IterateLocalNames(scope_info)) { + if (i::ScopeInfo::VariableIsSynthetic(it->name())) continue; + names->Append(Utils::ToLocal(handle(it->name(), isolate))); } } } diff --git a/src/debug/debug-scopes.cc b/src/debug/debug-scopes.cc index 93f7667486..fc0e0aef51 100644 --- a/src/debug/debug-scopes.cc +++ b/src/debug/debug-scopes.cc @@ -780,10 +780,10 @@ bool ScopeIterator::VisitContextLocals(const Visitor& visitor, Handle context, ScopeType scope_type) const { // Fill all context locals to the context extension. - for (int i = 0; i < scope_info->ContextLocalCount(); ++i) { - Handle name(scope_info->ContextLocalName(i), isolate_); + for (auto it : ScopeInfo::IterateLocalNames(scope_info)) { + Handle name(it->name(), isolate_); if (ScopeInfo::VariableIsSynthetic(*name)) continue; - int context_index = scope_info->ContextHeaderLength() + i; + int context_index = scope_info->ContextHeaderLength() + it->index(); Handle value(context->get(context_index), isolate_); if (visitor(name, value, scope_type)) return true; } diff --git a/src/execution/execution.cc b/src/execution/execution.cc index 736553f57a..c829e5f644 100644 --- a/src/execution/execution.cc +++ b/src/execution/execution.cc @@ -207,9 +207,9 @@ MaybeHandle NewScriptContext(Isolate* isolate, native_context->script_context_table(), isolate); // Find name clashes. - for (int var = 0; var < scope_info->ContextLocalCount(); var++) { - Handle name(scope_info->ContextLocalName(var), isolate); - VariableMode mode = scope_info->ContextLocalMode(var); + for (auto it : ScopeInfo::IterateLocalNames(scope_info)) { + Handle name(it->name(), isolate); + VariableMode mode = scope_info->ContextLocalMode(it->index()); VariableLookupResult lookup; if (ScriptContextTable::Lookup(isolate, *script_context, *name, &lookup)) { if (IsLexicalVariableMode(mode) || IsLexicalVariableMode(lookup.mode)) { diff --git a/src/execution/frames.cc b/src/execution/frames.cc index 35511a9ddf..04a9c613d6 100644 --- a/src/execution/frames.cc +++ b/src/execution/frames.cc @@ -2342,12 +2342,12 @@ void JavaScriptFrame::Print(StringStream* accumulator, PrintMode mode, if (heap_locals_count > 0) { accumulator->Add(" // heap-allocated locals\n"); } - for (int i = 0; i < heap_locals_count; i++) { + for (auto it : ScopeInfo::IterateLocalNames(&scope_info, no_gc)) { accumulator->Add(" var "); - accumulator->PrintName(scope_info.ContextLocalName(i)); + accumulator->PrintName(it->name()); accumulator->Add(" = "); if (!context.is_null()) { - int slot_index = Context::MIN_CONTEXT_SLOTS + i; + int slot_index = Context::MIN_CONTEXT_SLOTS + it->index(); if (slot_index < context.length()) { accumulator->Add("%o", context.get(slot_index)); } else { diff --git a/src/objects/scope-info-inl.h b/src/objects/scope-info-inl.h index db54388017..ba59fc263e 100644 --- a/src/objects/scope-info-inl.h +++ b/src/objects/scope-info-inl.h @@ -36,6 +36,66 @@ bool ScopeInfo::HasInlinedLocalNames() const { return ContextLocalCount() < kScopeInfoMaxInlinedLocalNamesSize; } +template +class ScopeInfo::LocalNamesIterator { + public: + class Iterator { + public: + Iterator(ScopeInfoPtr scope_info, int index) + : scope_info_(scope_info), index_(index) {} + + Iterator& operator++() { + index_++; + return *this; + } + + friend bool operator==(const Iterator& a, const Iterator& b) { + return *a.scope_info_ == *b.scope_info_ && a.index_ == b.index_; + } + friend bool operator!=(const Iterator& a, const Iterator& b) { + return !(a == b); + } + + String name() const { + DCHECK_LT(index_, scope_info_->context_local_count()); + return scope_info_->context_local_names(index_); + } + + const Iterator* operator*() const { return this; } + + int index() const { return index_; } + + private: + ScopeInfoPtr scope_info_; + int index_; + }; + + explicit LocalNamesIterator(ScopeInfoPtr scope_info) + : scope_info_(scope_info) {} + + inline Iterator begin() const { return Iterator(scope_info_, 0); } + + inline Iterator end() const { + return Iterator(scope_info_, scope_info_->ContextLocalCount()); + } + + private: + ScopeInfoPtr scope_info_; +}; + +// static +ScopeInfo::LocalNamesIterator> ScopeInfo::IterateLocalNames( + Handle scope_info) { + return LocalNamesIterator>(scope_info); +} + +// static +ScopeInfo::LocalNamesIterator ScopeInfo::IterateLocalNames( + ScopeInfo* scope_info, const DisallowGarbageCollection& no_gc) { + USE(no_gc); + return LocalNamesIterator(scope_info); +} + } // namespace internal } // namespace v8 diff --git a/src/objects/scope-info.cc b/src/objects/scope-info.cc index 47499378f3..982ecbc4d0 100644 --- a/src/objects/scope-info.cc +++ b/src/objects/scope-info.cc @@ -814,7 +814,8 @@ SourceTextModuleInfo ScopeInfo::ModuleDescriptorInfo() const { return SourceTextModuleInfo::cast(module_info()); } -String ScopeInfo::ContextLocalName(int var) const { +String ScopeInfo::ContextInlinedLocalName(int var) const { + DCHECK(HasInlinedLocalNames()); return context_local_names(var); } @@ -923,7 +924,7 @@ std::pair ScopeInfo::SavedClassVariable() const { int index = saved_class_variable_info() - Context::MIN_CONTEXT_SLOTS; DCHECK_GE(index, 0); DCHECK_LT(index, ContextLocalCount()); - String name = ContextLocalName(index); + String name = ContextInlinedLocalName(index); return std::make_pair(name, index); } else { // The saved class variable info corresponds to the offset in the hash diff --git a/src/objects/scope-info.h b/src/objects/scope-info.h index 25e03875a9..6baf9af43c 100644 --- a/src/objects/scope-info.h +++ b/src/objects/scope-info.h @@ -143,8 +143,18 @@ class ScopeInfo : public TorqueGeneratedScopeInfo { // Return true if the local names are inlined in the scope info object. inline bool HasInlinedLocalNames() const; - // Return the name of the given context local. - String ContextLocalName(int var) const; + template + class LocalNamesIterator; + + static inline LocalNamesIterator> IterateLocalNames( + Handle scope_info); + + static inline LocalNamesIterator IterateLocalNames( + ScopeInfo* scope_info, const DisallowGarbageCollection& no_gc); + + // Return the name of a given context local. + // It should only be used if inlined local names. + String ContextInlinedLocalName(int var) const; // Return the mode of the given context local. VariableMode ContextLocalMode(int var) const; diff --git a/src/profiler/heap-snapshot-generator.cc b/src/profiler/heap-snapshot-generator.cc index fb9cceb125..c8abf12e8b 100644 --- a/src/profiler/heap-snapshot-generator.cc +++ b/src/profiler/heap-snapshot-generator.cc @@ -1040,14 +1040,13 @@ static const struct { void V8HeapExplorer::ExtractContextReferences(HeapEntry* entry, Context context) { + DisallowGarbageCollection no_gc; if (!context.IsNativeContext() && context.is_declaration_context()) { ScopeInfo scope_info = context.scope_info(); // Add context allocated locals. - int context_locals = scope_info.ContextLocalCount(); - for (int i = 0; i < context_locals; ++i) { - String local_name = scope_info.ContextLocalName(i); - int idx = scope_info.ContextHeaderLength() + i; - SetContextReference(entry, local_name, context.get(idx), + for (auto it : ScopeInfo::IterateLocalNames(&scope_info, no_gc)) { + int idx = scope_info.ContextHeaderLength() + it->index(); + SetContextReference(entry, it->name(), context.get(idx), Context::OffsetOfElementAt(idx)); } if (scope_info.HasContextAllocatedFunctionName()) {