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

Add support for profiling using tracy #209

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Add support for profiling using tracy #209

merged 1 commit into from
Mar 20, 2024

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Oct 31, 2023

I'm not sure entirely how to deal with profiling::finish_frame! when there are multiple outputs rendering frames, and some more functions could be annotated, but Tracy seems generally useful for profiling with these changes. So they might as well be merged.

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

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

I wonder if we should just drop puffin?

@ids1024
Copy link
Member Author

ids1024 commented Oct 31, 2023

Looks like profiling has support for puffin as well? So presumably if we want we should be able to support both fairly easily without having explicit puffin::profile_function! calls everywhere.

@Drakulix
Copy link
Member

Looks like profiling has support for puffin as well? So presumably if we want we should be able to support both fairly easily without having explicit puffin::profile_function! calls everywhere.

Right. It doesn't hurt to also have puffin-egui available inside the compositor I guess. But we definitely don't want special puffin calls, where we could just use profiling.

@ids1024
Copy link
Member Author

ids1024 commented Nov 7, 2023

Seems profiling doesn't currently support the latest version of puffin: aclysma/profiling#58.

@ids1024
Copy link
Member Author

ids1024 commented Dec 12, 2023

profiling has been updated. But to use puffin 0.18 (which it now uses), we'd need to use puffin_egui 0.24, which also requires egui 0.24, which I ran into an issue updating to: Smithay/smithay-egui#16

@ids1024 ids1024 force-pushed the tracy_jammy branch 2 times, most recently from 2d91d21 to 2d4845c Compare December 12, 2023 23:25
@ids1024
Copy link
Member Author

ids1024 commented Feb 13, 2024

Rebased on master, and added a change I had stashed to work around a panic in UtcOffset::current_local_offset().expect("No yet multithreaded"). That one is annoying, still not sure what to do about it...

@Drakulix
Copy link
Member

Rebased on master, and added a change I had stashed to work around a panic in UtcOffset::current_local_offset().expect("No yet multithreaded"). That one is annoying, still not sure what to do about it...

I mean we do have cosmic-screenshot now, so maybe we can drop the code for creating window screenshots now? That is the only thing that needs the local clock for formatting the file name.

@ids1024
Copy link
Member Author

ids1024 commented Mar 20, 2024

Oh, actually that part can be addressed by temporarily using time::util::local_offset:set_soundness. A bit verbose but it's correct even from a paranoid perspective, as long as tracy's thread isn't engaging in any setenv shenanigans.

I've committed more profiling annotations I've found useful, as well as added them to any function using puffin::profile_function!. profiling won't work with puffin here until various packages are released and updated. But anyway, if we care about puffin this doesn't change anything. If we don't we can remove the puffin code, but that's somewhat independent.

I also cargo update the tracy deps so it's on Tracy 0.10.

This should be fine to merge, and would be useful to have in master.

@ids1024 ids1024 requested a review from Drakulix March 20, 2024 22:02
@Drakulix Drakulix merged commit a1c8b3a into master Mar 20, 2024
3 checks passed
@jackpot51 jackpot51 deleted the tracy_jammy branch April 26, 2024 19:17
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