Reorganize shootout benchmarks into a single directory
#260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Now that bytecodealliance#251 and bytecodealliance#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.
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.
These are all migrated over to be a part of the single `shootout` directory.
The new file structure for `shootout` now expects these paths to look like `shootout-ackermann.*.input`.
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.
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`.
fitzgen
approved these changes
Jul 6, 2023
jlb6740
requested changes
Jul 6, 2023
Note, when I tried native does build and seem to execute fine .. it's just are we ok with doing away with the Cargo build system and the update of the READMEs (in the native engine folder and the benchmark folder) where the former's instructions on how to build are now wrong and the later explicitly tells developers that the benchmark folder is required to supply a cargo based build if they want to support a native build. |
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
approved these changes
Jul 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
shootoutartifacts into a single directory. The commits in this PR perform these steps atomically and can be reviewed sequentially. Even with the documentation I added in several places, this supposes a large decrease in the number of lines of code to maintain; the main benefit here is likely coming from using a single script to build all of the native benchmarks. Upon recompilation, the WebAssembly benchmarks have increased in size and I'm not exactly sure why.