Skip to content

Commit b022146

Browse files
sygV8 LUCI CQ
syg
authored and
V8 LUCI CQ
committed
Reland^2 "[builtins] Optimize and generate builtins off-main-thread"
This is a reland of commit 9125716 Changes since revert: - Drain the output queue of the dispatcher when input queue is unavailable or output queue is using too much memory - Fix data race in AddBuiltin - Disable when profiling Original change's description: > Reland "[builtins] Optimize and generate builtins off-main-thread" > > This is a reland of commit b132bd4 > > Changes since revert: > - Don't request InstallCode interrupt during concurrent builtin > generation > > Original change's description: > > [builtins] Optimize and generate builtins off-main-thread > > > > Bug: 342692713 > > Change-Id: I7c397ba93bc3b717a00ff4cfafa6919ad571cd43 > > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5594234 > > Reviewed-by: Nico Hartmann <[email protected]> > > Commit-Queue: Shu-yu Guo <[email protected]> > > Cr-Commit-Position: refs/heads/main@{#94750} > > Bug: 342692713 > Change-Id: I64c7e482901763aadacc5c6c4a35b99d1c03481f > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5670430 > Reviewed-by: Nico Hartmann <[email protected]> > Commit-Queue: Shu-yu Guo <[email protected]> > Cr-Commit-Position: refs/heads/main@{#94784} Bug: 342692713 Change-Id: I99cbcfb4576b3a5a256eb36c3ac926cfd02cb6fc Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5671629 Reviewed-by: Nico Hartmann <[email protected]> Commit-Queue: Shu-yu Guo <[email protected]> Cr-Commit-Position: refs/heads/main@{#95024}
1 parent b369d13 commit b022146

18 files changed

Lines changed: 765 additions & 166 deletions

‎src/DEPS‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,9 @@ specific_include_rules = {
121121
"+src/heap/factory-base.h",
122122
"+src/heap/local-factory.h",
123123
],
124+
"setup-builtins-internal\.cc": [
125+
"+src/compiler/pipeline.h",
126+
],
124127
"snapshot\.cc": [
125128
"+src/heap/read-only-promotion.h",
126129
],

‎src/builtins/constants-table-builder.cc‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,6 @@ uint32_t BuiltinsConstantsTableBuilder::AddObject(Handle<Object> object) {
3636
DCHECK_EQ(ReadOnlyRoots(isolate_).empty_fixed_array(),
3737
isolate_->heap()->builtins_constants_table());
3838

39-
// Must be on the main thread.
40-
DCHECK_EQ(ThreadId::Current(), isolate_->thread_id());
41-
4239
// Must be generating embedded builtin code.
4340
DCHECK(isolate_->IsGeneratingEmbeddedBuiltins());
4441

@@ -47,12 +44,24 @@ uint32_t BuiltinsConstantsTableBuilder::AddObject(Handle<Object> object) {
4744
DCHECK(!IsInstructionStream(*object));
4845
#endif
4946

50-
auto find_result = map_.FindOrInsert(object);
51-
if (!find_result.already_exists) {
52-
DCHECK(IsHeapObject(*object));
53-
*find_result.entry = map_.size() - 1;
47+
// This method is called concurrently from both the main thread and
48+
// compilation threads. Constant indices need to be reproducible during
49+
// builtin generation, so the main thread pre-adds all the constants. A lock
50+
// is still needed since the map data structure is still being concurrently
51+
// accessed.
52+
base::MutexGuard guard(&mutex_);
53+
if (ThreadId::Current() != isolate_->thread_id()) {
54+
auto find_result = map_.Find(object);
55+
DCHECK_NOT_NULL(find_result);
56+
return *find_result;
57+
} else {
58+
auto find_result = map_.FindOrInsert(object);
59+
if (!find_result.already_exists) {
60+
DCHECK(IsHeapObject(*object));
61+
*find_result.entry = map_.size() - 1;
62+
}
63+
return *find_result.entry;
5464
}
55-
return *find_result.entry;
5665
}
5766

5867
namespace {

‎src/builtins/constants-table-builder.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,10 @@ class BuiltinsConstantsTableBuilder final {
5454
// Maps objects to corresponding indices within the constants list.
5555
using ConstantsMap = IdentityMap<uint32_t, FreeStoreAllocationPolicy>;
5656
ConstantsMap map_;
57+
58+
// Protects accesses to map_, which is concurrently accessed when generating
59+
// builtins off-main-thread.
60+
base::Mutex mutex_;
5761
};
5862

5963
} // namespace internal

‎src/builtins/setup-builtins-internal.cc‎

Lines changed: 140 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,13 @@
77
#include "src/builtins/builtins-inl.h"
88
#include "src/builtins/profile-data-reader.h"
99
#include "src/codegen/assembler-inl.h"
10+
#include "src/codegen/compiler.h"
1011
#include "src/codegen/interface-descriptors.h"
1112
#include "src/codegen/macro-assembler-inl.h"
1213
#include "src/codegen/macro-assembler.h"
1314
#include "src/codegen/reloc-info-inl.h"
1415
#include "src/common/globals.h"
16+
#include "src/compiler-dispatcher/optimizing-compile-dispatcher.h"
1517
#include "src/compiler/code-assembler.h"
1618
#include "src/compiler/pipeline.h"
1719
#include "src/compiler/turboshaft/phase.h"
@@ -92,10 +94,11 @@ AssemblerOptions BuiltinAssemblerOptions(Isolate* isolate, Builtin builtin) {
9294
}
9395

9496
using MacroAssemblerGenerator = void (*)(MacroAssembler*);
95-
using CodeAssemblerGenerator = void (*)(compiler::CodeAssemblerState*);
9697
using TurboshaftAssemblerGenerator =
9798
void (*)(compiler::turboshaft::PipelineData*, Isolate*,
9899
compiler::turboshaft::Graph&, Zone*);
100+
using CodeAssemblerGenerator = compiler::Pipeline::CodeAssemblerGenerator;
101+
using CodeAssemblerInstaller = compiler::Pipeline::CodeAssemblerInstaller;
99102

100103
Handle<Code> BuildPlaceholder(Isolate* isolate, Builtin builtin) {
101104
HandleScope scope(isolate);
@@ -189,20 +192,18 @@ Tagged<Code> BuildAdaptor(Isolate* isolate, Builtin builtin,
189192
return *code;
190193
}
191194

192-
// Builder for builtins implemented in TurboFan with JS linkage.
193-
V8_NOINLINE Tagged<Code> BuildWithCodeStubAssemblerJS(
194-
Isolate* isolate, Builtin builtin, CodeAssemblerGenerator generator,
195-
int argc, const char* name) {
196-
HandleScope scope(isolate);
197-
198-
Zone zone(isolate->allocator(), ZONE_NAME, kCompressGraphZone);
199-
compiler::CodeAssemblerState state(isolate, &zone, argc, CodeKind::BUILTIN,
200-
name, builtin);
201-
generator(&state);
202-
DirectHandle<Code> code = compiler::CodeAssembler::GenerateCode(
203-
&state, BuiltinAssemblerOptions(isolate, builtin),
204-
ProfileDataFromFile::TryRead(name));
205-
return *code;
195+
void CompileJSLinkageCodeStubBuiltin(Isolate* isolate, Builtin builtin,
196+
CodeAssemblerGenerator generator,
197+
CodeAssemblerInstaller installer, int argc,
198+
const char* name, int finalize_order,
199+
int& builtins_installed_count) {
200+
std::unique_ptr<TurbofanCompilationJob> job(
201+
compiler::Pipeline::NewJSLinkageCodeStubBuiltinCompilationJob(
202+
isolate, builtin, generator, installer,
203+
BuiltinAssemblerOptions(isolate, builtin), argc, name,
204+
ProfileDataFromFile::TryRead(name), finalize_order));
205+
compiler::CodeAssembler::CompileCode(isolate, std::move(job),
206+
builtins_installed_count);
206207
}
207208

208209
inline constexpr char kTempZoneName[] = "temp-zone";
@@ -244,26 +245,42 @@ V8_NOINLINE Tagged<Code> BuildWithTurboshaftAssemblerCS(
244245
}
245246

246247
// Builder for builtins implemented in TurboFan with CallStub linkage.
247-
V8_NOINLINE Tagged<Code> BuildWithCodeStubAssemblerCS(
248-
Isolate* isolate, Builtin builtin, CodeAssemblerGenerator generator,
249-
CallDescriptors::Key interface_descriptor, const char* name) {
248+
void CompileCSLinkageCodeStubBuiltin(Isolate* isolate, Builtin builtin,
249+
CodeAssemblerGenerator generator,
250+
CodeAssemblerInstaller installer,
251+
CallDescriptors::Key interface_descriptor,
252+
const char* name, int finalize_order,
253+
int& builtins_installed_count) {
250254
// TODO(nicohartmann): Remove this once `BuildWithTurboshaftAssemblerCS` has
251255
// an actual use.
252256
USE(&BuildWithTurboshaftAssemblerCS);
253-
HandleScope scope(isolate);
254-
Zone zone(isolate->allocator(), ZONE_NAME, kCompressGraphZone);
255-
// The interface descriptor with given key must be initialized at this point
256-
// and this construction just queries the details from the descriptors table.
257-
CallInterfaceDescriptor descriptor(interface_descriptor);
258-
// Ensure descriptor is already initialized.
259-
DCHECK_LE(0, descriptor.GetRegisterParameterCount());
260-
compiler::CodeAssemblerState state(isolate, &zone, descriptor,
261-
CodeKind::BUILTIN, name, builtin);
262-
generator(&state);
263-
DirectHandle<Code> code = compiler::CodeAssembler::GenerateCode(
264-
&state, BuiltinAssemblerOptions(isolate, builtin),
265-
ProfileDataFromFile::TryRead(name));
266-
return *code;
257+
std::unique_ptr<TurbofanCompilationJob> job(
258+
compiler::Pipeline::NewCSLinkageCodeStubBuiltinCompilationJob(
259+
isolate, builtin, generator, installer,
260+
BuiltinAssemblerOptions(isolate, builtin), interface_descriptor, name,
261+
ProfileDataFromFile::TryRead(name), finalize_order));
262+
compiler::CodeAssembler::CompileCode(isolate, std::move(job),
263+
builtins_installed_count);
264+
}
265+
266+
void CompileBytecodeHandler(
267+
Isolate* isolate, Builtin builtin, interpreter::OperandScale operand_scale,
268+
interpreter::Bytecode bytecode,
269+
compiler::Pipeline::CodeAssemblerInstaller installer, int finalize_order,
270+
int& builtins_installed_count) {
271+
DCHECK(interpreter::Bytecodes::BytecodeHasHandler(bytecode, operand_scale));
272+
const char* name = Builtins::name(builtin);
273+
auto generator = [bytecode,
274+
operand_scale](compiler::CodeAssemblerState* state) {
275+
interpreter::GenerateBytecodeHandler(state, bytecode, operand_scale);
276+
};
277+
std::unique_ptr<TurbofanCompilationJob> job(
278+
compiler::Pipeline::NewBytecodeHandlerCompilationJob(
279+
isolate, builtin, generator, installer,
280+
BuiltinAssemblerOptions(isolate, builtin), name,
281+
ProfileDataFromFile::TryRead(name), finalize_order));
282+
compiler::CodeAssembler::CompileCode(isolate, std::move(job),
283+
builtins_installed_count);
267284
}
268285

269286
} // anonymous namespace
@@ -341,20 +358,6 @@ void SetupIsolateDelegate::ReplacePlaceholders(Isolate* isolate) {
341358
}
342359
}
343360

344-
namespace {
345-
346-
V8_NOINLINE Tagged<Code> GenerateBytecodeHandler(
347-
Isolate* isolate, Builtin builtin, interpreter::OperandScale operand_scale,
348-
interpreter::Bytecode bytecode) {
349-
DCHECK(interpreter::Bytecodes::BytecodeHasHandler(bytecode, operand_scale));
350-
DirectHandle<Code> code = interpreter::GenerateBytecodeHandler(
351-
isolate, Builtins::name(builtin), bytecode, operand_scale, builtin,
352-
BuiltinAssemblerOptions(isolate, builtin));
353-
return *code;
354-
}
355-
356-
} // namespace
357-
358361
// static
359362
void SetupIsolateDelegate::SetupBuiltinsInternal(Isolate* isolate) {
360363
Builtins* builtins = isolate->builtins();
@@ -370,76 +373,109 @@ void SetupIsolateDelegate::SetupBuiltinsInternal(Isolate* isolate) {
370373
// Create a scope for the handles in the builtins.
371374
HandleScope scope(isolate);
372375

373-
int index = 0;
376+
// Generated builtins are temporarily stored in this array to avoid data races
377+
// on the isolate's builtin table.
378+
std::array<Handle<Code>, Builtins::kBuiltinCount> generated_builtins;
379+
auto install_generated_builtin = [&generated_builtins](Builtin builtin,
380+
Handle<Code> code) {
381+
generated_builtins[Builtins::ToInt(builtin)] = code;
382+
};
383+
384+
int builtins_built_without_job_count = 0;
385+
int builtins_built_with_job_count = 0;
386+
int job_creation_order = 0;
374387
Tagged<Code> code;
375-
#define BUILD_CPP(Name) \
388+
389+
#define BUILD_CPP_WITHOUT_JOB(Name) \
376390
code = BuildAdaptor(isolate, Builtin::k##Name, \
377391
FUNCTION_ADDR(Builtin_##Name), #Name); \
378-
AddBuiltin(builtins, Builtin::k##Name, code); \
379-
index++;
392+
generated_builtins[Builtins::ToInt(Builtin::k##Name)] = \
393+
handle(code, isolate); \
394+
builtins_built_without_job_count++;
380395

381-
#define BUILD_TFJ(Name, Argc, ...) \
382-
code = BuildWithCodeStubAssemblerJS( \
383-
isolate, Builtin::k##Name, &Builtins::Generate_##Name, Argc, #Name); \
384-
AddBuiltin(builtins, Builtin::k##Name, code); \
385-
index++;
396+
#define BUILD_TFJ_WITH_JOB(Name, Argc, ...) \
397+
CompileJSLinkageCodeStubBuiltin( \
398+
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
399+
install_generated_builtin, Argc, #Name, job_creation_order++, \
400+
builtins_built_with_job_count);
386401

387-
#define BUILD_TSC(Name, InterfaceDescriptor) \
402+
#define BUILD_TSC_WITHOUT_JOB(Name, InterfaceDescriptor) \
388403
/* Return size is from the provided CallInterfaceDescriptor. */ \
389404
code = BuildWithTurboshaftAssemblerCS( \
390405
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
391406
CallDescriptors::InterfaceDescriptor, #Name); \
392-
AddBuiltin(builtins, Builtin::k##Name, code); \
393-
index++;
394-
395-
#define BUILD_TFC(Name, InterfaceDescriptor) \
396-
/* Return size is from the provided CallInterfaceDescriptor. */ \
397-
code = BuildWithCodeStubAssemblerCS( \
398-
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
399-
CallDescriptors::InterfaceDescriptor, #Name); \
400-
AddBuiltin(builtins, Builtin::k##Name, code); \
401-
index++;
402-
403-
#define BUILD_TFS(Name, ...) \
407+
generated_builtins[Builtins::ToInt(Builtin::k##Name)] = \
408+
handle(code, isolate); \
409+
builtins_built_without_job_count++;
410+
411+
#define BUILD_TFC_WITH_JOB(Name, InterfaceDescriptor) \
412+
/* Return size is from the provided CallInterfaceDescriptor. */ \
413+
CompileCSLinkageCodeStubBuiltin( \
414+
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
415+
install_generated_builtin, CallDescriptors::InterfaceDescriptor, #Name, \
416+
job_creation_order++, builtins_built_with_job_count);
417+
418+
#define BUILD_TFS_WITH_JOB(Name, ...) \
404419
/* Return size for generic TF builtins (stub linkage) is always 1. */ \
405-
code = BuildWithCodeStubAssemblerCS(isolate, Builtin::k##Name, \
406-
&Builtins::Generate_##Name, \
407-
CallDescriptors::Name, #Name); \
408-
AddBuiltin(builtins, Builtin::k##Name, code); \
409-
index++;
410-
411-
#define BUILD_TFH(Name, InterfaceDescriptor) \
412-
/* Return size for IC builtins/handlers is always 1. */ \
413-
code = BuildWithCodeStubAssemblerCS( \
414-
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
415-
CallDescriptors::InterfaceDescriptor, #Name); \
416-
AddBuiltin(builtins, Builtin::k##Name, code); \
417-
index++;
418-
419-
#define BUILD_BCH(Name, OperandScale, Bytecode) \
420-
code = GenerateBytecodeHandler(isolate, Builtin::k##Name, OperandScale, \
421-
Bytecode); \
422-
AddBuiltin(builtins, Builtin::k##Name, code); \
423-
index++;
424-
425-
#define BUILD_ASM(Name, InterfaceDescriptor) \
420+
CompileCSLinkageCodeStubBuiltin( \
421+
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
422+
install_generated_builtin, CallDescriptors::Name, #Name, \
423+
job_creation_order++, builtins_built_with_job_count);
424+
425+
#define BUILD_TFH_WITH_JOB(Name, InterfaceDescriptor) \
426+
/* Return size for IC builtins/handlers is always 1. */ \
427+
CompileCSLinkageCodeStubBuiltin( \
428+
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
429+
install_generated_builtin, CallDescriptors::InterfaceDescriptor, #Name, \
430+
job_creation_order++, builtins_built_with_job_count);
431+
432+
#define BUILD_BCH_WITH_JOB(Name, OperandScale, Bytecode) \
433+
CompileBytecodeHandler(isolate, Builtin::k##Name, OperandScale, Bytecode, \
434+
install_generated_builtin, job_creation_order++, \
435+
builtins_built_with_job_count);
436+
437+
#define BUILD_ASM_WITHOUT_JOB(Name, InterfaceDescriptor) \
426438
code = BuildWithMacroAssembler(isolate, Builtin::k##Name, \
427439
Builtins::Generate_##Name, #Name); \
428-
AddBuiltin(builtins, Builtin::k##Name, code); \
429-
index++;
430-
431-
BUILTIN_LIST(BUILD_CPP, BUILD_TFJ, BUILD_TSC, BUILD_TFC, BUILD_TFS, BUILD_TFH,
432-
BUILD_BCH, BUILD_ASM);
433-
434-
#undef BUILD_CPP
435-
#undef BUILD_TFJ
436-
#undef BUILD_TSC
437-
#undef BUILD_TFC
438-
#undef BUILD_TFS
439-
#undef BUILD_TFH
440-
#undef BUILD_BCH
441-
#undef BUILD_ASM
442-
CHECK_EQ(Builtins::kBuiltinCount, index);
440+
generated_builtins[Builtins::ToInt(Builtin::k##Name)] = \
441+
handle(code, isolate); \
442+
builtins_built_without_job_count++;
443+
444+
#define NOP(...)
445+
446+
// Build all builtins without jobs first. When concurrent builtin generation
447+
// is enabled, allocations needs to be deterministic. Having main-thread
448+
// generated builtins in the middle of the list breaks determinism.
449+
BUILTIN_LIST(BUILD_CPP_WITHOUT_JOB, NOP, BUILD_TSC_WITHOUT_JOB, NOP, NOP, NOP,
450+
NOP, BUILD_ASM_WITHOUT_JOB);
451+
BUILTIN_LIST(NOP, BUILD_TFJ_WITH_JOB, NOP, BUILD_TFC_WITH_JOB,
452+
BUILD_TFS_WITH_JOB, BUILD_TFH_WITH_JOB, BUILD_BCH_WITH_JOB, NOP)
453+
454+
#undef BUILD_CPP_WITHOUT_JOB
455+
#undef BUILD_TFJ_WITH_JOB
456+
#undef BUILD_TSC_WITHOUT_JOB
457+
#undef BUILD_TFC_WITH_JOB
458+
#undef BUILD_TFS_WITH_JOB
459+
#undef BUILD_TFH_WITH_JOB
460+
#undef BUILD_BCH_WITH_JOB
461+
#undef BUILD_ASM_WITHOUT_JOB
462+
#undef NOP
463+
464+
if (v8_flags.concurrent_builtin_generation) {
465+
isolate->optimizing_compile_dispatcher()->AwaitCompileTasks();
466+
CHECK(isolate->optimizing_compile_dispatcher()->InstallGeneratedBuiltins(
467+
builtins_built_with_job_count));
468+
}
469+
470+
CHECK_EQ(Builtins::kBuiltinCount,
471+
builtins_built_without_job_count + builtins_built_with_job_count);
472+
473+
// Add the generated builtins to the isolate.
474+
for (Builtin builtin = Builtins::kFirst; builtin <= Builtins::kLast;
475+
++builtin) {
476+
AddBuiltin(builtins, builtin,
477+
*generated_builtins[Builtins::ToInt(builtin)]);
478+
}
443479

444480
ReplacePlaceholders(isolate);
445481

‎src/codegen/compiler.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,10 @@ class TurbofanCompilationJob : public OptimizedCompilationJob {
480480
void RecordFunctionCompilation(LogEventListener::CodeTag code_type,
481481
Isolate* isolate) const;
482482

483+
// Only used for concurrent builtin generation, which needs to be
484+
// deterministic and reproducible.
485+
virtual int FinalizeOrder() const { UNREACHABLE(); }
486+
483487
// Intended for use as a globally unique id in trace events.
484488
uint64_t trace_id() const;
485489

0 commit comments

Comments
 (0)