SKIKO-1159: introduce wasm-opt build step - #1247
Conversation
569c0b9 to
7d463c7
Compare
7d463c7 to
cee1838
Compare
Shagen Ogandzhanian (Schahen)
left a comment
Shagen Ogandzhanian (Schahen)
left a comment
There was a problem hiding this comment.
Apart from the comments left, there's one fundamental thing this PR should take care of so that we won't harm workflow for our fellow skiko developers.
the EMSDK_DIR is a good direction - let's discuss whether it should be gradle property rather then environment variable (Ivan Matkov (@MatkovIvan) what is your though on this) - however ideologically its the right thing to do.
However what will be broken is that for people who don't have this param set but already been relying on the existence of emsdk/upstream (to which the which emcc is resolved).
Can you please, add following logic:
-
if EMSDK_DIR is present (again, later on may be we'll switch to a property) then both emcc and wasm-opt are resolved relying on it
-
if it is not present, then we fallback to emcc and if that emcc is actually in the upstream folder of the emsdk then we resolve wasm-opt accordingly
-
otherwise we fall back to wasm-opt available globally
| if (logger.isInfoEnabled || argFile == null) { | ||
| val argsString = args.toArray().joinToString(", ", prefix = "[", postfix = "]") | ||
| logger.info("$prefix: $argsString") | ||
| val logLevel = if (logger.isInfoEnabled) LogLevel.INFO else LogLevel.WARN |
There was a problem hiding this comment.
What's the purpose of this changes?
There was a problem hiding this comment.
I've oriented myself on the LinkSkikoTask.kt and the CompileSkikoCppTask.kt. Both store their arguments for the emcc command in a text file and call it with @path/to/args.txt. That syntax is not support by wasm-opt. So we need to pass the arguments directly. But that method requires an arg-file. So I updated the method to also accept no arg-file while the result is unchanged and still respects log-levels like before.
|
Another thing that make sense to do in the framework of this PR is to make sure that emcc step is not invoking wasm-opt by itself. |
I just did a few experiments and I couldn't find a way to just disable the wasm-opt optimization without having to sacrifice other optimizations that emcc currently does. |
… improve documentation
2c8eeeb to
3532125
Compare
|
Dustin Feucht (@nopjar) are you comparing with the +wasm-opt scenario? can you elaborate on that? |
|
Yes, passing If I understand it correctly, it passes the same arg to clang and wasm-opt. We can't therefore only omit one. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated wasm-opt optimization step in the wasm build pipeline so the produced .wasm is post-processed separately from the link step, aiming to reduce binary size (e.g., using -Oz).
Changes:
- Added a new Gradle task (
OptimizeSkikoWasmTask) and wired it into the wasm build so the jar packages the optimized.wasm. - Implemented
wasm-optdiscovery logic (EMSDK-based and PATH-based) for the optimization step. - Adjusted native tool argument logging behavior to support tasks that don’t use an args file.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| skiko/buildSrc/src/main/kotlin/tasks/configuration/WasmTasksConfiguration.kt | Registers and wires an optimization task into the wasm build/jar pipeline; adds wasm-opt discovery logic. |
| skiko/buildSrc/src/main/kotlin/OptimizeSkikoWasmTask.kt | New Gradle task that runs wasm-opt to optimize a produced wasm artifact. |
| skiko/buildSrc/src/main/kotlin/AbstractSkikoNativeToolTask.kt | Updates argument logging to handle calls without an args file and adjusts log level behavior. |
Comments suppressed due to low confidence (1)
skiko/buildSrc/src/main/kotlin/OptimizeSkikoWasmTask.kt:40
- After switching
inputFileto aRegularFileProperty, this should pass the input file path to wasm-opt (instead of theRegularFileobject).
arg(inputFile.get())
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected fun logArgs(prefix: String, args: ArgBuilder, argFile: File? = null) { | ||
| if (logger.isInfoEnabled || argFile == null) { | ||
| val argsString = args.toArray().joinToString(", ", prefix = "[", postfix = "]") | ||
| logger.info("$prefix: $argsString") | ||
| val logLevel = if (logger.isInfoEnabled) LogLevel.INFO else LogLevel.WARN | ||
| logger.log(logLevel, "$prefix: $argsString") |
…put/output handling
…tFileName property With having an explicit `@OutputFile` we end up with the same output being declared multiple times as `outDir` is also declared with `@OutputDir`. This will result in having the same artifact (`skiko.wasm`) multiple times present. But as the whole directory is marked as an output, it should not interfere with gradle.
…sing Refactored to process all files in the input directory, supporting `.wasm` optimization and `.mjs` file copying. Updated task configuration to use `DirectoryProperty` for input and adjusted output file naming conventions for better clarity.
Simplified wasm-opt path resolution to handle OS-specific executable names and better fallback handling. Removed unused imports and redundant code for efficiency.
Updated task dependencies to use `optimizeWasm` instead of `linkWasm` for improved Wasm build optimizations. Introduced `optimizeWasmD8` task and refactored related task logic for better modularity and consistency. Reintroducing the buildSuffix as we now have two different optimize folders.
Refactored .mjs file processing to update references to optimized .wasm filenames, ensuring correct linkage with emcc-generated outputs. Removed unused `Files` import.
Shagen Ogandzhanian (Schahen)
left a comment
Shagen Ogandzhanian (Schahen)
left a comment
There was a problem hiding this comment.
I don't have any strong subjections anymore
Corrected environment variable name for wasm-opt path resolution to align with the standard EMSDK setup.
|
|
||
| flags.addAll( | ||
| listOf( | ||
| "-Oz", // set optimization level to compress (highest size reduction) |
There was a problem hiding this comment.
Since -Oz optimizes size, it might affect the performance (comparing with -O2).
We have Compose benchmarks - https://github.com/JetBrains/compose-multiplatform/tree/master/benchmarks/multiplatform
Have a look at different modes in README - https://github.com/JetBrains/compose-multiplatform/tree/master/benchmarks/multiplatform#benchmark-modes
Checking with SIMPLE and REAL should be enough.
Before leaving it -Oz it's worth to verify O2 vs Oz on Compose benchmarks. Could you please check it?
Alternatively, to move forward with the PR sooner than later, we can set O2 here for now and then compare O2 vs Oz on the benchmarks + change to Oz in a new PR.
What do you prefer?
There was a problem hiding this comment.
After running some benchmarks with different cases, the conclusion is that with -Oz we do not have a regression overall. Neither compared to the current master nor with just passing -O2 again.
This table illustrates it to some extend. Also one needs to keep in mind that of course some variation in the timings is possible as various external factors influence the execution. Therefore, there were also runs where -Oz performed better and worse than master in almost all categories. But it has been always just a few percent. So I'd say that there is almost no impact (if at all) to performance.
| Benchmark | -Oz (ms) | -O2 (ms) | master (ms) |
|---|---|---|---|
| AnimatedVisibility | 0.090 | 0.091 | 0.090 |
| CanvasDrawing | 9.524 | 9.621 | 9.552 |
| HeavyShader | 1.612 | 1.626 | 1.597 |
| LazyGrid | 4.695 | 4.635 | 4.642 |
| LazyGrid-ItemLaunchedEffect | 4.660 | 4.602 | 4.689 |
| LazyGrid-SmoothScroll | 0.448 | 0.443 | 0.457 |
| LazyGrid-SmoothScroll-ItemLaunchedEffect | 0.463 | 0.463 | 0.467 |
| LazyList | 0.124 | 0.124 | 0.124 |
| MultipleComponents | 0.179 | 0.179 | 0.182 |
| MultipleComponents-NoVectorGraphics | 0.146 | 0.145 | 0.147 |
| TextLayout | 41.874 | 41.532 | 41.423 |
| VisualEffects | 1.034 | 1.024 | 1.011 |
So if nothing from your side speak against it, I would go with the -Oz step as it also reduces size.
Shagen Ogandzhanian (Schahen)
changed the title
Updated Wasm task configuration to skip stripping debug info when profiling is enabled, ensuring compatibility with profiling workflows.
This PR introduces a separate optimization step for wasm target.
It is a cleaner approach than the one included in #1242.
Emscripten basically does the same, but with
-O2instead of-Oz, which results in a size reduction of ~200KB (9 MB -> 8.8 MB).Ticket can be found here.