Skip to content

cranelift: Introduce a feature to enable trace logs#4484

Merged
bnjbvr merged 2 commits intobytecodealliance:mainfrom
bnjbvr:log-macros
Aug 1, 2022
Merged

cranelift: Introduce a feature to enable trace logs#4484
bnjbvr merged 2 commits intobytecodealliance:mainfrom
bnjbvr:log-macros

Conversation

@bnjbvr
Copy link
Copy Markdown
Member

@bnjbvr bnjbvr commented Jul 20, 2022

  • First commit introduces a Cargo feature (disabled by default) to enable/disable trace logs in cranelift-codegen, as well as a macro that will call into log::trace only if the feature is enabled, following exactly what regalloc2 does. If downstream users set their log::max_level to Trace, they would pay the cost for the logging infrastructure, even if the logger eventually decides to throw away the logs' contents, so it's better overall to not call into the logging infra at all.
  • Second commit: previously disassembly was emitted if explicitly enabled (only in testing setups), or when the log level was debug or more. I suppose this was done during hacking on regalloc2 (as the commit introducing this change is the switch to regalloc2), and never reverted, so I've done that here. We could add a cranelift flag to be able to request that at runtime with the CLI, if needs be.

With this change and those from other PRs, there's no more meaningful difference between a Wasmtime embedding setting the log's max level to trace or above.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen labels Jul 20, 2022
@alexcrichton
Copy link
Copy Markdown
Member

To confirm, does this problem stem from the desire to enable trace logging for some modules in your program, but not the cranelift modules? In such a situation is the logging framework's implementation of "no, don't actually emit this trace log" for the cranelift crates too expensive?

(I can certainly imagine that's true, just want to confirm)

@alexcrichton
Copy link
Copy Markdown
Member

Also apologies for the broken CI, but if you rebase it should be back-to-green.

@bnjbvr
Copy link
Copy Markdown
Member Author

bnjbvr commented Jul 21, 2022

To confirm, does this problem stem from the desire to enable trace logging for some modules in your program, but not the cranelift modules?

Yes indeed.

In such a situation is the logging framework's implementation of "no, don't actually emit this trace log" for the cranelift crates too expensive?

Yes, specifically we create something that we feed the log calls (and that's the expensive part here), but then the logger implementation decides to throw it away based on its own configuration.

@bnjbvr bnjbvr requested a review from cfallin July 26, 2022 14:57
Copy link
Copy Markdown
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I left this hanging and missed it on my review queue. This looks good!

@cfallin
Copy link
Copy Markdown
Member

cfallin commented Jul 29, 2022

I think it needs a rebase again now that #4553 merged; happy to merge once rebased.

@bnjbvr
Copy link
Copy Markdown
Member Author

bnjbvr commented Aug 1, 2022

This now passes CI and as there wasn't any pending comment, will merge. Thanks for the review!

@bnjbvr bnjbvr merged commit 8d02243 into bytecodealliance:main Aug 1, 2022
@bnjbvr bnjbvr deleted the log-macros branch August 1, 2022 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants