Skip to content

Commit

Permalink
[wasm][compiler] Adapt inlining budgets to better compromise
Browse files Browse the repository at this point in the history
While the current aggressive inlining budget seems to provide large
value for wasm-gc modules, it provides significantly less impact on
linear wasm. Still, the inlining quite significantly increases
compile time and code size of linear wasm.

The new budgets try to find a balance between linear wasm and wasm-gc.

The updated budgets result in pspdfkit spending half as much time on
Turbofan compilation than with the previous updates while still having
comparable performance.
Compared to without inlining this still implies a ~2x TF compile time
for about 15% benchmark score improvement.

For wasm-gc measured on internal benchmarks the performance regression
seems to be between 5-10% which is not ideal.
Further iterations on the budgets might help us to decrease this
regression however for now it's better to be able to ship inlining
without its full potential than not shipping it due to too large
compile time regressions.

Bug: v8:7748
Change-Id: I419fd6f3252837076c3efd29461c655540b71eb4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4806563
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Matthias Liedtke <mliedtke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#89588}
  • Loading branch information
Liedtke authored and V8 LUCI CQ committed Aug 23, 2023
1 parent 4628443 commit d420e55
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 8 deletions.
3 changes: 2 additions & 1 deletion src/compiler/pipeline.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1771,7 +1771,8 @@ struct WasmInliningPhase {
WasmCompilationData& compilation_data,
ZoneVector<WasmInliningPosition>* inlining_positions,
wasm::WasmFeatures* detected) {
if (!WasmInliner::graph_size_allows_inlining(data->graph()->NodeCount())) {
if (!WasmInliner::graph_size_allows_inlining(
data->graph()->NodeCount(), v8_flags.wasm_inlining_budget)) {
return;
}
GraphReducer graph_reducer(
Expand Down
13 changes: 8 additions & 5 deletions src/compiler/wasm-inlining.cc
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,19 @@ Reduction WasmInliner::ReduceCall(Node* call) {
return NoChange();
}

bool SmallEnoughToInline(size_t current_graph_size, uint32_t candidate_size) {
bool SmallEnoughToInline(size_t current_graph_size, uint32_t candidate_size,
size_t initial_graph_size) {
if (candidate_size > v8_flags.wasm_inlining_max_size) {
return false;
}
if (WasmInliner::graph_size_allows_inlining(current_graph_size +
candidate_size)) {
if (WasmInliner::graph_size_allows_inlining(
current_graph_size + candidate_size, initial_graph_size)) {
return true;
}
// For truly tiny functions, let's be a bit more generous.
return candidate_size <= 12 &&
WasmInliner::graph_size_allows_inlining(current_graph_size - 100);
WasmInliner::graph_size_allows_inlining(current_graph_size - 100,
initial_graph_size);
}

void WasmInliner::Trace(const CandidateInfo& candidate, const char* decision) {
Expand All @@ -149,7 +151,8 @@ void WasmInliner::Finalize() {
// We could build the candidate's graph first and consider its node count,
// but it turns out that wire byte size and node count are quite strongly
// correlated, at about 1.16 nodes per wire byte (measured for J2Wasm).
if (!SmallEnoughToInline(current_graph_size_, candidate.wire_byte_size)) {
if (!SmallEnoughToInline(current_graph_size_, candidate.wire_byte_size,
initial_graph_size_)) {
Trace(candidate, "not enough inlining budget");
continue;
}
Expand Down
9 changes: 7 additions & 2 deletions src/compiler/wasm-inlining.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,13 @@ class WasmInliner final : public AdvancedReducer {
// Inlines calls registered by {Reduce}, until an inlining budget is exceeded.
void Finalize() final;

static bool graph_size_allows_inlining(size_t graph_size) {
return graph_size < v8_flags.wasm_inlining_budget;
static bool graph_size_allows_inlining(size_t graph_size,
size_t initial_graph_size) {
size_t budget =
std::max<size_t>(v8_flags.wasm_inlining_min_budget,
v8_flags.wasm_inlining_factor * initial_graph_size);
budget = std::min<size_t>(v8_flags.wasm_inlining_budget, budget);
return graph_size < budget;
}

private:
Expand Down
6 changes: 6 additions & 0 deletions src/flags/flag-definitions.h
Original file line number Diff line number Diff line change
Expand Up @@ -1431,6 +1431,12 @@ DEFINE_SIZE_T(wasm_inlining_budget, 5000,
"maximum graph size (in TF nodes) that allows inlining more")
DEFINE_SIZE_T(wasm_inlining_max_size, 500,
"maximum function size (in wire bytes) that may be inlined")
DEFINE_SIZE_T(
wasm_inlining_factor, 3,
"maximum multiple graph size (in TF nodes) in comparison to initial size")
DEFINE_SIZE_T(wasm_inlining_min_budget, 50,
"minimum graph size budget (in TF nodes) for which the "
"wasm_inlinining_factor does not apply")
DEFINE_BOOL(trace_wasm_inlining, false, "trace wasm inlining")
DEFINE_BOOL(trace_wasm_typer, false, "trace wasm typer")
DEFINE_BOOL(wasm_final_types, false,
Expand Down

0 comments on commit d420e55

Please sign in to comment.