Add a V8 engine #166
Add a V8 engine #166
Conversation
Do they publish prebuilt |
There was a problem hiding this comment.
Can you add engines/v8/README.md that describes what this is, gives an overview of its current implementation status, and explains why it is here (rough comparison point for wasmtime/cranelift) and why it is not (benchmark wars, getting rigorous benchmark result numbers for V8)
| is_debug = true | ||
| symbol_level = 2 | ||
| v8_optimized_debug = false |
There was a problem hiding this comment.
Does this mean we are building V8 with debug symbols and/or without optimizations?
| st->engine = wasm::Engine::make(); // TODO cannot instantiate an EngineImpl | ||
| // multiple times. |
There was a problem hiding this comment.
Can you do something like
static wasm::Engine* engine;
if (engine == nullptr) {
engine = wasm::Engine::make();
}
st->engine = engine;here?
| public: | ||
| BenchState(Config config) : config(config) {} | ||
| Config config; | ||
| std::unique_ptr<wasm::Engine> engine; |
There was a problem hiding this comment.
And then this would have to become a raw pointer or reference instead of a unique_ptr.
| // std::cerr << "Found import: " << module_str << " " << name_str << | ||
| // std::endl; | ||
| import_names.push_back(ImportName(module_str, name_str)); | ||
| } | ||
| // std::cerr << "Number of imports: " << import_names.size() << std::endl; |
There was a problem hiding this comment.
nitpick: delete this commented out logging or add a proper logging macro that can turn logging on/off at build time or via env var.
| auto proc_exit(const wasm::Val args[], wasm::Val results[]) | ||
| -> wasm::own<wasm::Trap> { | ||
| std::cerr << "proc_exit" << std::endl; | ||
| // exit(args[0].i32()); // TODO this cannot actually exit here; should trap? |
There was a problem hiding this comment.
Yes, WASI traps when you call proc_exit, and saves the exit code in some side state so the embedder can differentiate between an exit and another kind of trap.
| // TODO actually write contents; needs access to linear memory. | ||
| // results[0] = args[0].copy(); | ||
| results[0] = wasm::Val::i32(65553); |
There was a problem hiding this comment.
Why not return the length of the buffer, even if we didn't actually do the write (for now)
This change adds the beginnings of a new V8 engine to Sightglass. It uses V8's
libwee8library as the backing engine and constructs alibengine.soin C++ that is compatible with Sightglass. As-is, it has some limitations (discussed below), but the current change can run benchmarks under very specific circumstances and represents a significant effort to make this all work. I do not expect to include this as a part of CI until some of these limitations are resolved.Limitations, in order of importance:
make test-libwill only run a single iteration.uvwasi; the challenge is that some of the WASI calls are modified, i.e., to collect all stdout and stderr into files. To avoid this limitation for now, this change adds a--no-output-checkto skip checking for collected output.libwee8build is slow: it takes approximately 27 minutes on my machine. Much of the time is spent retrieving the various dependencies of V8. It is not clear to me that all of these are needed and it may be possible to compilelibwee8with less fetching.For now, though, this library is functional: