Add manual_scope macro - Githubissues

Imberflur commented 3 years ago

Attempt at #3

Imberflur commented 3 years ago

I'm currently leaning towards leaving this functionality out of this crate but I thought it would be interesting to see what it could look like.

aclysma commented 3 years ago

I liked how combining guard objects to a raw struct turned out - if we had more than a couple in the non "manual scope" case it might be worth combining them in that way in general for the normal case. If we had a canonical name for it (like _profiling_guard) then someone could drop it purposefully (undocumented feature - I'd be more comfortable with it not working with one of the profilers if it's not "fully supported" API :) )
If we did something like this, I'd want the api to be like let my_scope = manual_scope!(...); and then they would drop(my_scope)
Optick behaving inconsistently with the rest of the APIs might be a deal-breaker for this to be a first-class part of the API
I'm on the fence whether the amount of additional code (given the library is currently small and single-file) is worth the value, especially since it's pretty straightforward how to do it outside of this crate. (Just less convenient to reuse.)

I agree with you on leaning towards leaving it out for now. Thanks for sharing the the experiment, maybe something will change in the future that makes it an easier sell. Happy to take another look in the future if you come up with something! :)

Imberflur commented 3 years ago

If we had a canonical name for it (like _profiling_guard) then someone could drop it purposefully (undocumented feature - I'd be more comfortable with it not working with one of the profilers if it's not "fully supported" API :) )

I think macro hygiene means the user needs to specify the name to be able to access it. I might not understand what you are proposing here though.

If we did something like this, I'd want the api to be like let my_scope = manual_scope!(...); and then they would drop(my_scope)

That would work as well and feels like a more natural api. I was mainly inspired by the span! macro wgpu uses internally. I think their reason for including the let in the macro was to help keep the usage of the macro on a single line.

Optick behaving inconsistently with the rest of the APIs might be a deal-breaker for this to be a first-class part of the API

yeah :/ theoretically we could just mimic their macro but that involves unsafe and I didn't feel comfortable pulling that in here

aclysma commented 3 years ago

Closing for now but happy to take another look if you have more ideas :)

aclysma / profiling

Add manual_scope macro #5