Skip to content

Commit ea42065

Browse files
bmeurerCommit Bot
bmeurer
authored and
Commit Bot
committed
[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}
1 parent c6e6c2c commit ea42065

14 files changed

Lines changed: 128 additions & 146 deletions

File tree

‎src/api/api.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10382,8 +10382,7 @@ void InvokeAccessorGetterCallback(
1038210382
void InvokeFunctionCallback(const v8::FunctionCallbackInfo<v8::Value>& info,
1038310383
v8::FunctionCallback callback) {
1038410384
Isolate* isolate = reinterpret_cast<Isolate*>(info.GetIsolate());
10385-
RuntimeCallTimerScope timer(isolate,
10386-
RuntimeCallCounterId::kInvokeFunctionCallback);
10385+
RuntimeCallTimerScope timer(isolate, RuntimeCallCounterId::kFunctionCallback);
1038710386
Address callback_address = reinterpret_cast<Address>(callback);
1038810387
VMState<EXTERNAL> state(isolate);
1038910388
ExternalCallbackScope call_scope(isolate, callback_address);

‎src/builtins/arm/builtins-arm.cc‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2835,19 +2835,25 @@ void CallApiFunctionAndReturn(MacroAssembler* masm, Register function_address,
28352835

28362836
DCHECK(function_address == r1 || function_address == r2);
28372837

2838-
Label profiler_disabled;
2839-
Label end_profiler_check;
2838+
Label profiler_enabled, end_profiler_check;
28402839
__ Move(r9, ExternalReference::is_profiling_address(isolate));
28412840
__ ldrb(r9, MemOperand(r9, 0));
28422841
__ cmp(r9, Operand(0));
2843-
__ b(eq, &profiler_disabled);
2844-
2845-
// Additional parameter is the address of the actual callback.
2846-
__ Move(r3, thunk_ref);
2847-
__ jmp(&end_profiler_check);
2848-
2849-
__ bind(&profiler_disabled);
2850-
__ Move(r3, function_address);
2842+
__ b(ne, &profiler_enabled);
2843+
__ Move(r9, ExternalReference::address_of_runtime_stats_flag());
2844+
__ ldrb(r9, MemOperand(r9, 0));
2845+
__ cmp(r9, Operand(0));
2846+
__ b(ne, &profiler_enabled);
2847+
{
2848+
// Call the api function directly.
2849+
__ Move(r3, function_address);
2850+
__ b(&end_profiler_check);
2851+
}
2852+
__ bind(&profiler_enabled);
2853+
{
2854+
// Additional parameter is the address of the actual callback.
2855+
__ Move(r3, thunk_ref);
2856+
}
28512857
__ bind(&end_profiler_check);
28522858

28532859
// Allocate HandleScope in callee-save registers.

‎src/builtins/arm64/builtins-arm64.cc‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3400,16 +3400,23 @@ void CallApiFunctionAndReturn(MacroAssembler* masm, Register function_address,
34003400

34013401
DCHECK(function_address.is(x1) || function_address.is(x2));
34023402

3403-
Label profiler_disabled;
3404-
Label end_profiler_check;
3403+
Label profiler_enabled, end_profiler_check;
34053404
__ Mov(x10, ExternalReference::is_profiling_address(isolate));
34063405
__ Ldrb(w10, MemOperand(x10));
3407-
__ Cbz(w10, &profiler_disabled);
3408-
__ Mov(x3, thunk_ref);
3409-
__ B(&end_profiler_check);
3410-
3411-
__ Bind(&profiler_disabled);
3412-
__ Mov(x3, function_address);
3406+
__ Cbnz(w10, &profiler_enabled);
3407+
__ Mov(x10, ExternalReference::address_of_runtime_stats_flag());
3408+
__ Ldrb(w10, MemOperand(x10));
3409+
__ Cbnz(w10, &profiler_enabled);
3410+
{
3411+
// Call the api function directly.
3412+
__ Mov(x3, function_address);
3413+
__ B(&end_profiler_check);
3414+
}
3415+
__ Bind(&profiler_enabled);
3416+
{
3417+
// Additional parameter is the address of the actual callback.
3418+
__ Mov(x3, thunk_ref);
3419+
}
34133420
__ Bind(&end_profiler_check);
34143421

34153422
// Save the callee-save registers we are going to use.

‎src/builtins/ia32/builtins-ia32.cc‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3012,23 +3012,28 @@ void CallApiFunctionAndReturn(MacroAssembler* masm, Register function_address,
30123012
__ mov(esi, __ ExternalReferenceAsOperand(next_address, esi));
30133013
__ mov(edi, __ ExternalReferenceAsOperand(limit_address, edi));
30143014

3015-
Label profiler_disabled;
3016-
Label end_profiler_check;
3015+
Label profiler_enabled, end_profiler_check;
30173016
__ Move(eax, Immediate(ExternalReference::is_profiling_address(isolate)));
30183017
__ cmpb(Operand(eax, 0), Immediate(0));
3019-
__ j(zero, &profiler_disabled);
3018+
__ j(not_zero, &profiler_enabled);
3019+
__ Move(eax, Immediate(ExternalReference::address_of_runtime_stats_flag()));
3020+
__ cmpb(Operand(eax, 0), Immediate(0));
3021+
__ j(not_zero, &profiler_enabled);
3022+
{
3023+
// Call the api function directly.
3024+
__ mov(eax, function_address);
3025+
__ jmp(&end_profiler_check);
3026+
}
3027+
__ bind(&profiler_enabled);
3028+
{
3029+
// Additional parameter is the address of the actual getter function.
3030+
__ mov(thunk_last_arg, function_address);
3031+
__ Move(eax, Immediate(thunk_ref));
3032+
}
3033+
__ bind(&end_profiler_check);
30203034

3021-
// Additional parameter is the address of the actual getter function.
3022-
__ mov(thunk_last_arg, function_address);
30233035
// Call the api function.
3024-
__ Move(eax, Immediate(thunk_ref));
30253036
__ call(eax);
3026-
__ jmp(&end_profiler_check);
3027-
3028-
__ bind(&profiler_disabled);
3029-
// Call the api function.
3030-
__ call(function_address);
3031-
__ bind(&end_profiler_check);
30323037

30333038
Label prologue;
30343039
// Load the value from ReturnValue

‎src/builtins/x64/builtins-x64.cc‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3002,21 +3002,24 @@ void CallApiFunctionAndReturn(MacroAssembler* masm, Register function_address,
30023002
__ movq(prev_limit_reg, Operand(base_reg, kLimitOffset));
30033003
__ addl(Operand(base_reg, kLevelOffset), Immediate(1));
30043004

3005-
Label profiler_disabled;
3006-
Label end_profiler_check;
3005+
Label profiler_enabled, end_profiler_check;
30073006
__ Move(rax, ExternalReference::is_profiling_address(isolate));
30083007
__ cmpb(Operand(rax, 0), Immediate(0));
3009-
__ j(zero, &profiler_disabled);
3010-
3011-
// Third parameter is the address of the actual getter function.
3012-
__ Move(thunk_last_arg, function_address);
3013-
__ Move(rax, thunk_ref);
3014-
__ jmp(&end_profiler_check);
3015-
3016-
__ bind(&profiler_disabled);
3017-
// Call the api function!
3018-
__ Move(rax, function_address);
3019-
3008+
__ j(not_zero, &profiler_enabled);
3009+
__ Move(rax, ExternalReference::address_of_runtime_stats_flag());
3010+
__ cmpb(Operand(rax, 0), Immediate(0));
3011+
__ j(not_zero, &profiler_enabled);
3012+
{
3013+
// Call the api function directly.
3014+
__ Move(rax, function_address);
3015+
__ jmp(&end_profiler_check);
3016+
}
3017+
__ bind(&profiler_enabled);
3018+
{
3019+
// Third parameter is the address of the actual getter function.
3020+
__ Move(thunk_last_arg, function_address);
3021+
__ Move(rax, thunk_ref);
3022+
}
30203023
__ bind(&end_profiler_check);
30213024

30223025
// Call the api function!

‎src/codegen/code-stub-assembler.cc‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13469,14 +13469,6 @@ Node* CodeStubAssembler::IsDebugActive() {
1346913469
return Word32NotEqual(is_debug_active, Int32Constant(0));
1347013470
}
1347113471

13472-
TNode<BoolT> CodeStubAssembler::IsRuntimeCallStatsEnabled() {
13473-
STATIC_ASSERT(sizeof(TracingFlags::runtime_stats) == kInt32Size);
13474-
TNode<Word32T> flag_value = UncheckedCast<Word32T>(Load(
13475-
MachineType::Int32(),
13476-
ExternalConstant(ExternalReference::address_of_runtime_stats_flag())));
13477-
return Word32NotEqual(flag_value, Int32Constant(0));
13478-
}
13479-
1348013472
Node* CodeStubAssembler::IsPromiseHookEnabled() {
1348113473
Node* const promise_hook = Load(
1348213474
MachineType::Pointer(),

‎src/codegen/code-stub-assembler.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3254,8 +3254,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
32543254
// Debug helpers
32553255
Node* IsDebugActive();
32563256

3257-
TNode<BoolT> IsRuntimeCallStatsEnabled();
3258-
32593257
// JSArrayBuffer helpers
32603258
TNode<Uint32T> LoadJSArrayBufferBitField(TNode<JSArrayBuffer> array_buffer);
32613259
TNode<RawPtrT> LoadJSArrayBufferBackingStore(

‎src/compiler/access-info.cc‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -445,9 +445,6 @@ PropertyAccessInfo AccessInfoFactory::ComputeAccessorDescriptorAccessInfo(
445445
DCHECK_IMPLIES(lookup == CallOptimization::kHolderIsReceiver,
446446
holder.is_null());
447447
DCHECK_IMPLIES(lookup == CallOptimization::kHolderFound, !holder.is_null());
448-
if (V8_UNLIKELY(TracingFlags::is_runtime_stats_enabled())) {
449-
return PropertyAccessInfo::Invalid(zone());
450-
}
451448
}
452449
if (access_mode == AccessMode::kLoad) {
453450
Handle<Name> cached_property_name;

‎src/compiler/js-call-reducer.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3646,8 +3646,7 @@ Reduction JSCallReducer::ReduceJSCall(Node* node,
36463646
break;
36473647
}
36483648

3649-
if (!TracingFlags::is_runtime_stats_enabled() &&
3650-
shared.object()->IsApiFunction()) {
3649+
if (shared.object()->IsApiFunction()) {
36513650
return ReduceCallApiFunction(node, shared);
36523651
}
36533652
return NoChange();

‎src/ic/accessor-assembler.cc‎

Lines changed: 21 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -184,26 +184,19 @@ void AccessorAssembler::HandleLoadCallbackProperty(const LoadICParameters* p,
184184
TNode<IntPtrT> descriptor =
185185
Signed(DecodeWord<LoadHandler::DescriptorBits>(handler_word));
186186

187-
Label runtime(this, Label::kDeferred);
188187
Callable callable = CodeFactory::ApiGetter(isolate());
189188
TNode<AccessorInfo> accessor_info =
190189
CAST(LoadDescriptorValue(LoadMap(holder), descriptor));
191190

192-
GotoIf(IsRuntimeCallStatsEnabled(), &runtime);
193191
exit_point->ReturnCallStub(callable, p->context, p->receiver, holder,
194192
accessor_info);
195-
196-
BIND(&runtime);
197-
exit_point->ReturnCallRuntime(Runtime::kLoadCallbackProperty, p->context,
198-
p->receiver, holder, accessor_info, p->name);
199193
}
200194

201195
void AccessorAssembler::HandleLoadAccessor(
202196
const LoadICParameters* p, TNode<CallHandlerInfo> call_handler_info,
203197
TNode<WordT> handler_word, TNode<DataHandler> handler,
204198
TNode<IntPtrT> handler_kind, ExitPoint* exit_point) {
205199
Comment("api_getter");
206-
Label runtime(this, Label::kDeferred);
207200
// Context is stored either in data2 or data3 field depending on whether
208201
// the access check is enabled for this handler or not.
209202
TNode<MaybeObject> maybe_context = Select<MaybeObject>(
@@ -215,39 +208,31 @@ void AccessorAssembler::HandleLoadAccessor(
215208
CSA_CHECK(this, IsNotCleared(maybe_context));
216209
TNode<Object> context = GetHeapObjectAssumeWeak(maybe_context);
217210

218-
GotoIf(IsRuntimeCallStatsEnabled(), &runtime);
219-
{
220-
TNode<Foreign> foreign = CAST(
221-
LoadObjectField(call_handler_info, CallHandlerInfo::kJsCallbackOffset));
222-
TNode<WordT> callback = TNode<WordT>::UncheckedCast(LoadObjectField(
223-
foreign, Foreign::kForeignAddressOffset, MachineType::Pointer()));
224-
TNode<Object> data =
225-
LoadObjectField(call_handler_info, CallHandlerInfo::kDataOffset);
226-
227-
VARIABLE(api_holder, MachineRepresentation::kTagged, p->receiver);
228-
Label load(this);
229-
GotoIf(WordEqual(handler_kind, IntPtrConstant(LoadHandler::kApiGetter)),
230-
&load);
211+
TNode<Foreign> foreign = CAST(
212+
LoadObjectField(call_handler_info, CallHandlerInfo::kJsCallbackOffset));
213+
TNode<WordT> callback = TNode<WordT>::UncheckedCast(LoadObjectField(
214+
foreign, Foreign::kForeignAddressOffset, MachineType::Pointer()));
215+
TNode<Object> data =
216+
LoadObjectField(call_handler_info, CallHandlerInfo::kDataOffset);
231217

232-
CSA_ASSERT(
233-
this,
234-
WordEqual(handler_kind,
235-
IntPtrConstant(LoadHandler::kApiGetterHolderIsPrototype)));
218+
VARIABLE(api_holder, MachineRepresentation::kTagged, p->receiver);
219+
Label load(this);
220+
GotoIf(WordEqual(handler_kind, IntPtrConstant(LoadHandler::kApiGetter)),
221+
&load);
236222

237-
api_holder.Bind(LoadMapPrototype(LoadMap(p->receiver)));
238-
Goto(&load);
223+
CSA_ASSERT(
224+
this,
225+
WordEqual(handler_kind,
226+
IntPtrConstant(LoadHandler::kApiGetterHolderIsPrototype)));
239227

240-
BIND(&load);
241-
Callable callable = CodeFactory::CallApiCallback(isolate());
242-
TNode<IntPtrT> argc = IntPtrConstant(0);
243-
exit_point->Return(CallStub(callable, context, callback, argc, data,
244-
api_holder.value(), p->receiver));
245-
}
228+
api_holder.Bind(LoadMapPrototype(LoadMap(p->receiver)));
229+
Goto(&load);
246230

247-
BIND(&runtime);
248-
exit_point->ReturnCallRuntime(Runtime::kLoadAccessorProperty, context,
249-
p->receiver, SmiTag(handler_kind),
250-
call_handler_info);
231+
BIND(&load);
232+
Callable callable = CodeFactory::CallApiCallback(isolate());
233+
TNode<IntPtrT> argc = IntPtrConstant(0);
234+
exit_point->Return(CallStub(callable, context, callback, argc, data,
235+
api_holder.value(), p->receiver));
251236
}
252237

253238
void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word,

0 commit comments

Comments
 (0)