Skip to content

Commit

Permalink
Revert "[builtins] Optimize and generate builtins off-main-thread"
Browse files Browse the repository at this point in the history 
This reverts commit b132bd4.

Reason for revert: Broke custom snapshot
https://ci.chromium.org/ui/p/v8/builders/ci/V8 Linux64 - custom snapshot - debug builder/62233/overview

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/ /5863234
> Reviewed-by: Nico Hartmann <[email protected]>
> Commit-Queue: Shu-yu Guo <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#94750}

Bug: 342692713
Change-Id: I31cdcfa4cef316e527bc0676a24ab6e3ec5f0427
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /5670429
Bot-Commit: Rubber Stamper <[email protected]>
Auto-Submit: Shu-yu Guo <[email protected]>
Commit-Queue: Rubber Stamper <[email protected]>
Cr-Commit-Position: refs/heads/main@{#94751}
  • Loading branch information
@syg
syg authored and V8 LUCI CQ committed Jul 1, 2024
1 parent b132bd4 commit cf152c8
Show file tree
Hide file tree
Showing 17 changed files with 132 additions and 670 deletions.
3 changes: 0 additions & 3 deletions src/DEPS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 121,6 @@ specific_include_rules = {
" src/heap/factory-base.h",
" src/heap/local-factory.h",
],
"setup-builtins-internal\.cc": [
" src/compiler/pipeline.h",
],
"snapshot\.cc": [
" src/heap/read-only-promotion.h",
],
Expand Down
25 changes: 8 additions & 17 deletions src/builtins/constants-table-builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 36,9 @@ uint32_t BuiltinsConstantsTableBuilder::AddObject(Handle<Object> object) {
DCHECK_EQ(ReadOnlyRoots(isolate_).empty_fixed_array(),
isolate_->heap()->builtins_constants_table());

// Must be on the main thread.
DCHECK_EQ(ThreadId::Current(), isolate_->thread_id());

// Must be generating embedded builtin code.
DCHECK(isolate_->IsGeneratingEmbeddedBuiltins());

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

// This method is called concurrently from both the main thread and
// compilation threads. Constant indices need to be reproducible during
// builtin generation, so the main thread pre-adds all the constants. A lock
// is still needed since the map data structure is still being concurrently
// accessed.
base::MutexGuard guard(&mutex_);
if (ThreadId::Current() != isolate_->thread_id()) {
auto find_result = map_.Find(object);
DCHECK_NOT_NULL(find_result);
return *find_result;
} else {
auto find_result = map_.FindOrInsert(object);
if (!find_result.already_exists) {
DCHECK(IsHeapObject(*object));
*find_result.entry = map_.size() - 1;
}
return *find_result.entry;
auto find_result = map_.FindOrInsert(object);
if (!find_result.already_exists) {
DCHECK(IsHeapObject(*object));
*find_result.entry = map_.size() - 1;
}
return *find_result.entry;
}

namespace {
Expand Down
4 changes: 0 additions & 4 deletions src/builtins/constants-table-builder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 54,6 @@ class BuiltinsConstantsTableBuilder final {
// Maps objects to corresponding indices within the constants list.
using ConstantsMap = IdentityMap<uint32_t, FreeStoreAllocationPolicy>;
ConstantsMap map_;

// Protects accesses to map_, which is concurrently accessed when generating
// builtins off-main-thread.
base::Mutex mutex_;
};

} // namespace internal
Expand Down
152 changes: 70 additions & 82 deletions src/builtins/setup-builtins-internal.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 7,11 @@
#include "src/builtins/builtins-inl.h"
#include "src/builtins/profile-data-reader.h"
#include "src/codegen/assembler-inl.h"
#include "src/codegen/compiler.h"
#include "src/codegen/interface-descriptors.h"
#include "src/codegen/macro-assembler-inl.h"
#include "src/codegen/macro-assembler.h"
#include "src/codegen/reloc-info-inl.h"
#include "src/common/globals.h"
#include "src/compiler-dispatcher/optimizing-compile-dispatcher.h"
#include "src/compiler/code-assembler.h"
#include "src/compiler/pipeline.h"
#include "src/compiler/turboshaft/phase.h"
Expand Down Expand Up @@ -94,11 92,10 @@ AssemblerOptions BuiltinAssemblerOptions(Isolate* isolate, Builtin builtin) {
}

using MacroAssemblerGenerator = void (*)(MacroAssembler*);
using CodeAssemblerGenerator = void (*)(compiler::CodeAssemblerState*);
using TurboshaftAssemblerGenerator =
void (*)(compiler::turboshaft::PipelineData*, Isolate*,
compiler::turboshaft::Graph&, Zone*);
using CodeAssemblerGenerator = compiler::Pipeline::CodeAssemblerGenerator;
using CodeAssemblerInstaller = compiler::Pipeline::CodeAssemblerInstaller;

Handle<Code> BuildPlaceholder(Isolate* isolate, Builtin builtin) {
HandleScope scope(isolate);
Expand Down Expand Up @@ -192,16 189,20 @@ Tagged<Code> BuildAdaptor(Isolate* isolate, Builtin builtin,
return *code;
}

void CompileJSLinkageCodeStubBuiltin(Isolate* isolate, Builtin builtin,
CodeAssemblerGenerator generator,
CodeAssemblerInstaller installer, int argc,
const char* name) {
std::unique_ptr<TurbofanCompilationJob> job(
compiler::Pipeline::NewJSLinkageCodeStubBuiltinCompilationJob(
isolate, builtin, generator, installer,
BuiltinAssemblerOptions(isolate, builtin), argc, name,
ProfileDataFromFile::TryRead(name)));
compiler::CodeAssembler::CompileCode(isolate, std::move(job));
// Builder for builtins implemented in TurboFan with JS linkage.
V8_NOINLINE Tagged<Code> BuildWithCodeStubAssemblerJS(
Isolate* isolate, Builtin builtin, CodeAssemblerGenerator generator,
int argc, const char* name) {
HandleScope scope(isolate);

Zone zone(isolate->allocator(), ZONE_NAME, kCompressGraphZone);
compiler::CodeAssemblerState state(isolate, &zone, argc, CodeKind::BUILTIN,
name, builtin);
generator(&state);
DirectHandle<Code> code = compiler::CodeAssembler::GenerateCode(
&state, BuiltinAssemblerOptions(isolate, builtin),
ProfileDataFromFile::TryRead(name));
return *code;
}

inline constexpr char kTempZoneName[] = "temp-zone";
Expand Down Expand Up @@ -244,38 245,26 @@ V8_NOINLINE Tagged<Code> BuildWithTurboshaftAssemblerCS(
}

// Builder for builtins implemented in TurboFan with CallStub linkage.
void CompileCSLinkageCodeStubBuiltin(Isolate* isolate, Builtin builtin,
CodeAssemblerGenerator generator,
CodeAssemblerInstaller installer,
CallDescriptors::Key interface_descriptor,
const char* name) {
V8_NOINLINE Tagged<Code> BuildWithCodeStubAssemblerCS(
Isolate* isolate, Builtin builtin, CodeAssemblerGenerator generator,
CallDescriptors::Key interface_descriptor, const char* name) {
// TODO(nicohartmann): Remove this once `BuildWithTurboshaftAssemblerCS` has
// an actual use.
USE(&BuildWithTurboshaftAssemblerCS);
std::unique_ptr<TurbofanCompilationJob> job(
compiler::Pipeline::NewCSLinkageCodeStubBuiltinCompilationJob(
isolate, builtin, generator, installer,
BuiltinAssemblerOptions(isolate, builtin), interface_descriptor, name,
ProfileDataFromFile::TryRead(name)));
compiler::CodeAssembler::CompileCode(isolate, std::move(job));
}

void CompileBytecodeHandler(
Isolate* isolate, Builtin builtin, interpreter::OperandScale operand_scale,
interpreter::Bytecode bytecode,
compiler::Pipeline::CodeAssemblerInstaller installer) {
DCHECK(interpreter::Bytecodes::BytecodeHasHandler(bytecode, operand_scale));
const char* name = Builtins::name(builtin);
auto generator = [bytecode,
operand_scale](compiler::CodeAssemblerState* state) {
interpreter::GenerateBytecodeHandler(state, bytecode, operand_scale);
};
std::unique_ptr<TurbofanCompilationJob> job(
compiler::Pipeline::NewBytecodeHandlerCompilationJob(
isolate, builtin, generator, installer,
BuiltinAssemblerOptions(isolate, builtin), name,
ProfileDataFromFile::TryRead(name)));
compiler::CodeAssembler::CompileCode(isolate, std::move(job));
HandleScope scope(isolate);
Zone zone(isolate->allocator(), ZONE_NAME, kCompressGraphZone);
// The interface descriptor with given key must be initialized at this point
// and this construction just queries the details from the descriptors table.
CallInterfaceDescriptor descriptor(interface_descriptor);
// Ensure descriptor is already initialized.
DCHECK_LE(0, descriptor.GetRegisterParameterCount());
compiler::CodeAssemblerState state(isolate, &zone, descriptor,
CodeKind::BUILTIN, name, builtin);
generator(&state);
DirectHandle<Code> code = compiler::CodeAssembler::GenerateCode(
&state, BuiltinAssemblerOptions(isolate, builtin),
ProfileDataFromFile::TryRead(name));
return *code;
}

} // anonymous namespace
Expand Down Expand Up @@ -353,6 342,20 @@ void SetupIsolateDelegate::ReplacePlaceholders(Isolate* isolate) {
}
}

namespace {

V8_NOINLINE Tagged<Code> GenerateBytecodeHandler(
Isolate* isolate, Builtin builtin, interpreter::OperandScale operand_scale,
interpreter::Bytecode bytecode) {
DCHECK(interpreter::Bytecodes::BytecodeHasHandler(bytecode, operand_scale));
DirectHandle<Code> code = interpreter::GenerateBytecodeHandler(
isolate, Builtins::name(builtin), bytecode, operand_scale, builtin,
BuiltinAssemblerOptions(isolate, builtin));
return *code;
}

} // namespace

// static
void SetupIsolateDelegate::SetupBuiltinsInternal(Isolate* isolate) {
Builtins* builtins = isolate->builtins();
Expand All @@ -376,14 379,11 @@ void SetupIsolateDelegate::SetupBuiltinsInternal(Isolate* isolate) {
AddBuiltin(builtins, Builtin::k##Name, code); \
index ;

#define BUILD_TFJ(Name, Argc, ...) \
CompileJSLinkageCodeStubBuiltin( \
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
[builtins, &index](Handle<Code> code) { \
AddBuiltin(builtins, Builtin::k##Name, *code); \
index ; \
}, \
Argc, #Name);
#define BUILD_TFJ(Name, Argc, ...) \
code = BuildWithCodeStubAssemblerJS( \
isolate, Builtin::k##Name, &Builtins::Generate_##Name, Argc, #Name); \
AddBuiltin(builtins, Builtin::k##Name, code); \
index ;

#define BUILD_TSC(Name, InterfaceDescriptor) \
/* Return size is from the provided CallInterfaceDescriptor. */ \
Expand All @@ -395,40 395,33 @@ void SetupIsolateDelegate::SetupBuiltinsInternal(Isolate* isolate) {

#define BUILD_TFC(Name, InterfaceDescriptor) \
/* Return size is from the provided CallInterfaceDescriptor. */ \
CompileCSLinkageCodeStubBuiltin( \
code = BuildWithCodeStubAssemblerCS( \
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
[builtins, &index](Handle<Code> code) { \
AddBuiltin(builtins, Builtin::k##Name, *code); \
index ; \
}, \
CallDescriptors::InterfaceDescriptor, #Name);
CallDescriptors::InterfaceDescriptor, #Name); \
AddBuiltin(builtins, Builtin::k##Name, code); \
index ;

#define BUILD_TFS(Name, ...) \
/* Return size for generic TF builtins (stub linkage) is always 1. */ \
CompileCSLinkageCodeStubBuiltin( \
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
[builtins, &index](Handle<Code> code) { \
AddBuiltin(builtins, Builtin::k##Name, *code); \
index ; \
}, \
CallDescriptors::Name, #Name);
code = BuildWithCodeStubAssemblerCS(isolate, Builtin::k##Name, \
&Builtins::Generate_##Name, \
CallDescriptors::Name, #Name); \
AddBuiltin(builtins, Builtin::k##Name, code); \
index ;

#define BUILD_TFH(Name, InterfaceDescriptor) \
/* Return size for IC builtins/handlers is always 1. */ \
CompileCSLinkageCodeStubBuiltin( \
code = BuildWithCodeStubAssemblerCS( \
isolate, Builtin::k##Name, &Builtins::Generate_##Name, \
[builtins, &index](Handle<Code> code) { \
AddBuiltin(builtins, Builtin::k##Name, *code); \
index ; \
}, \
CallDescriptors::InterfaceDescriptor, #Name);

#define BUILD_BCH(Name, OperandScale, Bytecode) \
CompileBytecodeHandler(isolate, Builtin::k##Name, OperandScale, Bytecode, \
[builtins, &index](Handle<Code> code) { \
AddBuiltin(builtins, Builtin::k##Name, *code); \
index ; \
});
CallDescriptors::InterfaceDescriptor, #Name); \
AddBuiltin(builtins, Builtin::k##Name, code); \
index ;

#define BUILD_BCH(Name, OperandScale, Bytecode) \
code = GenerateBytecodeHandler(isolate, Builtin::k##Name, OperandScale, \
Bytecode); \
AddBuiltin(builtins, Builtin::k##Name, code); \
index ;

#define BUILD_ASM(Name, InterfaceDescriptor) \
code = BuildWithMacroAssembler(isolate, Builtin::k##Name, \
Expand All @@ -447,11 440,6 @@ void SetupIsolateDelegate::SetupBuiltinsInternal(Isolate* isolate) {
#undef BUILD_TFH
#undef BUILD_BCH
#undef BUILD_ASM

if (v8_flags.concurrent_builtin_generation) {
isolate->optimizing_compile_dispatcher()->AwaitCompileTasks();
isolate->optimizing_compile_dispatcher()->InstallGeneratedBuiltins();
}
CHECK_EQ(Builtins::kBuiltinCount, index);

ReplacePlaceholders(isolate);
Expand Down
44 changes: 5 additions & 39 deletions src/compiler-dispatcher/optimizing-compile-dispatcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 101,7 @@ void OptimizingCompileDispatcher::CompileNext(TurbofanCompilationJob* job,
// Use a mutex to make sure that functions marked for install
// are always also queued.
base::MutexGuard access_output_queue_(&output_queue_mutex_);
output_queue_.push_back(job);
output_queue_.push(job);
}

if (finalize()) isolate_->stack_guard()->RequestInstallCode();
Expand All @@ -114,7 114,7 @@ void OptimizingCompileDispatcher::FlushOutputQueue(bool restore_function_code) {
base::MutexGuard access_output_queue_(&output_queue_mutex_);
if (output_queue_.empty()) return;
job.reset(output_queue_.front());
output_queue_.pop_front();
output_queue_.pop();
}

Compiler::DisposeTurbofanCompilationJob(isolate_, job.get(),
Expand Down Expand Up @@ -186,7 186,7 @@ void OptimizingCompileDispatcher::InstallOptimizedFunctions() {
base::MutexGuard access_output_queue_(&output_queue_mutex_);
if (output_queue_.empty()) return;
job.reset(output_queue_.front());
output_queue_.pop_front();
output_queue_.pop();
}
OptimizedCompilationInfo* info = job->compilation_info();
DirectHandle<JSFunction> function(*info->closure(), isolate_);
Expand All @@ -208,35 208,6 @@ void OptimizingCompileDispatcher::InstallOptimizedFunctions() {
}
}

void OptimizingCompileDispatcher::InstallGeneratedBuiltins() {
// Builtin generation needs to be deterministic, meaning heap allocations
// must happen in a deterministic order. To ensure determinism with
// concurrent compilation, finalize all builtins at the end in ascending
// order of their BuiltinId.

CHECK(isolate_->IsGeneratingEmbeddedBuiltins());
CHECK_EQ(0, input_queue_.Length());

HandleScope handle_scope(isolate_);
base::MutexGuard access_output_queue_(&output_queue_mutex_);

std::sort(output_queue_.begin(), output_queue_.end(),
[](const TurbofanCompilationJob* job1,
const TurbofanCompilationJob* job2) {
Builtin builtin1 = job1->compilation_info()->builtin();
Builtin builtin2 = job2->compilation_info()->builtin();
DCHECK(Builtins::IsBuiltinId(builtin1));
DCHECK(Builtins::IsBuiltinId(builtin2));
return Builtins::ToInt(builtin1) < Builtins::ToInt(builtin2);
});

while (!output_queue_.empty()) {
std::unique_ptr<TurbofanCompilationJob> job(output_queue_.front());
output_queue_.pop_front();
CHECK_EQ(CompilationJob::SUCCEEDED, job->FinalizeJob(isolate_));
}
}

bool OptimizingCompileDispatcher::HasJobs() {
DCHECK_EQ(ThreadId::Current(), isolate_->thread_id());
return job_handle_->IsActive() || !output_queue_.empty();
Expand Down Expand Up @@ -275,14 246,9 @@ void OptimizingCompileDispatcher::Prioritize(

OptimizingCompileDispatcher::OptimizingCompileDispatcher(Isolate* isolate)
: isolate_(isolate),
input_queue_((v8_flags.concurrent_builtin_generation &&
isolate->IsGeneratingEmbeddedBuiltins())
? Builtins::kBuiltinCount
: v8_flags.concurrent_recompilation_queue_length),
input_queue_(v8_flags.concurrent_recompilation_queue_length),
recompilation_delay_(v8_flags.concurrent_recompilation_delay) {
if (v8_flags.concurrent_recompilation ||
(v8_flags.concurrent_builtin_generation &&
isolate->IsGeneratingEmbeddedBuiltins())) {
if (v8_flags.concurrent_recompilation) {
job_handle_ = V8::GetCurrentPlatform()->PostJob(
kTaskPriority, std::make_unique<CompileTask>(isolate, this));
}
Expand Down
3 changes: 1 addition & 2 deletions src/compiler-dispatcher/optimizing-compile-dispatcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 91,6 @@ class V8_EXPORT_PRIVATE OptimizingCompileDispatcher {
void QueueForOptimization(TurbofanCompilationJob* job);
void AwaitCompileTasks();
void InstallOptimizedFunctions();
void InstallGeneratedBuiltins();

inline bool IsQueueAvailable() { return input_queue_.IsAvailable(); }

Expand Down Expand Up @@ -131,7 130,7 @@ class V8_EXPORT_PRIVATE OptimizingCompileDispatcher {
OptimizingCompileDispatcherQueue input_queue_;

// Queue of recompilation tasks ready to be installed (excluding OSR).
std::deque<TurbofanCompilationJob*> output_queue_;
std::queue<TurbofanCompilationJob*> output_queue_;
// Used for job based recompilation which has multiple producers on
// different threads.
base::Mutex output_queue_mutex_;
Expand Down
Loading

0 comments on commit cf152c8

Please sign in to comment.