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.

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

fix: discard arguments in empty backend #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ErichDonGubler
Copy link

@ErichDonGubler ErichDonGubler commented Oct 18, 2024

The WGPU project uses profiling for debugging, and speaking as a maintainer of it, it's been lovely. Thank you so much for this crate!

We wanted to give back by fixing an issue we noticed just now in gfx-rs/wgpu#6422 (comment) where arguments to profiling macros aren't evaluated. This causes the compiler to completely ignore whatever is provided as arguments when no features/backends for profiling are enabled. In turn, this can cause divergence in compiler errors if one accidentally writes code that wouldn't compile when a backend is disabled. For example, this Rust program compiles just fine with profiling 1.0.16 and default-features = false:

fn main() {
	profiling::scope!(blarg, unsafe { absolute_nonsense });
}

As soon as a feature corresponding to a backend of profiling is enabled, the above would fail to compile, as one might expect. This is because Rust actually tries to use the expression arguments provided.

This PR fixes this by explicitly discarding each argument in the empty backend, thus forcing Rust to actually try to use the expressions written. This should make any code written against the empty backend portable across all backends.

@aclysma
Copy link
Owner

aclysma commented Oct 18, 2024

It's interesting that you bring this up, it's something I've been thinking about recently. IMO there's actually several issues intersecting here and the best way forward isn't obvious.

  1. There is a type-check backend that you can hook up in CI that is meant to help address this. It was actually suggested by kvark and I thought wgpu was using it. However, it's possible it wasn't hooked up or was removed at some point. Please see the issue where it was suggested Features don't work inside macros #16 and the PR that implemented it Add a backend that does type checking (for CI purposes) #18

  2. Macros emitting nothing when this crate isn't "turned on" is a first principle of this crate. No overhead is acceptable when it's off, and we do not want to rely on the optimizer to uphold this promise. (See https://godbolt.org/z/sEc85cb8n for an example of how this trivially goes wrong)

  3. I've noticed that the type-check backend actually isn't strict enough to catch compile errors for all backends. Some backends accept any strings for the first argument. Some require &'static str. Tracing all but requires literal strings. We could go with the most conservative, but IMO the restrictions that assure the tracing backend always compiles are overbearing. Even if we find something we like now, upstream crates can change. The superluminal backend actually added the requirement that the scope names are 'static.

  4. Another concern with this crate is that it respects semantic versioning. A stable version 1 is critical to the usefulness of this crate - otherwise libraries using it won't be on the same version and won't interoperate well. And up to now, the public profiling API without features enabled - which is what ends up on crates.io - hasn't enforced anything. Adding any type checking when the crate is turned of is technically going to be a semver problem, and I'm reticent to do that.

I definitely do not like that profiling markers can silently be broken. I'm open to ideas for making this situation better, but I think balancing all of these considerations is tricky.

(One more idea. If a downstream project like wgpu really does want to ensure expressions in scope! are evaluated, the project could wrap the profiling scope macros in macros of its own.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants