Closed jlb6740 closed 1 year ago
Hey Johnnnie, thanks for this PR, excited for this functionality so we can keep our finger on Wasm vs native overheads.
Hi @fitzgen, yes this is overdue. Certainly this will enable that directly in standalone sightglass as you describe and I think that is pretty cool. But also this patch will enable that functionality in that separate benchmark we discussed that directly leverages sightlglass. So really this is also intended to be first step for that.
The thing that I don't see here, but which we talked about before the holiday break, was defining and documenting an ABI/pattern for how new (and existing) benchmarks can support native execution in addition to the default Wasm execution.
Yes, I remember that discussion but I don't remember the details of what was concluded we wanted. Here I've made this so that the native leverages the same bench_start and bench_end api that wasmtime uses. So for measuring execution, there are zero modifications needed to the benchmark source for the benchmarks already here. For benchmarks we add, the same api is still used. The two things that aren't captured though are compile time and instantiation time. Compile time I don't think makes sense because we also don't quote the equivalent compile time for the Wasm files here .. which is actually done offline. For instantiation time I was thinking about capturing the loading of the shared library but then I think we'd need to also have that step recorded with the Wasm as well, which I don't think is done in isolation right now. But like I said, I don't really remember what we concluded so please refresh my memory on what types of things to document ABI-wise or to encourage good citizenship.
I'll start with Andrew's comments, I knew this would need some cleanup when I pushed but just thought it was in a good state to start review.
@fitzgen and @abrown .. I think this is ready for another review. All comments have been responded to .. some with code, others with explanation that may or may not be satisfying to you. Let me know.
@abrown This native patch is ready for follow-up review. Ackermann is failing because the output doesn't match the input, not because of a problem with this patch. So why doesn't the wasm fail for Ackermann? Don't know yet, but it's not reading the input files and reporting n and m as 0 which is what the results file was wrongly expecting. Patch #236 is to at least fix the output file. That patch allows the native build and run of ackermann to test properly here.
We discussed this PR at today's Cranelift meeting, and one thing we wanted to make sure of was that adding a new Wasm benchmark doesn't require also adding support for the native build up front (i.e. that functionality can come in subsequent PRs). This may mean that the CI job that randomly chooses benchmarks to build as native needs to have an allow list of which benchmarks support compiling to native.
We discussed this PR at today's Cranelift meeting, and one thing we wanted to make sure of was that adding a new Wasm benchmark doesn't require also adding support for the native build up front (i.e. that functionality can come in subsequent PRs). This may mean that the CI job that randomly chooses benchmarks to build as native needs to have an allow list of which benchmarks support compiling to native.
Hi @fitzgen, sorry I had an overlap this morning but I heard this was discussed. Yes, when you add a benchmark for Wasm, adding support for native is completely optional. CI would not break. The CI code that targets native looks for a docker.native file in the benchmark's base directory. If that file does not exist for a benchmark then CI targeting native ignores that benchmark. In fact, this patch only adds native build scripts for sightglass benchmarks. In future PRs we can add for the other benchmarks where we are interested in it's native score.
Currently does not support writing to stderr but would like to do that and a few other things in a follow-up patch. This patch does not attempt to implement native support for every benchmark as it is intended to be a patch for the native engine, that said, it does include native build support for the shootout-benchmarks. shootout-ackermann does not work because the source is broken but that is fixed by this patch: https://github.com/bytecodealliance/sightglass/pull/227. After this patch merges ackermann still does not run to completion. That issue is not understood yet and will be fixed in a follow-up patch.