From b9d55654c82569668a8515f95b14c47e6754a253 Mon Sep 17 00:00:00 2001 From: Georg Neis Date: Wed, 29 May 2019 03:13:32 +0200 Subject: [PATCH] [turbofan] Don't overwrite jump target serialization environment A given target offset may already have an environment associated with it (there can be multiple jumps to the same target). In that case we used to throw away the previous environment. With this CL we merge the environments instead. Bug: v8:7790 Change-Id: I0c22182436fc48e29675e49627729a33cbeaaf4d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1631603 Commit-Queue: Georg Neis Auto-Submit: Georg Neis Reviewed-by: Maya Lekova Cr-Commit-Position: refs/heads/master@{#61905} --- .../serializer-for-background-compilation.cc | 50 ++++++++++--------- .../serializer-for-background-compilation.h | 13 ++++- test/cctest/compiler/serializer-tester.cc | 14 ++++++ 3 files changed, 52 insertions(+), 25 deletions(-) diff --git a/src/compiler/serializer-for-background-compilation.cc b/src/compiler/serializer-for-background-compilation.cc index 1193eb1908..03eedda667 100644 --- a/src/compiler/serializer-for-background-compilation.cc +++ b/src/compiler/serializer-for-background-compilation.cc @@ -106,10 +106,7 @@ class SerializerForBackgroundCompilation::Environment : public ZoneObject { DCHECK(!IsDead()); } - // When control flow bytecodes are encountered, e.g. a conditional jump, - // the current environment needs to be stashed together with the target jump - // address. Later, when this target bytecode is handled, the stashed - // environment will be merged into the current one. + // Merge {other} into {this} environment (leaving {other} unmodified). void Merge(Environment* other); FunctionBlueprint function() const { return function_; } @@ -297,7 +294,7 @@ SerializerForBackgroundCompilation::SerializerForBackgroundCompilation( dependencies_(dependencies), zone_(zone), environment_(new (zone) Environment(zone, {closure, broker_->isolate()})), - stashed_environments_(zone), + jump_target_environments_(zone), flags_(flags) { JSFunctionRef(broker, closure).Serialize(); } @@ -311,7 +308,7 @@ SerializerForBackgroundCompilation::SerializerForBackgroundCompilation( zone_(zone), environment_(new (zone) Environment(zone, broker_->isolate(), function, new_target, arguments)), - stashed_environments_(zone), + jump_target_environments_(zone), flags_(flags) { DCHECK(!(flags_ & SerializerForBackgroundCompilationFlag::kOsr)); TraceScope tracer( @@ -415,7 +412,7 @@ void SerializerForBackgroundCompilation::TraverseBytecode() { ExceptionHandlerMatcher handler_matcher(iterator); for (; !iterator.done(); iterator.Advance()) { - MergeAfterJump(&iterator); + IncorporateJumpTargetEnvironment(iterator.current_offset()); if (environment()->IsDead()) { if (iterator.current_bytecode() == @@ -773,22 +770,31 @@ void SerializerForBackgroundCompilation::ProcessCallVarArgs( ProcessCallOrConstruct(callee, base::nullopt, arguments, slot); } +void SerializerForBackgroundCompilation::ContributeToJumpTargetEnvironment( + int target_offset) { + auto it = jump_target_environments_.find(target_offset); + if (it == jump_target_environments_.end()) { + jump_target_environments_[target_offset] = + new (zone()) Environment(*environment()); + } else { + it->second->Merge(environment()); + } +} + +void SerializerForBackgroundCompilation::IncorporateJumpTargetEnvironment( + int target_offset) { + auto it = jump_target_environments_.find(target_offset); + if (it != jump_target_environments_.end()) { + environment()->Merge(it->second); + jump_target_environments_.erase(it); + } +} + void SerializerForBackgroundCompilation::ProcessJump( interpreter::BytecodeArrayIterator* iterator) { int jump_target = iterator->GetJumpTargetOffset(); - int current_offset = iterator->current_offset(); - if (current_offset >= jump_target) return; - - stashed_environments_[jump_target] = new (zone()) Environment(*environment()); -} - -void SerializerForBackgroundCompilation::MergeAfterJump( - interpreter::BytecodeArrayIterator* iterator) { - int current_offset = iterator->current_offset(); - auto stash = stashed_environments_.find(current_offset); - if (stash != stashed_environments_.end()) { - environment()->Merge(stash->second); - stashed_environments_.erase(stash); + if (iterator->current_offset() < jump_target) { + ContributeToJumpTargetEnvironment(jump_target); } } @@ -803,9 +809,7 @@ void SerializerForBackgroundCompilation::VisitSwitchOnSmiNoFeedback( interpreter::JumpTableTargetOffsets targets = iterator->GetJumpTableTargetOffsets(); for (const auto& target : targets) { - // TODO(neis): Here and in ProcessJump, don't overwrite stashed environment. - stashed_environments_[target.target_offset] = - new (zone()) Environment(*environment()); + ContributeToJumpTargetEnvironment(target.target_offset); } } diff --git a/src/compiler/serializer-for-background-compilation.h b/src/compiler/serializer-for-background-compilation.h index 4733306371..c94d6a9f43 100644 --- a/src/compiler/serializer-for-background-compilation.h +++ b/src/compiler/serializer-for-background-compilation.h @@ -317,7 +317,6 @@ class SerializerForBackgroundCompilation { bool with_spread = false); void ProcessJump(interpreter::BytecodeArrayIterator* iterator); - void MergeAfterJump(interpreter::BytecodeArrayIterator* iterator); void ProcessKeyedPropertyAccess(Hints const& receiver, Hints const& key, FeedbackSlot slot, AccessMode mode); @@ -339,6 +338,16 @@ class SerializerForBackgroundCompilation { base::Optional new_target, const HintsVector& arguments, bool with_spread); + // When (forward-)branching bytecodes are encountered, e.g. a conditional + // jump, we call ContributeToJumpTargetEnvironment to "remember" the current + // environment, associated with the jump target offset. When serialization + // eventually reaches that offset, we call IncorporateJumpTargetEnvironment to + // merge that environment back into whatever is the current environment then. + // Note: Since there may be multiple jumps to the same target, + // ContributeToJumpTargetEnvironment may actually do a merge as well. + void ContributeToJumpTargetEnvironment(int target_offset); + void IncorporateJumpTargetEnvironment(int target_offset); + JSHeapBroker* broker() const { return broker_; } CompilationDependencies* dependencies() const { return dependencies_; } Zone* zone() const { return zone_; } @@ -349,7 +358,7 @@ class SerializerForBackgroundCompilation { CompilationDependencies* const dependencies_; Zone* const zone_; Environment* const environment_; - ZoneUnorderedMap stashed_environments_; + ZoneUnorderedMap jump_target_environments_; SerializerForBackgroundCompilationFlags const flags_; }; diff --git a/test/cctest/compiler/serializer-tester.cc b/test/cctest/compiler/serializer-tester.cc index 45f4a1fb9c..ee41ae86fe 100644 --- a/test/cctest/compiler/serializer-tester.cc +++ b/test/cctest/compiler/serializer-tester.cc @@ -274,6 +274,20 @@ TEST(SerializeUnconditionalJump) { "f(); return f;"); } +TEST(MergeJumpTargetEnvironment) { + CheckForSerializedInlinee( + "function f() {" + " let g;" + " while (true) {" + " if (g === undefined) {g = ()=>1; break;} else {g = ()=>2; break};" + " };" + " g(); return g;" + "};" + "%EnsureFeedbackVectorForFunction(f);" + "%EnsureFeedbackVectorForFunction(f());" + "f(); return f;"); // Two calls to f to make g() megamorhpic. +} + } // namespace compiler } // namespace internal } // namespace v8