From f4d0d7e9fdc2b49bcd59e364ff90d7af864de4c5 Mon Sep 17 00:00:00 2001 From: Michael Achenbach Date: Sun, 30 Sep 2018 09:29:04 +0000 Subject: [PATCH] Revert "Create a fast path to get migration target when updating map" This reverts commit c285380ca8b7ef93c7dc1558bb26f5c51a6af0ea. Reason for revert: Lots of dcheck failures on GPU bots, e.g.: https://ci.chromium.org/p/v8/builders/luci.v8.ci/Win%20V8%20FYI%20Release%20(NVIDIA)/1997 https://ci.chromium.org/p/v8/builders/luci.v8.ci/Linux%20V8%20FYI%20Release%20(NVIDIA)/2770 Original change's description: > Create a fast path to get migration target when updating map > > During map updating, store the pointer to new map in the > raw_transitions slot of the old map that is deprecated from map > transition tree. Thus, we can get the migration target directly > instead of TryReplayPropertyTransitions when updating map. > > This can improve Speedometer2.0 Elm-TodoMVC case by ~5% on ATOM > Chromebook and ~9% on big-core Ubuntu. > > Change-Id: I56f9ce5183bbdd567b964890f623ef0ceed9b7db > Reviewed-on: https://chromium-review.googlesource.com/1233433 > Commit-Queue: Shiyu Zhang > Reviewed-by: Igor Sheludko > Cr-Commit-Position: refs/heads/master@{#56303} TBR=ishell@chromium.org,shiyu.zhang@intel.com Change-Id: I9b268d662cfa3a7fec577468eafe6570389252bc No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/1253104 Reviewed-by: Michael Achenbach Commit-Queue: Michael Achenbach Cr-Commit-Position: refs/heads/master@{#56305} --- src/map-updater.cc | 1 - src/objects-printer.cc | 1 - src/objects.cc | 51 ++++--------------------- src/objects/map.h | 1 - src/transitions-inl.h | 2 - src/transitions.cc | 27 ++----------- src/transitions.h | 9 ----- test/cctest/test-field-type-tracking.cc | 17 --------- 8 files changed, 10 insertions(+), 99 deletions(-) diff --git a/src/map-updater.cc b/src/map-updater.cc index f08d2ff67b..470215a310 100644 --- a/src/map-updater.cc +++ b/src/map-updater.cc @@ -170,7 +170,6 @@ Handle MapUpdater::Update() { if (FindTargetMap() == kEnd) return result_map_; ConstructNewMap(); DCHECK_EQ(kEnd, state_); - TransitionsAccessor(isolate_, old_map_).SetMigrationTarget(*result_map_); return result_map_; } diff --git a/src/objects-printer.cc b/src/objects-printer.cc index 69d84f88f3..e058337935 100644 --- a/src/objects-printer.cc +++ b/src/objects-printer.cc @@ -2386,7 +2386,6 @@ void TransitionsAccessor::PrintTransitions(std::ostream& os) { // NOLINT switch (encoding()) { case kPrototypeInfo: case kUninitialized: - case kMigrationTarget: return; case kWeakRef: { Map* target = Map::cast(raw_transitions_->GetHeapObjectAssumeWeak()); diff --git a/src/objects.cc b/src/objects.cc index d268a996af..dec2fcd8c7 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -4882,25 +4882,6 @@ Handle Map::ReconfigureElementsKind(Isolate* isolate, Handle map, return mu.ReconfigureElementsKind(new_elements_kind); } -namespace { - -Map* SearchMigrationTarget(Isolate* isolate, Map* old_map) { - DisallowHeapAllocation no_allocation; - DisallowDeoptimization no_deoptimization(isolate); - - Map* target = old_map; - do { - target = TransitionsAccessor(isolate, target, &no_allocation) - .GetMigrationTarget(); - } while (target != nullptr && target->is_deprecated()); - SLOW_DCHECK(target == nullptr || - Map::TryUpdateSlow(isolate, old_map) == nullptr || - Map::TryUpdateSlow(isolate, old_map) == target); - return target; -} -} // namespace - -// TODO(ishell): Move TryUpdate() and friends to MapUpdater // static MaybeHandle Map::TryUpdate(Isolate* isolate, Handle old_map) { DisallowHeapAllocation no_allocation; @@ -4908,22 +4889,6 @@ MaybeHandle Map::TryUpdate(Isolate* isolate, Handle old_map) { if (!old_map->is_deprecated()) return old_map; - Map* target_map = SearchMigrationTarget(isolate, *old_map); - if (target_map != nullptr) { - return handle(target_map, isolate); - } - - Map* new_map = TryUpdateSlow(isolate, *old_map); - if (new_map == nullptr) return MaybeHandle(); - TransitionsAccessor(isolate, *old_map, &no_allocation) - .SetMigrationTarget(new_map); - return handle(new_map, isolate); -} - -Map* Map::TryUpdateSlow(Isolate* isolate, Map* old_map) { - DisallowHeapAllocation no_allocation; - DisallowDeoptimization no_deoptimization(isolate); - // Check the state of the root map. Map* root_map = old_map->FindRootMap(isolate); if (root_map->is_deprecated()) { @@ -4932,21 +4897,23 @@ Map* Map::TryUpdateSlow(Isolate* isolate, Map* old_map) { DCHECK(constructor->initial_map()->is_dictionary_map()); if (constructor->initial_map()->elements_kind() != old_map->elements_kind()) { - return nullptr; + return MaybeHandle(); } - return constructor->initial_map(); + return handle(constructor->initial_map(), constructor->GetIsolate()); } - if (!old_map->EquivalentToForTransition(root_map)) return nullptr; + if (!old_map->EquivalentToForTransition(root_map)) return MaybeHandle(); ElementsKind from_kind = root_map->elements_kind(); ElementsKind to_kind = old_map->elements_kind(); if (from_kind != to_kind) { // Try to follow existing elements kind transitions. root_map = root_map->LookupElementsTransitionMap(isolate, to_kind); - if (root_map == nullptr) return nullptr; + if (root_map == nullptr) return MaybeHandle(); // From here on, use the map with correct elements kind as root map. } - return root_map->TryReplayPropertyTransitions(isolate, old_map); + Map* new_map = root_map->TryReplayPropertyTransitions(isolate, *old_map); + if (new_map == nullptr) return MaybeHandle(); + return handle(new_map, isolate); } Map* Map::TryReplayPropertyTransitions(Isolate* isolate, Map* old_map) { @@ -5028,10 +4995,6 @@ Map* Map::TryReplayPropertyTransitions(Isolate* isolate, Map* old_map) { // static Handle Map::Update(Isolate* isolate, Handle map) { if (!map->is_deprecated()) return map; - Map* target_map = SearchMigrationTarget(isolate, *map); - if (target_map != nullptr) { - return handle(target_map, isolate); - } MapUpdater mu(isolate, map); return mu.Update(); } diff --git a/src/objects/map.h b/src/objects/map.h index ed5dfbdc00..5f6b173cd3 100644 --- a/src/objects/map.h +++ b/src/objects/map.h @@ -632,7 +632,6 @@ class Map : public HeapObject { // is found. static MaybeHandle TryUpdate(Isolate* isolate, Handle map) V8_WARN_UNUSED_RESULT; - static Map* TryUpdateSlow(Isolate* isolate, Map* map) V8_WARN_UNUSED_RESULT; // Returns a non-deprecated version of the input. This method may deprecate // existing maps along the way if encodings conflict. Not for use while diff --git a/src/transitions-inl.h b/src/transitions-inl.h index b5ac9667f3..072e15318b 100644 --- a/src/transitions-inl.h +++ b/src/transitions-inl.h @@ -65,7 +65,6 @@ Name* TransitionsAccessor::GetKey(int transition_number) { switch (encoding()) { case kPrototypeInfo: case kUninitialized: - case kMigrationTarget: UNREACHABLE(); return nullptr; case kWeakRef: { @@ -119,7 +118,6 @@ Map* TransitionsAccessor::GetTarget(int transition_number) { switch (encoding()) { case kPrototypeInfo: case kUninitialized: - case kMigrationTarget: UNREACHABLE(); return nullptr; case kWeakRef: diff --git a/src/transitions.cc b/src/transitions.cc index 90113a946d..6c55f53b03 100644 --- a/src/transitions.cc +++ b/src/transitions.cc @@ -21,12 +21,9 @@ void TransitionsAccessor::Initialize() { } else if (raw_transitions_->GetHeapObjectIfStrong(&heap_object)) { if (heap_object->IsTransitionArray()) { encoding_ = kFullTransitionArray; - } else if (heap_object->IsPrototypeInfo()) { - encoding_ = kPrototypeInfo; } else { - DCHECK(map_->is_deprecated()); - DCHECK(heap_object->IsMap()); - encoding_ = kMigrationTarget; + DCHECK(heap_object->IsPrototypeInfo()); + encoding_ = kPrototypeInfo; } } else { UNREACHABLE(); @@ -51,7 +48,6 @@ bool TransitionsAccessor::HasSimpleTransitionTo(Map* map) { return raw_transitions_->GetHeapObjectAssumeWeak() == map; case kPrototypeInfo: case kUninitialized: - case kMigrationTarget: case kFullTransitionArray: return false; } @@ -216,7 +212,6 @@ Map* TransitionsAccessor::SearchTransition(Name* name, PropertyKind kind, switch (encoding()) { case kPrototypeInfo: case kUninitialized: - case kMigrationTarget: return nullptr; case kWeakRef: { Map* map = Map::cast(raw_transitions_->GetHeapObjectAssumeWeak()); @@ -269,7 +264,6 @@ Handle TransitionsAccessor::ExpectedTransitionKey() { switch (encoding()) { case kPrototypeInfo: case kUninitialized: - case kMigrationTarget: case kFullTransitionArray: return Handle::null(); case kWeakRef: { @@ -436,7 +430,6 @@ int TransitionsAccessor::NumberOfTransitions() { switch (encoding()) { case kPrototypeInfo: case kUninitialized: - case kMigrationTarget: return 0; case kWeakRef: return 1; @@ -447,18 +440,6 @@ int TransitionsAccessor::NumberOfTransitions() { return 0; // Make GCC happy. } -void TransitionsAccessor::SetMigrationTarget(Map* migration_target) { - DCHECK(map_->is_deprecated()); - ReplaceTransitions(MaybeObject::FromObject(migration_target)); -} - -Map* TransitionsAccessor::GetMigrationTarget() { - if (encoding() == kMigrationTarget) { - return map_->raw_transitions()->cast(); - } - return nullptr; -} - void TransitionArray::Zap(Isolate* isolate) { MemsetPointer( data_start() + kPrototypeTransitionsIndex, @@ -493,8 +474,7 @@ void TransitionsAccessor::SetPrototypeTransitions( void TransitionsAccessor::EnsureHasFullTransitionArray() { if (encoding() == kFullTransitionArray) return; - int nof = - (encoding() == kUninitialized || encoding() == kMigrationTarget) ? 0 : 1; + int nof = encoding() == kUninitialized ? 0 : 1; Handle result = isolate_->factory()->NewTransitionArray(nof); Reload(); // Reload after possible GC. if (nof == 1) { @@ -517,7 +497,6 @@ void TransitionsAccessor::TraverseTransitionTreeInternal( switch (encoding()) { case kPrototypeInfo: case kUninitialized: - case kMigrationTarget: break; case kWeakRef: { Map* simple_target = diff --git a/src/transitions.h b/src/transitions.h index 479ca2813a..9684815239 100644 --- a/src/transitions.h +++ b/src/transitions.h @@ -106,14 +106,6 @@ class TransitionsAccessor { void PutPrototypeTransition(Handle prototype, Handle target_map); Handle GetPrototypeTransition(Handle prototype); - // During the first-time Map::Update and Map::TryUpdate, the migration target - // map could be cached in the raw_transitions slot of the old map that is - // deprecated from the map transition tree. The next time old map is updated, - // we will check this cache slot as a shortcut to get the migration target - // map. - void SetMigrationTarget(Map* migration_target); - Map* GetMigrationTarget(); - #if DEBUG || OBJECT_PRINT void PrintTransitions(std::ostream& os); static void PrintOneTransition(std::ostream& os, Name* key, Map* target); @@ -133,7 +125,6 @@ class TransitionsAccessor { enum Encoding { kPrototypeInfo, kUninitialized, - kMigrationTarget, kWeakRef, kFullTransitionArray, }; diff --git a/test/cctest/test-field-type-tracking.cc b/test/cctest/test-field-type-tracking.cc index 400514b162..f40dbe83bd 100644 --- a/test/cctest/test-field-type-tracking.cc +++ b/test/cctest/test-field-type-tracking.cc @@ -78,14 +78,6 @@ static Handle CreateAccessorPair(bool with_getter, return pair; } -// Check cached migration target map after Map::Update() and Map::TryUpdate() -static void CheckMigrationTarget(Isolate* isolate, Map* old_map, Map* new_map) { - Map* target = TransitionsAccessor(isolate, handle(old_map, isolate)) - .GetMigrationTarget(); - if (!target) return; - CHECK_EQ(new_map, target); - CHECK_EQ(Map::TryUpdateSlow(isolate, old_map), target); -} class Expectations { static const int MAX_PROPERTIES = 10; @@ -715,7 +707,6 @@ static void TestGeneralizeField(int detach_property_at_index, // Update all deprecated maps and check that they are now the same. Handle updated_map = Map::Update(isolate, map); CHECK_EQ(*new_map, *updated_map); - CheckMigrationTarget(isolate, *map, *updated_map); } static void TestGeneralizeField(const CRFTData& from, const CRFTData& to, @@ -976,11 +967,9 @@ TEST(GeneralizeFieldWithAccessorProperties) { // Update all deprecated maps and check that they are now the same. Handle updated_map = Map::Update(isolate, map); CHECK_EQ(*active_map, *updated_map); - CheckMigrationTarget(isolate, *map, *updated_map); for (int i = 0; i < kPropCount; i++) { updated_map = Map::Update(isolate, maps[i]); CHECK_EQ(*active_map, *updated_map); - CheckMigrationTarget(isolate, *maps[i], *updated_map); } } @@ -1071,7 +1060,6 @@ static void TestReconfigureDataFieldAttribute_GeneralizeField( // Update deprecated |map|, it should become |new_map|. Handle updated_map = Map::Update(isolate, map); CHECK_EQ(*new_map, *updated_map); - CheckMigrationTarget(isolate, *map, *updated_map); } // This test ensures that trivial field generalization (from HeapObject to @@ -1382,7 +1370,6 @@ struct CheckDeprecated { // Update deprecated |map|, it should become |new_map|. Handle updated_map = Map::Update(isolate, map); CHECK_EQ(*new_map, *updated_map); - CheckMigrationTarget(isolate, *map, *updated_map); } }; @@ -1841,7 +1828,6 @@ static void TestReconfigureElementsKind_GeneralizeField( // Update deprecated |map|, it should become |new_map|. Handle updated_map = Map::Update(isolate, map); CHECK_EQ(*new_map, *updated_map); - CheckMigrationTarget(isolate, *map, *updated_map); // Ensure Map::FindElementsKindTransitionedMap() is able to find the // transitioned map. @@ -2353,11 +2339,9 @@ static void TestGeneralizeFieldWithSpecialTransition(TestConfig& config, // Update all deprecated maps and check that they are now the same. Handle updated_map = Map::Update(isolate, map); CHECK_EQ(*active_map, *updated_map); - CheckMigrationTarget(isolate, *map, *updated_map); for (int i = 0; i < kPropCount; i++) { updated_map = Map::Update(isolate, maps[i]); CHECK_EQ(*active_map, *updated_map); - CheckMigrationTarget(isolate, *maps[i], *updated_map); } } @@ -2639,7 +2623,6 @@ struct FieldGeneralizationChecker { CHECK_NE(*map1, *map2); Handle updated_map = Map::Update(isolate, map1); CHECK_EQ(*map2, *updated_map); - CheckMigrationTarget(isolate, *map1, *updated_map); expectations2.SetDataField(descriptor_, attributes_, constness_, representation_, heap_type_);