Switch internal instrumentation macros to use profiling crate instead, allowing instrumentation with different profilers #5150
Conversation
… to profiling crate
… to profiling crate
…as to profiling crate
…new frame usage in eframe to profiling crate
# Conflicts: # crates/egui-wgpu/src/renderer.rs
|
Preview available at https://egui-pr-preview.github.io/pr/5150-pr/profiling-crate |
There was a problem hiding this comment.
Looks good overall, but I would prefer not to have the costs of proc-macros for every egui user, if possible.
We should also document the use of profiling somewhere, with at least a line or two in the crate-level docs for egui eframe.
Btw, you can already combine the profiling output of wgpu with the puffin output of eframe/egui if you enable the right features on both :)
crates/eframe/src/native/app_icon.rs
Outdated
| @@ -58,9 +58,8 @@ enum AppIconStatus { | |||
| /// | |||
| /// Since window creation can be lazy, call this every frame until it's either successfully or gave up. | |||
| /// (See [`AppIconStatus`]) | |||
| #[profiling::function] | |||
There was a problem hiding this comment.
Proc-macros like these have a tendency to increase build-times. Luckily we can opt-out of them by disabling the default features of the profiling crate
There was a problem hiding this comment.
That's a fair point. I don't think the profiling crates currently implements another abstraction that would call puffin::profile_function! under the hood though. So I think we have 3 options here:
- Add
profiling::function!()macro upstream which would callpuffin::profile_function!() - Create our own
egui::profile_function!()macro which callsprofiling::scope!()with similar internal code to figure out the calling function name as puffin does internally in it's macro. - Use
profiling::scope!()and manually type the function name.
Let me know if you have any thoughts on this. I'll have a look and see how feasible option 1 would be tonight
|
Thank you for the feedback @emilk! I've added crate level docs as suggested for both egui and eframe (let me know if this is what you meant). Also opened a pull request for the profiling crate about adding a declarative macro so that we can instrument functions without the procmacro feature enabled, let's see if that is something there is interest for. |
# Conflicts: # Cargo.lock # crates/egui-winit/Cargo.toml
…ke formatting more consistent with current state
…t's only referenced in order to enable it's features which causes instrumentation to be emitted
# Conflicts: # Cargo.toml
…age of crate::profile_function!()
|
I've merged this PR with upstream and replaced usages of procedural macros with the new declarative macro. @emilk would you mind having another look at this, and seeing if this resolves your concerns? |
| @@ -28,3 +30,4 @@ env_logger = { version = "0.10", default-features = false, features = [ | |||
| log = { workspace = true } | |||
| puffin = "0.19" | |||
| puffin_http = "0.16" | |||
| profiling = {workspace = true, features = ["profile-with-puffin"] } | |||
There was a problem hiding this comment.
Always having the profile-with-puffin feature enabled means egui will now always compile puffin, which isn't great. I think instead we should keep the puffin features in the crates and make them enable the profiling/profile-with-puffin feature.
Alternatively we could remove the puffin features and users could just enable the profiling implementation they'd like by enabling the relevant profiling feature themselves.
Also, I don't think we're using the proc macro, so we should set default-features to false.
There was a problem hiding this comment.
Note that this cargo.toml is just in the puffin_profiler sample, based on the name it seems pretty reasonable to require it I think. For egui in general default features are disabled and no profiler is enabled by default I believe
There was a problem hiding this comment.
I just double checked regarding 'default-features', it's set to false in the workspace, so I don't think it should be necessary to put that in individual crates when they inherit settings from the workspace.
Hey! I am not sure if this is something that's been considered before and decided against (I couldn't find any PR's or issues).
This change removes the internal profiling macros in library crates and the
puffinfeature and replaces it with similar functions in the profiling crate. This crate provides a layer of abstraction over various profiler instrumentation crates and allows library users to pick their favorite (supported) profiler.An additional benefit for puffin users is that dependencies of egui are included in the instrumentation output too (mainly wgpu which uses the profiling crate), so more details might be available when profiling.
A breaking change is that instead of using the
puffinfeature on egui, users that want to profile the crate with puffin instead have to enable theprofile-with-puffinfeature on the profiling crate. Similarly they could instead choose to useprofile-with-tracyetc.I tried to add a 'tracy' feature to egui_demo_app in order to showcase , however the /scripts/check.sh currently breaks on mutually exclusive features (which this introduces), so I decided against including it for the initial PR. I'm happy to iterate more on this if there is interest in taking this PR though.
Screenshot showing the additional info for wgpu now available when using puffin
