From 71f9c1179ac784f9fb3ff66c15ca5792e1329290 Mon Sep 17 00:00:00 2001 From: Toon Verwaest Date: Tue, 28 Apr 2020 15:55:28 +0200 Subject: [PATCH] Reland "[runtime] Amortize descriptor array growing for fast-mode prototypes" This is a reland of 2de2d3dcdc2dd9842ca383c8fc192485dbd6e9dd Original change's description: > [runtime] Amortize descriptor array growing for fast-mode prototypes > > This avoids an O(n^2) algorithm that creates an equal amount of garbage. > Even though the actual final descriptor array might be a little bigger, > it reduces peak memory usage by allocating less. > > Bug: b:148346655 > Change-Id: I984159d36e9e0b37c19bc81afc90c94c9a9d168a > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2135728 > Commit-Queue: Toon Verwaest > Reviewed-by: Igor Sheludko > Cr-Commit-Position: refs/heads/master@{#67031} Bug: b:148346655, v8:10339 Change-Id: I24436d8f49dc1fe527c4f6558db1abcba323b6f8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2139215 Reviewed-by: Ulan Degenbaev Reviewed-by: Igor Sheludko Auto-Submit: Toon Verwaest Commit-Queue: Igor Sheludko Cr-Commit-Position: refs/heads/master@{#67475} --- src/diagnostics/objects-debug.cc | 8 ++- src/heap/marking-visitor-inl.h | 9 ++- src/json/json-parser.cc | 2 +- src/objects/map-inl.h | 6 ++ src/objects/map.cc | 99 ++++++++++++++++++++++---------- src/objects/map.h | 5 ++ 6 files changed, 95 insertions(+), 34 deletions(-) diff --git a/src/diagnostics/objects-debug.cc b/src/diagnostics/objects-debug.cc index 9d145d1510..2e09c1c0a1 100644 --- a/src/diagnostics/objects-debug.cc +++ b/src/diagnostics/objects-debug.cc @@ -439,8 +439,12 @@ void Map::MapVerify(Isolate* isolate) { if (GetBackPointer().IsUndefined(isolate)) { // Root maps must not have descriptors in the descriptor array that do not // belong to the map. - CHECK_EQ(NumberOfOwnDescriptors(), - instance_descriptors().number_of_descriptors()); + int nof = instance_descriptors().number_of_descriptors(); + if (IsDetached(isolate)) { + CHECK(base::IsInRange(NumberOfOwnDescriptors(), 0, nof)); + } else { + CHECK_EQ(NumberOfOwnDescriptors(), nof); + } } else { // If there is a parent map it must be non-stable. Map parent = Map::cast(GetBackPointer()); diff --git a/src/heap/marking-visitor-inl.h b/src/heap/marking-visitor-inl.h index e162268e30..0883b15aa1 100644 --- a/src/heap/marking-visitor-inl.h +++ b/src/heap/marking-visitor-inl.h @@ -407,7 +407,14 @@ int MarkingVisitorBase::VisitMap(Map meta_map, DescriptorArray descriptors = map.synchronized_instance_descriptors(); size += MarkDescriptorArrayBlack(map, descriptors); int number_of_own_descriptors = map.NumberOfOwnDescriptors(); - if (number_of_own_descriptors) { + if (map.IsDetached(heap_->isolate())) { + // Mark all descriptors in detached maps. Descriptor arrays are reused as + // objects transition between maps, and its possible that a newer map dies + // before an older map. Marking all descriptors through each map + // guarantees that we don't end up with dangling references that we don't + // clear since there are no explicit transitions. + VisitDescriptors(descriptors, descriptors.number_of_descriptors()); + } else if (number_of_own_descriptors) { // It is possible that the concurrent marker observes the // number_of_own_descriptors out of sync with the descriptors. In that // case the marking write barrier for the descriptor array will ensure diff --git a/src/json/json-parser.cc b/src/json/json-parser.cc index becf21cd4e..da2f60d320 100644 --- a/src/json/json-parser.cc +++ b/src/json/json-parser.cc @@ -838,7 +838,7 @@ MaybeHandle JsonParser::ParseJsonValue() { Map maybe_feedback = JSObject::cast(*element_stack.back()).map(); // Don't consume feedback from objects with a map that's detached // from the transition tree. - if (!maybe_feedback.GetBackPointer().IsUndefined(isolate_)) { + if (!maybe_feedback.IsDetached(isolate_)) { feedback = handle(maybe_feedback, isolate_); if (feedback->is_deprecated()) { feedback = Map::Update(isolate_, feedback); diff --git a/src/objects/map-inl.h b/src/objects/map-inl.h index ebf6f5a9b3..449253aa62 100644 --- a/src/objects/map-inl.h +++ b/src/objects/map-inl.h @@ -123,6 +123,12 @@ bool Map::CanHaveFastTransitionableElementsKind() const { return CanHaveFastTransitionableElementsKind(instance_type()); } +bool Map::IsDetached(Isolate* isolate) const { + if (is_prototype_map()) return true; + return instance_type() == JS_OBJECT_TYPE && NumberOfOwnDescriptors() > 0 && + GetBackPointer().IsUndefined(isolate); +} + // static void Map::GeneralizeIfCanHaveTransitionableFastElementsKind( Isolate* isolate, InstanceType instance_type, diff --git a/src/objects/map.cc b/src/objects/map.cc index fb27fdafc0..80c3d40399 100644 --- a/src/objects/map.cc +++ b/src/objects/map.cc @@ -658,7 +658,7 @@ Map Map::FindRootMap(Isolate* isolate) const { if (back.IsUndefined(isolate)) { // Initial map must not contain descriptors in the descriptors array // that do not belong to the map. - DCHECK_EQ(result.NumberOfOwnDescriptors(), + DCHECK_LE(result.NumberOfOwnDescriptors(), result.instance_descriptors().number_of_descriptors()); return result; } @@ -1221,7 +1221,7 @@ Map Map::FindElementsKindTransitionedMap(Isolate* isolate, DisallowHeapAllocation no_allocation; DisallowDeoptimization no_deoptimization(isolate); - if (is_prototype_map()) return Map(); + if (IsDetached(isolate)) return Map(); ElementsKind kind = elements_kind(); bool packed = IsFastPackedElementsKind(kind); @@ -1354,7 +1354,7 @@ static Handle AddMissingElementsTransitions(Isolate* isolate, ElementsKind kind = map->elements_kind(); TransitionFlag flag; - if (map->is_prototype_map()) { + if (map->IsDetached(isolate)) { flag = OMIT_TRANSITION; } else { flag = INSERT_TRANSITION; @@ -1721,14 +1721,14 @@ void Map::ConnectTransition(Isolate* isolate, Handle parent, child->may_have_interesting_symbols()); if (!parent->GetBackPointer().IsUndefined(isolate)) { parent->set_owns_descriptors(false); - } else { + } else if (!parent->IsDetached(isolate)) { // |parent| is initial map and it must not contain descriptors in the // descriptors array that do not belong to the map. DCHECK_EQ(parent->NumberOfOwnDescriptors(), parent->instance_descriptors().number_of_descriptors()); } - if (parent->is_prototype_map()) { - DCHECK(child->is_prototype_map()); + if (parent->IsDetached(isolate)) { + DCHECK(child->IsDetached(isolate)); if (FLAG_trace_maps) { LOG(isolate, MapEvent("Transition", parent, child, "prototype", name)); } @@ -1755,7 +1755,9 @@ Handle Map::CopyReplaceDescriptors( result->set_may_have_interesting_symbols(true); } - if (!map->is_prototype_map()) { + if (map->is_prototype_map()) { + result->InitializeDescriptors(isolate, *descriptors, *layout_descriptor); + } else { if (flag == INSERT_TRANSITION && TransitionsAccessor(isolate, map).CanHaveMoreTransitions()) { result->InitializeDescriptors(isolate, *descriptors, *layout_descriptor); @@ -1766,19 +1768,11 @@ Handle Map::CopyReplaceDescriptors( descriptors->GeneralizeAllFields(); result->InitializeDescriptors(isolate, *descriptors, LayoutDescriptor::FastPointerLayout()); - // If we were trying to insert a transition but failed because there are - // too many transitions already, mark the object as a prototype to avoid - // tracking transitions from the detached map. - if (flag == INSERT_TRANSITION) { - result->set_is_prototype_map(true); - } } - } else { - result->InitializeDescriptors(isolate, *descriptors, *layout_descriptor); } if (FLAG_trace_maps && // Mirror conditions above that did not call ConnectTransition(). - (map->is_prototype_map() || + (map->IsDetached(isolate) || !(flag == INSERT_TRANSITION && TransitionsAccessor(isolate, map).CanHaveMoreTransitions()))) { LOG(isolate, MapEvent("ReplaceDescriptors", map, result, reason, @@ -1960,7 +1954,7 @@ Handle Map::AsLanguageMode(Isolate* isolate, Handle initial_map, } Handle Map::CopyForElementsTransition(Isolate* isolate, Handle map) { - DCHECK(!map->is_prototype_map()); + DCHECK(!map->IsDetached(isolate)); Handle new_map = CopyDropDescriptors(isolate, map); if (map->owns_descriptors()) { @@ -2161,7 +2155,7 @@ Handle Map::TransitionToDataProperty(Isolate* isolate, Handle map, StoreOrigin store_origin) { RuntimeCallTimerScope stats_scope( isolate, - map->is_prototype_map() + map->IsDetached(isolate) ? RuntimeCallCounterId::kPrototypeMap_TransitionToDataProperty : RuntimeCallCounterId::kMap_TransitionToDataProperty); @@ -2275,7 +2269,7 @@ Handle Map::TransitionToAccessorProperty(Isolate* isolate, Handle map, PropertyAttributes attributes) { RuntimeCallTimerScope stats_scope( isolate, - map->is_prototype_map() + map->IsDetached(isolate) ? RuntimeCallCounterId::kPrototypeMap_TransitionToAccessorProperty : RuntimeCallCounterId::kMap_TransitionToAccessorProperty); @@ -2390,19 +2384,64 @@ Handle Map::CopyAddDescriptor(Isolate* isolate, Handle map, return ShareDescriptor(isolate, map, descriptors, descriptor); } - int nof = map->NumberOfOwnDescriptors(); - Handle new_descriptors = - DescriptorArray::CopyUpTo(isolate, descriptors, nof, 1); - new_descriptors->Append(descriptor); + Handle new_descriptors; + Handle new_layout_descriptor; + if (map->IsDetached(isolate)) { + if (descriptors->number_of_slack_descriptors() == 0) { + int old_size = descriptors->number_of_descriptors(); + if (old_size == 0) { + new_descriptors = DescriptorArray::Allocate(isolate, 0, 1); + } else { + int slack = SlackForArraySize(old_size, kMaxNumberOfDescriptors); + EnsureDescriptorSlack(isolate, map, slack); + new_descriptors = handle(map->instance_descriptors(), isolate); + } + } else { + new_descriptors = descriptors; + } + new_layout_descriptor = + FLAG_unbox_double_fields + ? LayoutDescriptor::ShareAppend(isolate, map, + descriptor->GetDetails()) + : handle(LayoutDescriptor::FastPointerLayout(), isolate); + } else { + int nof = map->NumberOfOwnDescriptors(); + new_descriptors = DescriptorArray::CopyUpTo(isolate, descriptors, nof, 1); + new_layout_descriptor = + FLAG_unbox_double_fields + ? LayoutDescriptor::New(isolate, map, new_descriptors, nof + 1) + : handle(LayoutDescriptor::FastPointerLayout(), isolate); + } - Handle new_layout_descriptor = - FLAG_unbox_double_fields - ? LayoutDescriptor::New(isolate, map, new_descriptors, nof + 1) - : handle(LayoutDescriptor::FastPointerLayout(), isolate); + Handle result = CopyDropDescriptors(isolate, map); - return CopyReplaceDescriptors( - isolate, map, new_descriptors, new_layout_descriptor, flag, - descriptor->GetKey(), "CopyAddDescriptor", SIMPLE_PROPERTY_TRANSITION); + if (descriptor->GetKey()->IsInterestingSymbol()) { + result->set_may_have_interesting_symbols(true); + } + + { + DisallowHeapAllocation no_gc; + new_descriptors->Append(descriptor); + result->InitializeDescriptors(isolate, *new_descriptors, + *new_layout_descriptor); + } + + if (flag == INSERT_TRANSITION && + TransitionsAccessor(isolate, map).CanHaveMoreTransitions()) { + ConnectTransition(isolate, map, result, descriptor->GetKey(), + SIMPLE_PROPERTY_TRANSITION); + } + + if (FLAG_trace_maps && + // Mirror conditions above that did not call ConnectTransition(). + (map->IsDetached(isolate) || + !(flag == INSERT_TRANSITION && + TransitionsAccessor(isolate, map).CanHaveMoreTransitions()))) { + LOG(isolate, MapEvent("ReplaceDescriptors", map, result, + "CopyAddDescriptor", descriptor->GetKey())); + } + + return result; } Handle Map::CopyInsertDescriptor(Isolate* isolate, Handle map, diff --git a/src/objects/map.h b/src/objects/map.h index 30b59e3777..f2eddf02e5 100644 --- a/src/objects/map.h +++ b/src/objects/map.h @@ -422,6 +422,11 @@ class Map : public HeapObject { inline bool has_sealed_elements() const; inline bool has_frozen_elements() const; + // Weakly checks whether a map is detached from all transition trees. If this + // returns true, the map is guaranteed to be detached. If it returns false, + // there is no guarantee it is attached. + inline bool IsDetached(Isolate* isolate) const; + // Returns true if the current map doesn't have DICTIONARY_ELEMENTS but if a // map with DICTIONARY_ELEMENTS was found in the prototype chain. bool DictionaryElementsInPrototypeChainOnly(Isolate* isolate);