Implement an incremental compilation cache for Cranelift (#4551)

This is the implementation of https://github.com/bytecodealliance/wasmtime/issues/4155, using the "inverted API" approach suggested by @cfallin (thanks!) in Cranelift, and trait object to provide a backend for an all-included experience in Wasmtime. 

After the suggestion of Chris, `Function` has been split into mostly two parts:

- on the one hand, `FunctionStencil` contains all the fields required during compilation, and that act as a compilation cache key: if two function stencils are the same, then the result of their compilation (`CompiledCodeBase<Stencil>`) will be the same. This makes caching trivial, as the only thing to cache is the `FunctionStencil`.
- on the other hand, `FunctionParameters` contain the... function parameters that are required to finalize the result of compilation into a `CompiledCode` (aka `CompiledCodeBase<Final>`) with proper final relocations etc., by applying fixups and so on.

Most changes are here to accomodate those requirements, in particular that `FunctionStencil` should be `Hash`able to be used as a key in the cache:

- most source locations are now relative to a base source location in the function, and as such they're encoded as `RelSourceLoc` in the `FunctionStencil`. This required changes so that there's no need to explicitly mark a `SourceLoc` as the base source location, it's automatically detected instead the first time a non-default `SourceLoc` is set.
- user-defined external names in the `FunctionStencil` (aka before this patch `ExternalName::User { namespace, index }`) are now references into an external table of `UserExternalNameRef -> UserExternalName`, present in the `FunctionParameters`, and must be explicitly declared using `Function::declare_imported_user_function`.
- some refactorings have been made for function names:
  - `ExternalName` was used as the type for a `Function`'s name; while it thus allowed `ExternalName::Libcall` in this place, this would have been quite confusing to use it there. Instead, a new enum `UserFuncName` is introduced for this name, that's either a user-defined function name (the above `UserExternalName`) or a test case name.
  - The future of `ExternalName` is likely to become a full reference into the `FunctionParameters`'s mapping, instead of being "either a handle for user-defined external names, or the thing itself for other variants". I'm running out of time to do this, and this is not trivial as it implies touching ISLE which I'm less familiar with.

The cache computes a sha256 hash of the `FunctionStencil`, and uses this as the cache key. No equality check (using `PartialEq`) is performed in addition to the hash being the same, as we hope that this is sufficient data to avoid collisions.

A basic fuzz target has been introduced that tries to do the bare minimum:

- check that a function successfully compiled and cached will be also successfully reloaded from the cache, and returns the exact same function.
- check that a trivial modification in the external mapping of `UserExternalNameRef -> UserExternalName` hits the cache, and that other modifications don't hit the cache.
  - This last check is less efficient and less likely to happen, so probably should be rethought a bit.

Thanks to both @alexcrichton and @cfallin for your very useful feedback on Zulip.

Some numbers show that for a large wasm module we're using internally, this is a 20% compile-time speedup, because so many `FunctionStencil`s are the same, even within a single module. For a group of modules that have a lot of code in common, we get hit rates up to 70% when they're used together. When a single function changes in a wasm module, every other function is reloaded; that's still slower than I expect (between 10% and 50% of the overall compile time), so there's likely room for improvement. 

Fixes #4155.
This commit is contained in:
Benjamin Bouvier
2022-08-12 18:47:43 +02:00
committed by GitHub
parent ac9725840d
commit 8a9b1a9025
103 changed files with 2176 additions and 693 deletions

View File

@@ -11,7 +11,7 @@ cargo-fuzz = true
[dependencies]
anyhow = { version = "1.0.19" }
arbitrary = { version = "1.1.0", features = ["derive"] }
cranelift-codegen = { path = "../cranelift/codegen" }
cranelift-codegen = { path = "../cranelift/codegen", features = ["incremental-cache"] }
cranelift-reader = { path = "../cranelift/reader" }
cranelift-wasm = { path = "../cranelift/wasm" }
cranelift-filetests = { path = "../cranelift/filetests" }
@@ -120,3 +120,9 @@ name = "component_api"
path = "fuzz_targets/component_api.rs"
test = false
doc = false
[[bin]]
name = "cranelift-icache"
path = "fuzz_targets/cranelift-icache.rs"
test = false
doc = false

View File

@@ -30,6 +30,10 @@ At the time of writing, we have the following fuzz targets:
* `cranelift-fuzzgen`: Generate a Cranelift function and check that it returns
the same results when compiled to the host and when using the Cranelift
interpreter; only a subset of Cranelift IR is currently supported.
* `cranelift-icache`: Generate a Cranelift function A, applies a small mutation
to its source, yielding a function A', and checks that A compiled +
incremental compilation generates the same machine code as if A' was compiled
from scratch.
* `differential`: Generate a Wasm module and check that Wasmtime returns
the same results when run with two different configurations.
* `differential_spec`: Generate a Wasm module and check that Wasmtime returns

View File

@@ -0,0 +1,148 @@
#![no_main]
use cranelift_codegen::{
cursor::{Cursor, FuncCursor},
incremental_cache as icache,
ir::{self, immediates::Imm64, ExternalName},
isa,
settings::{self, Configurable as _},
Context,
};
use libfuzzer_sys::fuzz_target;
use cranelift_fuzzgen::*;
use target_lexicon::Triple;
fuzz_target!(|func: SingleFunction| {
let mut func = func.0;
let flags = settings::Flags::new({
let mut builder = settings::builder();
// We need llvm ABI extensions for i128 values on x86
builder.set("enable_llvm_abi_extensions", "true").unwrap();
builder
});
let isa_builder = isa::lookup(Triple::host())
.map_err(|err| match err {
isa::LookupError::SupportDisabled => {
"support for architecture disabled at compile time"
}
isa::LookupError::Unsupported => "unsupported architecture",
})
.unwrap();
let isa = isa_builder.finish(flags).unwrap();
let cache_key_hash = icache::compute_cache_key(&*isa, &mut func);
let mut context = Context::for_function(func.clone());
let prev_stencil = match context.compile_stencil(&*isa) {
Ok(stencil) => stencil,
Err(_) => return,
};
let (prev_stencil, serialized) = icache::serialize_compiled(prev_stencil);
let serialized = serialized.expect("serialization should work");
let prev_result = prev_stencil.apply_params(&func.params);
let new_result = icache::try_finish_recompile(&func, &serialized)
.expect("recompilation should always work for identity");
assert_eq!(new_result, prev_result, "MachCompileResult:s don't match");
let new_info = new_result.code_info();
assert_eq!(new_info, prev_result.code_info(), "CodeInfo:s don't match");
// If the func has at least one user-defined func ref, change it to match a
// different external function.
let expect_cache_hit = if let Some(user_ext_ref) =
func.stencil.dfg.ext_funcs.values().find_map(|data| {
if let ExternalName::User(user_ext_ref) = &data.name {
Some(user_ext_ref)
} else {
None
}
}) {
let mut prev = func.params.user_named_funcs()[*user_ext_ref].clone();
prev.index = prev.index.checked_add(1).unwrap_or_else(|| prev.index - 1);
func.params.reset_user_func_name(*user_ext_ref, prev);
true
} else {
// otherwise just randomly change one instruction in the middle and see what happens.
let mut changed = false;
let mut cursor = FuncCursor::new(&mut func);
'out: while let Some(_block) = cursor.next_block() {
while let Some(inst) = cursor.next_inst() {
// It's impractical to do any replacement at this point. Try to find any
// instruction that returns one int value, and replace it with an iconst.
if cursor.func.dfg.inst_results(inst).len() != 1 {
continue;
}
let out_ty = cursor
.func
.dfg
.value_type(cursor.func.dfg.first_result(inst));
match out_ty {
ir::types::I32 | ir::types::I64 => {}
_ => continue,
}
if let ir::InstructionData::UnaryImm {
opcode: ir::Opcode::Iconst,
imm,
} = cursor.func.dfg[inst]
{
let imm = imm.bits();
cursor.func.dfg[inst] = ir::InstructionData::UnaryImm {
opcode: ir::Opcode::Iconst,
imm: Imm64::new(imm.checked_add(1).unwrap_or(imm - 1)),
};
} else {
cursor.func.dfg[inst] = ir::InstructionData::UnaryImm {
opcode: ir::Opcode::Iconst,
imm: Imm64::new(42),
};
}
changed = true;
break 'out;
}
}
if !changed {
return;
}
// We made it so that there shouldn't be a cache hit.
false
};
let new_cache_key_hash = icache::compute_cache_key(&*isa, &mut func);
if expect_cache_hit {
assert!(cache_key_hash == new_cache_key_hash);
} else {
assert!(cache_key_hash != new_cache_key_hash);
}
context = Context::for_function(func.clone());
let after_mutation_result = match context.compile(&*isa) {
Ok(info) => info,
Err(_) => return,
};
if expect_cache_hit {
let after_mutation_result_from_cache = icache::try_finish_recompile(&func, &serialized)
.expect("recompilation should always work for identity");
assert_eq!(*after_mutation_result, after_mutation_result_from_cache);
let new_info = after_mutation_result_from_cache.code_info();
assert_eq!(
new_info,
after_mutation_result.code_info(),
"CodeInfo:s don't match"
);
}
});