Closed kvark closed 3 years ago
This is expected. This crate requires changes in Cargo.toml: https://github.com/aclysma/profiling#using-from-a-binary https://github.com/aclysma/profiling#using-from-a-library
In rafx, I set up the features so that enabling it for the high-level rafx crate also enables it in the other crates below it. You could look at those as a more complete example in a multi-crate project.
On the upside, it allows turning this on/off per crate. On the downside, it requires some setup in Cargo.toml. Sorry if this is not what you expected or if it took you by surprise.
Wait, so if I have a whole stack using profiling
, I can't just enable the feature from the topmost binary target to get all of them? I have to actually expose every single backend feature in the libraries across the stack? This is very unfortunate, and kills one of the aspects of profiling
that I found attractive.
Would you consider solving this properly at your level? You can effectively define different macros (or generate macros code from a meta-macro?) using the feature checks in profiling
itself.
I can't just enable the feature from the topmost binary target to get all of them?
You can up the Cargo.toml files in your crates to work this way. https://github.com/aclysma/rafx/blob/c91bd5fcfdfa3f4d1b43507c32d84b94ffdf1b2e/rafx/Cargo.toml#L63
Would you consider solving this properly at your level? You can effectively define different macros (or generate macros code from a meta-macro?) using the feature checks in profiling itself.
The current method is not ideal but of the options I came up with, it was the one I preferred. It sounds like you have an alternative in mind but I don't follow it. Could you explain it in more detail?
Doing the way it's exposed right now means:
This is very unfortunate. We don't want to add more features, given that we already add profiling
as a dependency.
Instead of doing this:
#[macro_export]
macro_rules! scope {
($name:expr) => {
#[cfg(feature = "a")]
$crate::a::do_stuff();
#[cfg(feature = "b")]
$crate::b::do_stuff();
};
}
Checks can be lifted to outside the macro:
#[cfg(all(feature = "a", not(feature = "b"))]
macro_rules! scope {
($name:expr) => {
$crate::a::do_stuff();
};
}
#[cfg(all(not(feature = "a"), feature = "b")]
macro_rules! scope {
($name:expr) => {
$crate::b::do_stuff();
};
}
#[cfg(all(feature = "a", feature = "b")]
macro_rules! scope {
($name:expr) => {
$crate::a::do_stuff();
$crate::b::do_stuff();
};
}
And so on... you get the idea. This would make the decision of which backend to use, if any, to be purely specified at the top level, transparently.
Clearly, if you have 5 different options there, you can run into a combinatorial exposion of the macros. Therefore I suggested to look into meta-macros (a macro to generate the proper combination, but expanded here in the profiling crate). Maybe something along the lines can be prototyped with profiling
?
Thanks for the suggestion, I’ll give this more thought. If we only support turning one on at a time, it may keep the complexity of this much lower.
Yes, that's what I thought (edit: but didn't say 😅 ). I can't imagine people wanting to profile with multiple tools at once.
Also, it seems to me that such a change could be backwards compatible.
Also, it seems to me that such a change could be backwards compatible.
Yeah I think so too! I'm hoping to prototype it tonight to make sure it's feasible. If it is, then I think it would be a big improvement.
I made the changes we discussed and I think it turned out well.
pub use backend_impl::*;
in the root of the crate.This does seem to be backwards compatible! :)
You can copy/paste this into your root Cargo.toml if you want to give it a try:
[patch.crates-io]
profiling = { git = "https://github.com/aclysma/profiling.git", branch = "feature-flag-rework" }
I'm getting something! So it appears to be working as intended. Thank you for prototyping this quickly!
One thing I'm curious about is how we can could check that the parameters of profile_span!
(and other macros) make sense (i.e. a type check of the parameters). We'd need to test this on CI. We could pick an existing backend, like tracy, I suppose, but it might be a nice touch to have a dedicated "backend" for this?
Something like this? https://github.com/aclysma/profiling/blob/type-check-backend/src/type_check_impl.rs
I guess it could be even simpler..
#[macro_export]
macro_rules! scope {
($name:expr) => {
let _ :&str = $name;
};
($name:expr, $data:expr) => {
let _ :&str = $name;
let _ :&str = $data;
};
}
#[macro_export]
macro_rules! register_thread {
() => {};
($name:expr) => {
let _ :&str = $name;
};
}
#[macro_export]
macro_rules! finish_frame {
() => {};
}
I wonder if this should just be the default. My only hesitation is that the empty implementation at this point technically would not be "no additional code generated". But maybe it's worth it. Surely this would get compiled out.
It appears harmless to me (and useful) to be the default implementation indeed.
I do wonder how the code is going to be generated if you are trying to pass anything complex, i.e. profiling::scope!(format!("foof"));
. Would it still generate the code for on-heap construction and formatting, or would it drop it all on the floor.
Looks like it wouldn't get compiled out, even in release. So I think we will need to keep this as an opt-in behavior
Strange but ok. At least we can keep the "no additional code generated" promise then :)
Published as 1.0.1
Today, the suggested use case doesn't work.
All those feature checks happen after expansion, not before it. And after expansion, it no longer sees the features of
profiling
crate itself. It only sees the features of the target crate, so it would only work if the target itself has those "profiling-with-xxx" features on it.