Native benchmarks rely on hard-coded path #259

abrown · 2023-07-05T23:25:24Z

When running a native benchmark that is not named benchmark.so, the native engine will fail due to this hard-coded address:

sightglass/engines/native/libengine/src/lib.rs

Line 285 in 0454654

let native_lib = self.working_dir.join("./benchmark.so");

Previously this was just a paper cut, only affecting in-development benchmarks (one would expect the native engine to open your-benchmark-path.so but it would not) but now that multiple benchmarks can live in a single directory, the path given to the sightglass-cli benchmark command should be the one communicated to the native engine. (Some thoughts: (a) add a new field to the WasmBenchConfig passed in to the native engine in wasm_bench_create, (b) hack the wasm_bytes passed to wasm_bench_compile to contain something like NATIVE_PATH###/path/passed/to/sightglass.so instead of the bytes of the shared library, which are useless.)

The text was updated successfully, but these errors were encountered:

This change refactors how the shootout native benchmarks are built. The `Dockerfile.native` file is retained and is expected to be _the_ way to build the native shared libraries for this kind of benchmarking. A `build-native.sh` script is included in the directory to (a) be used by `Dockerfile.native` and (b) for building the native benchmarks in environments where running Docker may not be possible. Now that all of the benchmarks are built in one directory, the native libraries cannot all be named `benchmark.so`. Because of this and the hard-coded path expected by the native engine (see bytecodealliance#259), this change also modifies the associated `*-native.sh` scripts to set up a temporary directory that looks like the `benchmark.so` environment that was there previously. This additional logic could be removed once bytecodealliance#259 is fixed.

* Move all `shootout` benchmarks into a single directory Now that #251 and #256 make it possible for more than one benchmark to live in a single directory, this change moves all of the shootout artifacts into a single directory. This simply performs the file movement; subsequent commits will make necessary tweaks. * Rename shootout benchmarks in `*.suite` files * Enable native benchmarking in new `shootout` directory This change refactors how the shootout native benchmarks are built. The `Dockerfile.native` file is retained and is expected to be _the_ way to build the native shared libraries for this kind of benchmarking. A `build-native.sh` script is included in the directory to (a) be used by `Dockerfile.native` and (b) for building the native benchmarks in environments where running Docker may not be possible. Now that all of the benchmarks are built in one directory, the native libraries cannot all be named `benchmark.so`. Because of this and the hard-coded path expected by the native engine (see #259), this change also modifies the associated `*-native.sh` scripts to set up a temporary directory that looks like the `benchmark.so` environment that was there previously. This additional logic could be removed once #259 is fixed. * Remove the original `shootout-*` directories These are all migrated over to be a part of the single `shootout` directory. * Update verbiage in native GitHub action * Update `ackermann` to use new `*.input` paths The new file structure for `shootout` now expects these paths to look like `shootout-ackermann.*.input`. * Fix `heapsort` allocation When we allocate the array to sort, we should do so with items of size `double` (64 bits) instead of `double*` (32 bits in WebAssembly). I am very confused as to why this benchmark worked previously, but when I recompiled it prior to this change, it would invariably fail due to accessing addresses beyond the memory bounds. * Recompile `shootout` benchmarks with wasi-sdk v20 * Tweak native scripts This change fixes some issues highlighted by CI: - it adds more verbose output to see which commands are executed - it improves the documentation to clarify how to use certain flags - it fixes slight mistakes in the scripts missed by previous refactoring - and, __most especially__, it alters the order of the parameters passed to compile the native libraries. This last change is indicative of the fragility of the native benchmarks: apparently moving `-lengine` to the end was necessary for the linker to understand which library provides `bench_start` and `bench_end`. * Update documentation Now that `Dockerfile.native` relies on a script, `build-native.sh`, instead of the Cargo build system, the documentation for building native libraries has to change.

jlb6740 · 2023-07-11T19:51:13Z

@abrown 😢. The renaming definitely exposes this. It was coded to be the same basename as the wasm file (benchmark.wasm) and be minimally invasive into some of the other logic, but now we need to go ahead and propagate the name of the so. A proper change.

abrown changed the title ~~Native benchmarks rely on hard-coded address~~ Native benchmarks rely on hard-coded path Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native benchmarks rely on hard-coded path #259

Native benchmarks rely on hard-coded path #259

abrown commented Jul 5, 2023

jlb6740 commented Jul 11, 2023

Native benchmarks rely on hard-coded path #259

Native benchmarks rely on hard-coded path #259

Comments

abrown commented Jul 5, 2023

jlb6740 commented Jul 11, 2023