SpacetimeDB icon indicating copy to clipboard operation
SpacetimeDB copied to clipboard

`spacetime publish` should have flags for configuring `wasm-opt`

Open gefjon opened this issue 1 year ago • 4 comments

Optimizing modules with wasm-opt, at least under the default wasm-opt config, strips their debug symbols, making profiler output (e.g. perf) incomprehensible. We need some way to disable this other than "uninstall wasm-opt."

Bare minimum:

Flag to spacetime publish like --no-wasm-opt which still builds the module in release mode, but does not run wasm-opt. (Currently the only way to publish without running wasm-opt is via --debug, which is obviously undesirable for profiling.)

Ideal:

  • Accept arbitrary flags to pass to wasm-opt, either in an environment variable or a flag.
    • WASM_OPT_FLAGS='-O2 -g -ffm -uim -all' spacetime publish -c
    • spacetime publish -c --wasm-opt-flags '-O2 -g -ffm -uim -all'
  • If a set of flags is not supplied, use a sensible set of defaults.
    • Currently we pass -O2 -all, though it's not clear to me how much thought went into that choice.
    • Someone should check if we can depend on --low-memory-unused and --zero-filled-memory.
    • Determining a set of default flags can wait for a follow-up ticket.
  • Regardless of whether a set of flags is supplied or not, add input and output arguments.
  • If a set of flags is supplied, wasm-opt not found becomes a hard error, rather than a warning.

gefjon avatar Feb 14 '24 16:02 gefjon

  • Currently we pass -O2 -all, though it's not clear to me how much thought went into that choice.

-O2 has been chosen after careful comparisons of different modes on some example STDB modules. It had the best balance between performance and compilation times (-O3 is rarely useful in wasm-opt).

-all was added because C# doesn't emit proper target_features section, without which wasm-opt errors out on post-MVP Wasm features like bulk memory operations.

RReverser avatar Feb 14 '24 17:02 RReverser

FWIW I think -g should be fine to always keep around - if input Wasm was already stripped, it won't add anything, and if it has debug info, then it will be simply preserved. But might be better to only add it in debug builds (which STDB CLI already distinguishes).

RReverser avatar Feb 14 '24 17:02 RReverser

FWIW I think -g should be fine to always keep around - if input Wasm was already stripped, it won't add anything, and if it has debug info, then it will be simply preserved. But might be better to only add it in debug builds (which STDB CLI already distinguishes).

Well, we'd like a way to have release builds with symbols; that's the impetus for this ticket.

gefjon avatar Feb 14 '24 21:02 gefjon

We discussed this in https://github.com/clockworklabs/SpacetimeDB/pull/1743. Summary:

The original motivating use case was addressed by https://github.com/clockworklabs/SpacetimeDB/pull/1013.

@gefjon says:

So long as we're preserving debug symbols, I consider the original issue solved and see no need to remove wasm-opt. I also don't see any downside to providing the flag, but certainly it's not a priority. Long-term I'd like to have a mechanism for passing arbitrary wasm-opt flags through spacetime build/spacetime publish, but again, not a priority.

bfops avatar Oct 01 '24 16:10 bfops