Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental Native Compilation Refactor #2272

Closed
wants to merge 41 commits into from
Closed

Conversation

syrusakbary
Copy link
Member

@syrusakbary syrusakbary commented Apr 29, 2021

Description

This PR refactors the experimental native compilation, making it much more resilient.

  • It speeds up LLVM Native compilation by bypassing the single-threaded linking via LLVM, and emitting directly multiple object files (one per module). Therefore we speed it up via multi-threaded linking in via the lld linker (we can embed it in the future)
  • It refactors the output of the experimental native compilation to be just a struct with two fields:
    1. A list of object file contents
    2. The compilation frame info for each of the functions (containing the function length)
  • It parses the function size per module and outputs it in the frame info.

This PR is also designed in mind to make much easier the creation and parsing of SourceMaps that we can use later to detect traps in an exact location.

For the trap info to be completely binded we will need to merge this PR first:
#2250

And for traps to be working all the way on LLVM, we will need to merge this after:
#2216

Edit by @Hywan: Fixes #1727

Review

  • Add a short description of the change to the CHANGELOG.md file

lib/compiler-llvm/src/compiler.rs Outdated Show resolved Hide resolved
lib/compiler/src/compiler.rs Outdated Show resolved Hide resolved
lib/compiler-llvm/src/compiler.rs Show resolved Hide resolved
lib/compiler/src/compiler.rs Outdated Show resolved Hide resolved
lib/engine-native/src/artifact.rs Outdated Show resolved Hide resolved
lib/engine-native/src/serialize.rs Show resolved Hide resolved
lib/engine-object-file/src/serialize.rs Outdated Show resolved Hide resolved
.args(link_aganist_extra_libs)
.arg("-o")
.arg(&self.output_path);
println!("COMMAND: {:?}", command);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Remove

lib/compiler-llvm/src/object_file.rs Outdated Show resolved Hide resolved
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct ExperimentalNativeCompilation {
/// The contents of emitted object files
pub object_files: Vec<Vec<u8>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the comment document the order of these? Is the order deterministic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the order is deterministic as the current implementation (per llvm-compilation), but IMHO should not be required since it's just an implementation detail?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please comment that the order is irrelevant but deterministic. It helps the next person understand this data.

lib/engine-native/src/artifact.rs Outdated Show resolved Hide resolved
lib/compiler-llvm/src/object_file.rs Outdated Show resolved Hide resolved
lib/compiler-llvm/src/object_file.rs Outdated Show resolved Hide resolved
lib/engine-object-file/src/artifact.rs Outdated Show resolved Hide resolved
lib/engine-native/src/serialize.rs Outdated Show resolved Hide resolved
pub fn serialize(&mut self) -> Result<Vec<u8>, CompileError> {
/// Serialize the Metadata into bytes
/// The bytes will have the following format:
/// RKYV serialization (any length) + POS (8 bytes) + Endian (1 byte)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POS?

lib/engine-native/src/serialize.rs Outdated Show resolved Hide resolved
lib/engine-object-file/src/serialize.rs Outdated Show resolved Hide resolved
Comment on lines 73 to 76
#[cfg(not(windows))]
let wasm_object_path = PathBuf::from("wasm.o");
let wasm_object_path = PathBuf::from("wasm.a");
#[cfg(windows)]
let wasm_object_path = PathBuf::from("wasm.obj");
let wasm_object_path = PathBuf::from("wasm.a");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer need windows/non-windows here since they're the same.

lib/engine-native/src/artifact.rs Outdated Show resolved Hide resolved
lib/engine-native/src/artifact.rs Outdated Show resolved Hide resolved
lib/engine-native/src/artifact.rs Show resolved Hide resolved
lib/engine-native/src/serialize.rs Outdated Show resolved Hide resolved
lib/engine-native/src/serialize.rs Show resolved Hide resolved
lib/engine-object-file/src/artifact.rs Outdated Show resolved Hide resolved
lib/engine-object-file/src/serialize.rs Outdated Show resolved Hide resolved
@syrusakbary
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request May 24, 2021
@bors
Copy link
Contributor

bors bot commented May 24, 2021

try

Build failed:

@syrusakbary syrusakbary mentioned this pull request May 25, 2021
4 tasks
bors bot added a commit that referenced this pull request May 26, 2021
2342: Engine native (slim) r=syrusakbary a=syrusakbary

<!-- 
Prior to submitting a PR, review the CONTRIBUTING.md document for recommendations on how to test:
https://github.com/wasmerio/wasmer/blob/master/CONTRIBUTING.md#pull-requests

-->

# Description

This PR is the slim version of #2272. It advances on:
* [x] Stops encoding the endian into the serialized data (rkyv already does that for us since `0.6.1`)
* [x] Sets the proper function lengths in the native engine
* [x] Catch signal traps only when the pc is originated from a Wasm function, and not in the host (fixing #2328)

<!-- 
Provide details regarding the change including motivation,
links to related issues, and the context of the PR.
-->

# Review

- [ ] Add a short description of the change to the CHANGELOG.md file


Co-authored-by: Syrus <[email protected]>
Co-authored-by: Syrus Akbary <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cranelift reports wrong trap types with dylib engine
2 participants