Implement Wasm WASI target and fork mode for WasmJs with custom engines#361
Implement Wasm WASI target and fork mode for WasmJs with custom engines#361igoriakovlev wants to merge 8 commits intomasterfrom
Conversation
8576f98 to
c07a932
Compare
| } | ||
|
|
||
| else -> throw IllegalArgumentException("Invalid advanced option name: '$param'. Accepted options: \"nativeFork\", \"nativeGCAfterIteration\", \"jvmForks\", \"jsUseBridge\".") | ||
| "wasmFork" -> require(value is String) { |
There was a problem hiding this comment.
Please validate the value of the option like for the nativeFork
| if (compilationTarget.isNodejsConfigured) { | ||
| val execTask = createNodeJsExec(config, target, compilation, taskName) | ||
| tasks.getByName(config.prefixName(RUN_BENCHMARKS_TASKNAME)).dependsOn(execTask) | ||
| val execTask = createNodeJsExec(config, target, binary.compilation, executableFile, taskName) |
There was a problem hiding this comment.
Please add the dependency on the link task:
execTask.configure { it.dependsOn(binary.linkTask) }
It's one of those warning our tests were printing in CI
| if (compilationTarget.isNodejsConfigured) { | ||
| val execTask = createNodeJsExec(config, target, compilation, taskName) | ||
| tasks.getByName(config.prefixName(RUN_BENCHMARKS_TASKNAME)).dependsOn(execTask) | ||
| val execTask = createNodeJsExec(config, target, binary.compilation, executableFile, taskName) |
There was a problem hiding this comment.
Please add the dependency on the link task:
execTask.configure { it.dependsOn(binary.linkTask) }
It's one of those warning our tests were printing in CI
runtime/wasmJsMain/src/kotlinx/benchmark/wasm/WasmPerProcessExecutors.kt
Show resolved
Hide resolved
| import kotlin.text.replaceFirstChar | ||
|
|
||
| @KotlinxBenchmarkPluginInternalApi | ||
| data class CustomEngine(val name: String, val enginePath: Provider<RegularFile>, val engineArguments: Provider<String>) |
There was a problem hiding this comment.
Is it really internal? If so, it should not be exposed via BenchmarkConfiguration.
Also, please make it a regular class, not a data class.
There was a problem hiding this comment.
Alternative option: keep the class private, add a function customJsEngine(...) that will pack all the arguments to advanced. Also, it seems crucial to explicitly specify that the engine is the Js/WasmJs/WasmWasi engine. Maybe, the runtime or VM would be a better name?
There was a problem hiding this comment.
It is internal because not intended to used by the user. Could you please to
reveal an idea about this function customJsEngine? Where it should be placed, with which visibility? Any opt-ins? Etc. Thank you.
| import kotlin.text.replaceFirstChar | ||
|
|
||
| @KotlinxBenchmarkPluginInternalApi | ||
| data class CustomEngine(val name: String, val enginePath: Provider<RegularFile>, val engineArguments: Provider<String>) |
There was a problem hiding this comment.
Why engineArguments are string, and not a list of strings?
There was a problem hiding this comment.
Well. It is not very handy to have a list when you deal with Providers. Should it be a provider if list? or list of providers? or list of providers of strings? :)
For us (in wasm) to have this function the best way to express our needs is just a provider of string, But if you have a better idea from library point of view i would love to implement it.
There was a problem hiding this comment.
Will make it with Provider
6dc4298 to
3413d05
Compare
e7ef665 to
bb3b759
Compare
bb3b759 to
675d01d
Compare
fzhinkin
left a comment
There was a problem hiding this comment.
There are a few questions popped up during the last review round, but otherwise I'm OK with current implementation given that it is effectively non-public.
| import kotlinx.benchmark.internal.KotlinxBenchmarkRuntimeInternalApi | ||
|
|
||
| /** | ||
| * Runs a complete benchmark suite by iterating over all configured benchmarks and their parameter combinations. |
There was a problem hiding this comment.
If it runs the whole suite, then what's the difference with SuiteExecutor? Should they be a single class / interface? Is there any relation between them?
| * - [runBenchmark] should return raw sample values in nanoseconds, or `null` if the benchmark produced no result | ||
| * - [saveBenchmarkResults] converts raw samples, records the formatted result, and notifies [reporter] | ||
| */ | ||
| @KotlinxBenchmarkRuntimeInternalApi |
There was a problem hiding this comment.
Can we make the interface internal instead? After you introduced KotilnxBenchmarkRuntimeExperimentalApi annotation, can everything that is still annotated with KotlinxBenchmarkRuntimeInternalApi be converted into internal?
| * - [saveBenchmarkResults] converts raw samples, records the formatted result, and notifies [reporter] | ||
| */ | ||
| @KotlinxBenchmarkRuntimeInternalApi | ||
| interface RunAllBenchmarksExtension { |
There was a problem hiding this comment.
Where the Extension comes from? Here, and in CommonBenchmarkExtension. Why the latter can not remain CommonBenchmarkExecutor and this interface be SuiteExecutor?
| package kotlinx.benchmark | ||
|
|
||
| @KotlinxBenchmarkRuntimeExperimentalApi | ||
| abstract class Measurer { |
There was a problem hiding this comment.
Please add some doc describing how Measurer, BenchmarkEngineSupport and overrideEngineSupport are related, why are they needed and how to implement them.
Here we have several major changes:
First of all I have removed
d8support from gradle plugin (it was needed for internal use only)Second I have added
Wasm WASItarget (runned withnodejs)Third implement
wasmFork = perBenchmark. User can use it as in other targets, like inNative.Fourth - support
WasmJs custom engines- it supposed to be used internally.Fifth - as we now have now custom engines I remove internal d8 engine support from the Runtime.
(fixed 287)