[turbofan] Improve broker tracing wrt. function inlining

Make it clearer when the broker is missing information about
a potential inlinee.

Bug: v8:7790
Change-Id: I73d6066e75049e15a3fd821ac685476812482142
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1825241
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64007}

src/compiler/heap-refs.h

@@ -329,8 +329,6 @@ class V8_EXPORT_PRIVATE JSFunctionRef : public JSObjectRef {
   SharedFunctionInfoRef shared() const;
   FeedbackVectorRef feedback_vector() const;
   int InitialMapInstanceSizeWithMinSlack() const;
-
-  bool IsSerializedForCompilation() const;
 };
 
 class JSRegExpRef : public JSObjectRef {
@@ -727,7 +725,6 @@ class BytecodeArrayRef : public FixedArrayBaseRef {
   Address handler_table_address() const;
   int handler_table_size() const;
 
-  bool IsSerializedForCompilation() const;
   void SerializeForCompilation();
 };
 

src/compiler/js-heap-broker.cc

@@ -1497,10 +1497,6 @@ class BytecodeArrayData : public FixedArrayBaseData {
     return *(Handle<Smi>::cast(constant_pool_[index]->object()));
   }
 
-  bool IsSerializedForCompilation() const {
-    return is_serialized_for_compilation_;
-  }
-
   void SerializeForCompilation(JSHeapBroker* broker) {
     if (is_serialized_for_compilation_) return;
 
@@ -3084,11 +3080,6 @@ Smi BytecodeArrayRef::GetConstantAtIndexAsSmi(int index) const {
   return data()->AsBytecodeArray()->GetConstantAtIndexAsSmi(index);
 }
 
-bool BytecodeArrayRef::IsSerializedForCompilation() const {
-  if (broker()->mode() == JSHeapBroker::kDisabled) return true;
-  return data()->AsBytecodeArray()->IsSerializedForCompilation();
-}
-
 void BytecodeArrayRef::SerializeForCompilation() {
   if (broker()->mode() == JSHeapBroker::kDisabled) return;
   data()->AsBytecodeArray()->SerializeForCompilation(broker());
@@ -3867,18 +3858,6 @@ bool JSFunctionRef::serialized() const {
   return data()->AsJSFunction()->serialized();
 }
 
-bool JSFunctionRef::IsSerializedForCompilation() const {
-  if (broker()->mode() == JSHeapBroker::kDisabled) {
-    return handle(object()->shared(), broker()->isolate())->HasBytecodeArray();
-  }
-
-  // We get a crash if we try to access the shared() getter without
-  // checking for `serialized` first. Also it's possible to have a
-  // JSFunctionRef without a feedback vector.
-  return serialized() && has_feedback_vector() &&
-         shared().IsSerializedForCompilation(feedback_vector());
-}
-
 JSArrayRef SharedFunctionInfoRef::GetTemplateObject(
     ObjectRef description, FeedbackVectorRef vector, FeedbackSlot slot,
     SerializationPolicy policy) {
@@ -3921,6 +3900,7 @@ JSArrayRef SharedFunctionInfoRef::GetTemplateObject(
 void SharedFunctionInfoRef::SetSerializedForCompilation(
     FeedbackVectorRef feedback) {
   CHECK_EQ(broker()->mode(), JSHeapBroker::kSerializing);
+  CHECK(HasBytecodeArray());
   data()->AsSharedFunctionInfo()->SetSerializedForCompilation(broker(),
                                                               feedback);
 }
@@ -3947,7 +3927,7 @@ SharedFunctionInfoRef::function_template_info() const {
 
 bool SharedFunctionInfoRef::IsSerializedForCompilation(
     FeedbackVectorRef feedback) const {
-  if (broker()->mode() == JSHeapBroker::kDisabled) return true;
+  if (broker()->mode() == JSHeapBroker::kDisabled) return HasBytecodeArray();
   return data()->AsSharedFunctionInfo()->IsSerializedForCompilation(feedback);
 }
 

src/compiler/js-inlining-heuristic.cc

@@ -22,9 +22,35 @@ namespace compiler {
   } while (false)
 
 namespace {
-bool IsSmall(BytecodeArrayRef bytecode) {
+bool IsSmall(BytecodeArrayRef const& bytecode) {
   return bytecode.length() <= FLAG_max_inlined_bytecode_size_small;
 }
+
+bool CanConsiderForInlining(JSHeapBroker* broker,
+                            SharedFunctionInfoRef const& shared,
+                            FeedbackVectorRef const& feedback_vector) {
+  if (!shared.IsInlineable()) return false;
+  DCHECK(shared.HasBytecodeArray());
+  if (!shared.IsSerializedForCompilation(feedback_vector)) {
+    TRACE_BROKER_MISSING(
+        broker, "data for " << shared << " (not serialized for compilation)");
+    return false;
+  }
+  return true;
+}
+
+bool CanConsiderForInlining(JSHeapBroker* broker,
+                            JSFunctionRef const& function) {
+  if (!function.has_feedback_vector()) return false;
+  if (!function.serialized()) {
+    TRACE_BROKER_MISSING(
+        broker, "data for " << function << " (cannot consider for inlining)");
+    return false;
+  }
+  return CanConsiderForInlining(broker, function.shared(),
+                                function.feedback_vector());
+}
+
 }  // namespace
 
 JSInliningHeuristic::Candidate JSInliningHeuristic::CollectFunctions(
@@ -38,11 +64,11 @@ JSInliningHeuristic::Candidate JSInliningHeuristic::CollectFunctions(
   if (m.HasValue() && m.Ref(broker()).IsJSFunction()) {
     out.functions[0] = m.Ref(broker()).AsJSFunction();
     JSFunctionRef function = out.functions[0].value();
-    if (function.IsSerializedForCompilation()) {
+    if (CanConsiderForInlining(broker(), function)) {
       out.bytecode[0] = function.shared().GetBytecodeArray();
+      out.num_functions = 1;
+      return out;
     }
-    out.num_functions = 1;
-    return out;
   }
   if (m.IsPhi()) {
     int const value_input_count = m.node()->op()->ValueInputCount();
@@ -59,7 +85,7 @@ JSInliningHeuristic::Candidate JSInliningHeuristic::CollectFunctions(
 
       out.functions[n] = m.Ref(broker()).AsJSFunction();
       JSFunctionRef function = out.functions[n].value();
-      if (function.IsSerializedForCompilation()) {
+      if (CanConsiderForInlining(broker(), function)) {
         out.bytecode[n] = function.shared().GetBytecodeArray();
       }
     }
@@ -67,11 +93,14 @@ JSInliningHeuristic::Candidate JSInliningHeuristic::CollectFunctions(
     return out;
   }
   if (m.IsJSCreateClosure()) {
-    CreateClosureParameters const& p = CreateClosureParametersOf(m.op());
     DCHECK(!out.functions[0].has_value());
-    out.shared_info = SharedFunctionInfoRef(broker(), p.shared_info());
+    CreateClosureParameters const& p = CreateClosureParametersOf(m.op());
-    SharedFunctionInfoRef shared_info = out.shared_info.value();
+    FeedbackCellRef feedback_cell(broker(), p.feedback_cell());
-    if (shared_info.HasBytecodeArray()) {
+    SharedFunctionInfoRef shared_info(broker(), p.shared_info());
+    out.shared_info = shared_info;
+    if (feedback_cell.value().IsFeedbackVector() &&
+        CanConsiderForInlining(broker(), shared_info,
+                               feedback_cell.value().AsFeedbackVector())) {
       out.bytecode[0] = shared_info.GetBytecodeArray();
     }
     out.num_functions = 1;
@@ -135,7 +164,8 @@ Reduction JSInliningHeuristic::Reduce(Node* node) {
     SharedFunctionInfoRef shared = candidate.functions[i].has_value()
                                        ? candidate.functions[i].value().shared()
                                        : candidate.shared_info.value();
-    candidate.can_inline_function[i] = shared.IsInlineable();
+    candidate.can_inline_function[i] = candidate.bytecode[i].has_value();
+    CHECK_IMPLIES(candidate.can_inline_function[i], shared.IsInlineable());
     // Do not allow direct recursion i.e. f() -> f(). We still allow indirect
     // recurion like f() -> g() -> f(). The indirect recursion is helpful in
     // cases where f() is a small dispatch function that calls the appropriate
@@ -151,11 +181,9 @@ Reduction JSInliningHeuristic::Reduce(Node* node) {
             node->id(), node->op()->mnemonic());
       candidate.can_inline_function[i] = false;
     }
-    // A function reaching this point should always have its bytecode
-    // serialized.
-    BytecodeArrayRef bytecode = candidate.bytecode[i].value();
     if (candidate.can_inline_function[i]) {
       can_inline_candidate = true;
+      BytecodeArrayRef bytecode = candidate.bytecode[i].value();
       candidate.total_size += bytecode.length();
       candidate_is_small = candidate_is_small && IsSmall(bytecode);
     }

src/compiler/js-inlining.cc

@@ -321,7 +321,7 @@ base::Optional<SharedFunctionInfoRef> JSInliner::DetermineCallTarget(
 
     // TODO(turbofan): We might consider to eagerly create the feedback vector
     // in such a case (in {DetermineCallContext} below) eventually.
-    FeedbackCellRef cell(FeedbackCellRef(broker(), p.feedback_cell()));
+    FeedbackCellRef cell(broker(), p.feedback_cell());
     if (!cell.value().IsFeedbackVector()) return base::nullopt;
 
     return SharedFunctionInfoRef(broker(), p.shared_info());
@@ -413,11 +413,11 @@ Reduction JSInliner::ReduceJSCall(Node* node) {
   Node* exception_target = nullptr;
   NodeProperties::IsExceptionalCall(node, &exception_target);
 
-  // JSInliningHeuristic has already filtered candidates without a
+  // JSInliningHeuristic has already filtered candidates without a BytecodeArray
-  // BytecodeArray by calling SharedFunctionInfoRef::IsInlineable. For the ones
+  // by calling SharedFunctionInfoRef::IsInlineable. For the ones passing the
-  // passing the IsInlineable check, The broker holds a reference to the
+  // IsInlineable check, the broker holds a reference to the bytecode array,
-  // bytecode array, which prevents it from getting flushed.
+  // which prevents it from getting flushed.  Therefore, the following check
-  // Therefore, the following check should always hold true.
+  // should always hold true.
   CHECK(shared_info->is_compiled());
 
   if (!FLAG_concurrent_inlining && info_->is_source_positions_enabled()) {
@@ -428,17 +428,10 @@ Reduction JSInliner::ReduceJSCall(Node* node) {
   TRACE("Inlining " << *shared_info << " into " << outer_shared_info
                     << ((exception_target != nullptr) ? " (inside try-block)"
                                                       : ""));
-  // Determine the targets feedback vector and its context.
+  // Determine the target's feedback vector and its context.
   Node* context;
   FeedbackVectorRef feedback_vector = DetermineCallContext(node, &context);
-
+  CHECK(shared_info->IsSerializedForCompilation(feedback_vector));
-  if (FLAG_concurrent_inlining &&
-      !shared_info->IsSerializedForCompilation(feedback_vector)) {
-    // TODO(neis): Should this be a broker message?
-    TRACE("Missed opportunity to inline a function ("
-          << *shared_info << " with " << feedback_vector << ")");
-    return NoChange();
-  }
 
   // ----------------------------------------------------------------
   // After this point, we've made a decision to inline this function.

test/cctest/compiler/serializer-tester.cc

@@ -52,17 +52,19 @@ SerializerTester::SerializerTester(const char* source)
 TEST(SerializeEmptyFunction) {
   SerializerTester tester(
       "function f() {}; %EnsureFeedbackVectorForFunction(f); return f;");
-  CHECK(tester.function().IsSerializedForCompilation());
+  JSFunctionRef function = tester.function();
+  CHECK(
+      function.shared().IsSerializedForCompilation(function.feedback_vector()));
 }
 
-// This helper function allows for testing weather an inlinee candidate
+// This helper function allows for testing whether an inlinee candidate
 // was properly serialized. It expects that the top-level function (that is
 // run through the SerializerTester) will return its inlinee candidate.
 void CheckForSerializedInlinee(const char* source, int argc = 0,
                                Handle<Object> argv[] = {}) {
   SerializerTester tester(source);
   JSFunctionRef f = tester.function();
-  CHECK(f.IsSerializedForCompilation());
+  CHECK(f.shared().IsSerializedForCompilation(f.feedback_vector()));
 
   MaybeHandle<Object> g_obj = Execution::Call(
       tester.isolate(), tester.function().object(),