[counters] Introduce proper bottleneck for FunctionCallback.

API calls made via the CallApiCallback builtin, which is used from the
ICs and optimized code, are currently misattributed to the wrong counter
InvokeFunctionCallback instead of FunctionCallback. In addition we don't
use the C trampoline when only runtime call stats are enabled, but the
Chrome DevTools profiler is not active, which means that these calls
will not be attrituted properly at all, and that had to be worked around
using all kinds of tricks (i.e. disabling fast-paths in ICs when RCS is
active and not inlining calls/property accesses into optimized code
depending on the state of RCS).

All of this was really brittle and only due to the fact that the central
builtin didn't properly check for RCS (in addition to checking for the
CDT profiler). With this fix it's now handled in a central place and
attributed to the correct category, so user code doesn't need to worry
about RCS anymore and can just call straight into the fast-path.

Drive-by-fix: Do the same for AccessorInfo getter calls, which share the
core hand-written native code with the API callback logic.

Bug: v8:9183
Change-Id: Id0cd99d3dd676635fe3272b67cd76a19a9a9cea4
Cq-Include-Trybots: luci.chromium.try:linux-rel,win7-rel
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1651470
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62109}

src/api/api.cc

@@ -10382,8 +10382,7 @@ void InvokeAccessorGetterCallback(
 void InvokeFunctionCallback(const v8::FunctionCallbackInfo<v8::Value>& info,
                             v8::FunctionCallback callback) {
   Isolate* isolate = reinterpret_cast<Isolate*>(info.GetIsolate());
-  RuntimeCallTimerScope timer(isolate,
-                              RuntimeCallCounterId::kInvokeFunctionCallback);
+  RuntimeCallTimerScope timer(isolate, RuntimeCallCounterId::kFunctionCallback);
   Address callback_address = reinterpret_cast<Address>(callback);
   VMState<EXTERNAL> state(isolate);
   ExternalCallbackScope call_scope(isolate, callback_address);

src/builtins/arm/builtins-arm.cc

@@ -2835,19 +2835,25 @@ void CallApiFunctionAndReturn(MacroAssembler* masm, Register function_address,
 
   DCHECK(function_address == r1 || function_address == r2);
 
-  Label profiler_disabled;
-  Label end_profiler_check;
+  Label profiler_enabled, end_profiler_check;
   __ Move(r9, ExternalReference::is_profiling_address(isolate));
   __ ldrb(r9, MemOperand(r9, 0));
   __ cmp(r9, Operand(0));
-  __ b(eq, &profiler_disabled);
-
-  // Additional parameter is the address of the actual callback.
-  __ Move(r3, thunk_ref);
-  __ jmp(&end_profiler_check);
-
-  __ bind(&profiler_disabled);
-  __ Move(r3, function_address);
+  __ b(ne, &profiler_enabled);
+  __ Move(r9, ExternalReference::address_of_runtime_stats_flag());
+  __ ldrb(r9, MemOperand(r9, 0));
+  __ cmp(r9, Operand(0));
+  __ b(ne, &profiler_enabled);
+  {
+    // Call the api function directly.
+    __ Move(r3, function_address);
+    __ b(&end_profiler_check);
+  }
+  __ bind(&profiler_enabled);
+  {
+    // Additional parameter is the address of the actual callback.
+    __ Move(r3, thunk_ref);
+  }
   __ bind(&end_profiler_check);
 
   // Allocate HandleScope in callee-save registers.

src/builtins/arm64/builtins-arm64.cc

@@ -3400,16 +3400,23 @@ void CallApiFunctionAndReturn(MacroAssembler* masm, Register function_address,
 
   DCHECK(function_address.is(x1) || function_address.is(x2));
 
-  Label profiler_disabled;
-  Label end_profiler_check;
+  Label profiler_enabled, end_profiler_check;
   __ Mov(x10, ExternalReference::is_profiling_address(isolate));
   __ Ldrb(w10, MemOperand(x10));
-  __ Cbz(w10, &profiler_disabled);
-  __ Mov(x3, thunk_ref);
-  __ B(&end_profiler_check);
-
-  __ Bind(&profiler_disabled);
-  __ Mov(x3, function_address);
+  __ Cbnz(w10, &profiler_enabled);
+  __ Mov(x10, ExternalReference::address_of_runtime_stats_flag());
+  __ Ldrb(w10, MemOperand(x10));
+  __ Cbnz(w10, &profiler_enabled);
+  {
+    // Call the api function directly.
+    __ Mov(x3, function_address);
+    __ B(&end_profiler_check);
+  }
+  __ Bind(&profiler_enabled);
+  {
+    // Additional parameter is the address of the actual callback.
+    __ Mov(x3, thunk_ref);
+  }
   __ Bind(&end_profiler_check);
 
   // Save the callee-save registers we are going to use.

src/builtins/ia32/builtins-ia32.cc

@@ -3012,24 +3012,29 @@ void CallApiFunctionAndReturn(MacroAssembler* masm, Register function_address,
   __ mov(esi, __ ExternalReferenceAsOperand(next_address, esi));
   __ mov(edi, __ ExternalReferenceAsOperand(limit_address, edi));
 
-  Label profiler_disabled;
-  Label end_profiler_check;
+  Label profiler_enabled, end_profiler_check;
   __ Move(eax, Immediate(ExternalReference::is_profiling_address(isolate)));
   __ cmpb(Operand(eax, 0), Immediate(0));
-  __ j(zero, &profiler_disabled);
-
-  // Additional parameter is the address of the actual getter function.
-  __ mov(thunk_last_arg, function_address);
-  // Call the api function.
-  __ Move(eax, Immediate(thunk_ref));
-  __ call(eax);
-  __ jmp(&end_profiler_check);
-
-  __ bind(&profiler_disabled);
-  // Call the api function.
-  __ call(function_address);
+  __ j(not_zero, &profiler_enabled);
+  __ Move(eax, Immediate(ExternalReference::address_of_runtime_stats_flag()));
+  __ cmpb(Operand(eax, 0), Immediate(0));
+  __ j(not_zero, &profiler_enabled);
+  {
+    // Call the api function directly.
+    __ mov(eax, function_address);
+    __ jmp(&end_profiler_check);
+  }
+  __ bind(&profiler_enabled);
+  {
+    // Additional parameter is the address of the actual getter function.
+    __ mov(thunk_last_arg, function_address);
+    __ Move(eax, Immediate(thunk_ref));
+  }
   __ bind(&end_profiler_check);
 
+  // Call the api function.
+  __ call(eax);
+
   Label prologue;
   // Load the value from ReturnValue
   __ mov(eax, return_value_operand);

src/builtins/x64/builtins-x64.cc

@@ -3002,21 +3002,24 @@ void CallApiFunctionAndReturn(MacroAssembler* masm, Register function_address,
   __ movq(prev_limit_reg, Operand(base_reg, kLimitOffset));
   __ addl(Operand(base_reg, kLevelOffset), Immediate(1));
 
-  Label profiler_disabled;
-  Label end_profiler_check;
+  Label profiler_enabled, end_profiler_check;
   __ Move(rax, ExternalReference::is_profiling_address(isolate));
   __ cmpb(Operand(rax, 0), Immediate(0));
-  __ j(zero, &profiler_disabled);
-
-  // Third parameter is the address of the actual getter function.
-  __ Move(thunk_last_arg, function_address);
-  __ Move(rax, thunk_ref);
-  __ jmp(&end_profiler_check);
-
-  __ bind(&profiler_disabled);
-  // Call the api function!
-  __ Move(rax, function_address);
-
+  __ j(not_zero, &profiler_enabled);
+  __ Move(rax, ExternalReference::address_of_runtime_stats_flag());
+  __ cmpb(Operand(rax, 0), Immediate(0));
+  __ j(not_zero, &profiler_enabled);
+  {
+    // Call the api function directly.
+    __ Move(rax, function_address);
+    __ jmp(&end_profiler_check);
+  }
+  __ bind(&profiler_enabled);
+  {
+    // Third parameter is the address of the actual getter function.
+    __ Move(thunk_last_arg, function_address);
+    __ Move(rax, thunk_ref);
+  }
   __ bind(&end_profiler_check);
 
   // Call the api function!

src/codegen/code-stub-assembler.cc

@@ -13469,14 +13469,6 @@ Node* CodeStubAssembler::IsDebugActive() {
   return Word32NotEqual(is_debug_active, Int32Constant(0));
 }
 
-TNode<BoolT> CodeStubAssembler::IsRuntimeCallStatsEnabled() {
-  STATIC_ASSERT(sizeof(TracingFlags::runtime_stats) == kInt32Size);
-  TNode<Word32T> flag_value = UncheckedCast<Word32T>(Load(
-      MachineType::Int32(),
-      ExternalConstant(ExternalReference::address_of_runtime_stats_flag())));
-  return Word32NotEqual(flag_value, Int32Constant(0));
-}
-
 Node* CodeStubAssembler::IsPromiseHookEnabled() {
   Node* const promise_hook = Load(
       MachineType::Pointer(),

src/codegen/code-stub-assembler.h

@@ -3254,8 +3254,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   // Debug helpers
   Node* IsDebugActive();
 
-  TNode<BoolT> IsRuntimeCallStatsEnabled();
-
   // JSArrayBuffer helpers
   TNode<Uint32T> LoadJSArrayBufferBitField(TNode<JSArrayBuffer> array_buffer);
   TNode<RawPtrT> LoadJSArrayBufferBackingStore(

src/compiler/access-info.cc

@@ -445,9 +445,6 @@ PropertyAccessInfo AccessInfoFactory::ComputeAccessorDescriptorAccessInfo(
     DCHECK_IMPLIES(lookup == CallOptimization::kHolderIsReceiver,
                    holder.is_null());
     DCHECK_IMPLIES(lookup == CallOptimization::kHolderFound, !holder.is_null());
-    if (V8_UNLIKELY(TracingFlags::is_runtime_stats_enabled())) {
-      return PropertyAccessInfo::Invalid(zone());
-    }
   }
   if (access_mode == AccessMode::kLoad) {
     Handle<Name> cached_property_name;

src/compiler/js-call-reducer.cc

@@ -3646,8 +3646,7 @@ Reduction JSCallReducer::ReduceJSCall(Node* node,
       break;
   }
 
-  if (!TracingFlags::is_runtime_stats_enabled() &&
-      shared.object()->IsApiFunction()) {
+  if (shared.object()->IsApiFunction()) {
     return ReduceCallApiFunction(node, shared);
   }
   return NoChange();

src/ic/accessor-assembler.cc

@@ -184,18 +184,12 @@ void AccessorAssembler::HandleLoadCallbackProperty(const LoadICParameters* p,
   TNode<IntPtrT> descriptor =
       Signed(DecodeWord<LoadHandler::DescriptorBits>(handler_word));
 
-  Label runtime(this, Label::kDeferred);
   Callable callable = CodeFactory::ApiGetter(isolate());
   TNode<AccessorInfo> accessor_info =
       CAST(LoadDescriptorValue(LoadMap(holder), descriptor));
 
-  GotoIf(IsRuntimeCallStatsEnabled(), &runtime);
   exit_point->ReturnCallStub(callable, p->context, p->receiver, holder,
                              accessor_info);
-
-  BIND(&runtime);
-  exit_point->ReturnCallRuntime(Runtime::kLoadCallbackProperty, p->context,
-                                p->receiver, holder, accessor_info, p->name);
 }
 
 void AccessorAssembler::HandleLoadAccessor(
@@ -203,7 +197,6 @@ void AccessorAssembler::HandleLoadAccessor(
     TNode<WordT> handler_word, TNode<DataHandler> handler,
     TNode<IntPtrT> handler_kind, ExitPoint* exit_point) {
   Comment("api_getter");
-  Label runtime(this, Label::kDeferred);
   // Context is stored either in data2 or data3 field depending on whether
   // the access check is enabled for this handler or not.
   TNode<MaybeObject> maybe_context = Select<MaybeObject>(
@@ -215,39 +208,31 @@ void AccessorAssembler::HandleLoadAccessor(
   CSA_CHECK(this, IsNotCleared(maybe_context));
   TNode<Object> context = GetHeapObjectAssumeWeak(maybe_context);
 
-  GotoIf(IsRuntimeCallStatsEnabled(), &runtime);
-  {
-    TNode<Foreign> foreign = CAST(
-        LoadObjectField(call_handler_info, CallHandlerInfo::kJsCallbackOffset));
-    TNode<WordT> callback = TNode<WordT>::UncheckedCast(LoadObjectField(
-        foreign, Foreign::kForeignAddressOffset, MachineType::Pointer()));
-    TNode<Object> data =
-        LoadObjectField(call_handler_info, CallHandlerInfo::kDataOffset);
+  TNode<Foreign> foreign = CAST(
+      LoadObjectField(call_handler_info, CallHandlerInfo::kJsCallbackOffset));
+  TNode<WordT> callback = TNode<WordT>::UncheckedCast(LoadObjectField(
+      foreign, Foreign::kForeignAddressOffset, MachineType::Pointer()));
+  TNode<Object> data =
+      LoadObjectField(call_handler_info, CallHandlerInfo::kDataOffset);
 
-    VARIABLE(api_holder, MachineRepresentation::kTagged, p->receiver);
-    Label load(this);
-    GotoIf(WordEqual(handler_kind, IntPtrConstant(LoadHandler::kApiGetter)),
-           &load);
+  VARIABLE(api_holder, MachineRepresentation::kTagged, p->receiver);
+  Label load(this);
+  GotoIf(WordEqual(handler_kind, IntPtrConstant(LoadHandler::kApiGetter)),
+         &load);
 
-    CSA_ASSERT(
-        this,
-        WordEqual(handler_kind,
-                  IntPtrConstant(LoadHandler::kApiGetterHolderIsPrototype)));
+  CSA_ASSERT(
+      this,
+      WordEqual(handler_kind,
+                IntPtrConstant(LoadHandler::kApiGetterHolderIsPrototype)));
 
-    api_holder.Bind(LoadMapPrototype(LoadMap(p->receiver)));
-    Goto(&load);
+  api_holder.Bind(LoadMapPrototype(LoadMap(p->receiver)));
+  Goto(&load);
 
-    BIND(&load);
-    Callable callable = CodeFactory::CallApiCallback(isolate());
-    TNode<IntPtrT> argc = IntPtrConstant(0);
-    exit_point->Return(CallStub(callable, context, callback, argc, data,
-                                api_holder.value(), p->receiver));
-  }
-
-  BIND(&runtime);
-  exit_point->ReturnCallRuntime(Runtime::kLoadAccessorProperty, context,
-                                p->receiver, SmiTag(handler_kind),
-                                call_handler_info);
+  BIND(&load);
+  Callable callable = CodeFactory::CallApiCallback(isolate());
+  TNode<IntPtrT> argc = IntPtrConstant(0);
+  exit_point->Return(CallStub(callable, context, callback, argc, data,
+                              api_holder.value(), p->receiver));
 }
 
 void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word,

src/ic/ic.cc

@@ -2650,46 +2650,6 @@ RUNTIME_FUNCTION(Runtime_StoreCallbackProperty) {
   return *value;
 }
 
-RUNTIME_FUNCTION(Runtime_LoadCallbackProperty) {
-  Handle<JSObject> receiver = args.at<JSObject>(0);
-  Handle<JSObject> holder = args.at<JSObject>(1);
-  Handle<AccessorInfo> info = args.at<AccessorInfo>(2);
-  Handle<Name> name = args.at<Name>(3);
-  HandleScope scope(isolate);
-
-  DCHECK(info->IsCompatibleReceiver(*receiver));
-
-  PropertyCallbackArguments custom_args(isolate, info->data(), *receiver,
-                                        *holder, Just(kThrowOnError));
-  Handle<Object> result = custom_args.CallAccessorGetter(info, name);
-  RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate);
-  if (result.is_null()) return ReadOnlyRoots(isolate).undefined_value();
-  return *result;
-}
-
-RUNTIME_FUNCTION(Runtime_LoadAccessorProperty) {
-  HandleScope scope(isolate);
-  DCHECK_EQ(args.length(), 3);
-  Handle<JSObject> receiver = args.at<JSObject>(0);
-  int handler_kind = args.smi_at(1);
-  Handle<CallHandlerInfo> call_handler_info = args.at<CallHandlerInfo>(2);
-
-  Object holder = *receiver;
-  if (handler_kind == LoadHandler::kApiGetterHolderIsPrototype) {
-    holder = receiver->map().prototype();
-  } else {
-    DCHECK_EQ(handler_kind, LoadHandler::kApiGetter);
-  }
-
-  // Call the accessor without additional arguments.
-  FunctionCallbackArguments custom(isolate, call_handler_info->data(),
-                                   *receiver, holder, HeapObject(), nullptr, 0);
-  Handle<Object> result_handle = custom.Call(*call_handler_info);
-  RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate);
-  if (result_handle.is_null()) return ReadOnlyRoots(isolate).undefined_value();
-  return *result_handle;
-}
-
 /**
  * Loads a property with an interceptor performing post interceptor
  * lookup if interceptor failed.

src/logging/counters.h

@@ -941,7 +941,6 @@ class RuntimeCallTimer final {
   V(Invoke)                                    \
   V(InvokeApiFunction)                         \
   V(InvokeApiInterruptCallbacks)               \
-  V(InvokeFunctionCallback)                    \
   V(JS_Execution)                              \
   V(Map_SetPrototype)                          \
   V(Map_TransitionToAccessorProperty)          \

src/runtime/runtime.h

@@ -557,8 +557,6 @@ namespace internal {
   F(KeyedStoreIC_Miss, 5, 1)                 \
   F(StoreInArrayLiteralIC_Miss, 5, 1)        \
   F(KeyedStoreIC_Slow, 3, 1)                 \
-  F(LoadAccessorProperty, 4, 1)              \
-  F(LoadCallbackProperty, 4, 1)              \
   F(LoadElementWithInterceptor, 2, 1)        \
   F(LoadGlobalIC_Miss, 4, 1)                 \
   F(LoadGlobalIC_Slow, 3, 1)                 \

test/unittests/logging/counters-unittest.cc

@@ -88,9 +88,6 @@ class RuntimeCallStatsTest : public TestWithNativeContext {
     return isolate()->counters()->runtime_call_stats();
   }
 
-  // Print current RuntimeCallStats table. For debugging purposes.
-  void PrintStats() { stats()->Print(); }
-
   RuntimeCallCounterId counter_id() {
     return RuntimeCallCounterId::kTestCounter1;
   }
@@ -655,6 +652,8 @@ static void CustomCallback(const v8::FunctionCallbackInfo<v8::Value>& info) {
 }  // namespace
 
 TEST_F(RuntimeCallStatsTest, CallbackFunction) {
+  FLAG_allow_natives_syntax = true;
+
   RuntimeCallCounter* callback_counter =
       stats()->GetCounter(RuntimeCallCounterId::kFunctionCallback);
 
@@ -710,9 +709,29 @@ TEST_F(RuntimeCallStatsTest, CallbackFunction) {
   EXPECT_EQ(0, callback_counter->time().InMicroseconds());
   EXPECT_EQ(100, counter()->time().InMicroseconds());
   EXPECT_EQ(kCustomCallbackTime * 4010, counter2()->time().InMicroseconds());
+
+  // Check that the FunctionCallback tracing also works properly
+  // when the `callback` is called from optimized code.
+  RunJS(
+      "function wrap(o) { return o.callback(); };\n"
+      "%PrepareFunctionForOptimization(wrap);\n"
+      "wrap(custom_object);\n"
+      "wrap(custom_object);\n"
+      "%OptimizeFunctionOnNextCall(wrap);\n"
+      "wrap(custom_object);\n");
+  EXPECT_EQ(4, js_counter()->count());
+  EXPECT_EQ(1, counter()->count());
+  EXPECT_EQ(4013, callback_counter->count());
+  EXPECT_EQ(4013, counter2()->count());
+  EXPECT_EQ(0, js_counter()->time().InMicroseconds());
+  EXPECT_EQ(0, callback_counter->time().InMicroseconds());
+  EXPECT_EQ(100, counter()->time().InMicroseconds());
+  EXPECT_EQ(kCustomCallbackTime * 4013, counter2()->time().InMicroseconds());
 }
 
 TEST_F(RuntimeCallStatsTest, ApiGetter) {
+  FLAG_allow_natives_syntax = true;
+
   RuntimeCallCounter* callback_counter =
       stats()->GetCounter(RuntimeCallCounterId::kFunctionCallback);
   current_test = this;
@@ -740,7 +759,6 @@ TEST_F(RuntimeCallStatsTest, ApiGetter) {
     Sleep(100);
     RunJS("custom_object.apiGetter;");
   }
-  PrintStats();
 
   EXPECT_EQ(1, js_counter()->count());
   EXPECT_EQ(1, counter()->count());
@@ -754,7 +772,6 @@ TEST_F(RuntimeCallStatsTest, ApiGetter) {
   EXPECT_EQ(kCustomCallbackTime, counter2()->time().InMicroseconds());
 
   RunJS("for (let i = 0; i < 9; i++) { custom_object.apiGetter };");
-  PrintStats();
 
   EXPECT_EQ(2, js_counter()->count());
   EXPECT_EQ(1, counter()->count());
@@ -767,7 +784,6 @@ TEST_F(RuntimeCallStatsTest, ApiGetter) {
   EXPECT_EQ(kCustomCallbackTime * 10, counter2()->time().InMicroseconds());
 
   RunJS("for (let i = 0; i < 4000; i++) { custom_object.apiGetter };");
-  PrintStats();
 
   EXPECT_EQ(3, js_counter()->count());
   EXPECT_EQ(1, counter()->count());
@@ -779,7 +795,25 @@ TEST_F(RuntimeCallStatsTest, ApiGetter) {
   EXPECT_EQ(0, callback_counter->time().InMicroseconds());
   EXPECT_EQ(kCustomCallbackTime * 4010, counter2()->time().InMicroseconds());
 
-  PrintStats();
+  // Check that the FunctionCallback tracing also works properly
+  // when the `apiGetter` is called from optimized code.
+  RunJS(
+      "function wrap(o) { return o.apiGetter; };\n"
+      "%PrepareFunctionForOptimization(wrap);\n"
+      "wrap(custom_object);\n"
+      "wrap(custom_object);\n"
+      "%OptimizeFunctionOnNextCall(wrap);\n"
+      "wrap(custom_object);\n");
+
+  EXPECT_EQ(4, js_counter()->count());
+  EXPECT_EQ(1, counter()->count());
+  EXPECT_EQ(4013, callback_counter->count());
+  EXPECT_EQ(4013, counter2()->count());
+
+  EXPECT_EQ(0, js_counter()->time().InMicroseconds());
+  EXPECT_EQ(100, counter()->time().InMicroseconds());
+  EXPECT_EQ(0, callback_counter->time().InMicroseconds());
+  EXPECT_EQ(kCustomCallbackTime * 4013, counter2()->time().InMicroseconds());
 }
 
 TEST_F(SnapshotNativeCounterTest, StringAddNative) {