Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable trace logs by default to optimize module compilation #6548

Closed
TimonPost opened this issue Jun 9, 2023 · 2 comments · Fixed by #6549
Closed

Disable trace logs by default to optimize module compilation #6548

TimonPost opened this issue Jun 9, 2023 · 2 comments · Fixed by #6549

Comments

@TimonPost
Copy link
Contributor

TimonPost commented Jun 9, 2023 •
edited
Loading

There has been a performance regression for module compilation. The PR #5382 added the trace-log feature by default (this line)

Historically high-frequent trace logs have been a performance bottleneck. Some efforts were made to revert that (#4481, #4484, #2758). In #4484 @bnjbvr added this feature flag, claiming perf increment of 32%, whereafter this flag was enabled by default in #5382.

The following numbers are from sampling the app once before and after with the macos instruments profiler.

The module compilation code takes in total 29.5 sec, of that three log functions here take up ~13 secs in total (43% of total).

image

Disabling trace logs results in total 10.3s, which is roughly ~65% faster.

image

Branch for this instrumentation can be found here: https://github.com/bytecodealliance/wasmtime/compare/main...TimonPost:wasmtime:timon/make-trace-logs-not-default?expand=1

Feature

We probably want to disable 'trace-log' by default as shown this takes ~60% of the total module compilation. Also we likely want to expose upstream control over this feature.

Benefit

Speeds up compilation by 60% :).

Implementation

  • Remove the trace-log feature from the default features array in codegen.
  • Add trace-log feature to various crates that depend on codegen, and from those crates expose the ability to 'enable' trace logs as opt in.
  • Also expose this feature for wasmtime crate so that we can decide on enabling/disabling logs.

Details

  • Macos 13.0.1, Apple M1 Max
  • Precompiled Wasm module is roughly 400MB
@TimonPost TimonPost mentioned this issue Jun 9, 2023
Open
@jameysharp jameysharp mentioned this issue Jun 9, 2023
Merged
jameysharp added a commit to jameysharp/wasmtime that referenced this issue Jun 9, 2023
@jameysharp
cranelift: don't enable trace-log feature by default
c97cca8 
In bytecodealliance#5382 ("egraph support: rewrite to work in terms of CLIF data
structures"), we added the `trace-log` feature to the set of default
features for `cranelift-codegen`. I think this was an accident, probably
added while debugging and overlooked when cleaning up to merge.

So let's undo that change. Fixes bytecodealliance#6548.
@jameysharp
Copy link
Contributor

I think this was just an accident, so I've opened #6549 as the minimal fix. It would be nice to also add trace-log features to other crates which enable the feature in cranelift-codegen, but I don't think it's immediately necessary.

@repi
Copy link
Contributor

repi commented Jun 9, 2023

Thanks that minimal fix improves compilation speed for our big model on my machine from 7.0 s -> 5.55s 🎉 which is very significant and about what we saw last time when we implemented the feature flag for excluding the heavy trace log statements.

jameysharp added a commit to jameysharp/wasmtime that referenced this issue Jun 9, 2023
@jameysharp
cranelift: don't enable trace-log feature by default (bytecodeallianc…
b613b9d 
…e#6549)

In bytecodealliance#5382 ("egraph support: rewrite to work in terms of CLIF data
structures"), we added the `trace-log` feature to the set of default
features for `cranelift-codegen`. I think this was an accident, probably
added while debugging and overlooked when cleaning up to merge.

So let's undo that change. Fixes bytecodealliance#6548.
jameysharp added a commit to jameysharp/wasmtime that referenced this issue Jun 9, 2023
@jameysharp
cranelift: don't enable trace-log feature by default (bytecodeallianc…
ae489e6 
…e#6549)

In bytecodealliance#5382 ("egraph support: rewrite to work in terms of CLIF data
structures"), we added the `trace-log` feature to the set of default
features for `cranelift-codegen`. I think this was an accident, probably
added while debugging and overlooked when cleaning up to merge.

So let's undo that change. Fixes bytecodealliance#6548.
alexcrichton pushed a commit that referenced this issue Jun 12, 2023
@jameysharp
cranelift: don't enable trace-log feature by default (#6549) (#6552)
854fe1a 
In #5382 ("egraph support: rewrite to work in terms of CLIF data
structures"), we added the `trace-log` feature to the set of default
features for `cranelift-codegen`. I think this was an accident, probably
added while debugging and overlooked when cleaning up to merge.

So let's undo that change. Fixes #6548.
jameysharp added a commit to jameysharp/wasmtime that referenced this issue Jun 12, 2023
@jameysharp
cranelift: don't enable trace-log feature by default (bytecodeallianc…
a8c3c33 
…e#6549)

In bytecodealliance#5382 ("egraph support: rewrite to work in terms of CLIF data
structures"), we added the `trace-log` feature to the set of default
features for `cranelift-codegen`. I think this was an accident, probably
added while debugging and overlooked when cleaning up to merge.

So let's undo that change. Fixes bytecodealliance#6548.
jameysharp added a commit that referenced this issue Jun 12, 2023
@jameysharp
cranelift: don't enable trace-log feature by default (#6549) (#6553)
ad1fccf 
In #5382 ("egraph support: rewrite to work in terms of CLIF data
structures"), we added the `trace-log` feature to the set of default
features for `cranelift-codegen`. I think this was an accident, probably
added while debugging and overlooked when cleaning up to merge.

So let's undo that change. Fixes #6548.
@TimonPost TimonPost mentioned this issue Aug 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cranelift: don't enable trace-log feature by default jameysharp/wasmtime
3 participants
@jameysharp @repi @TimonPost