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

Turn configuration options into const generic params #88

Closed
wants to merge 2 commits into from
Closed

Turn configuration options into const generic params #88

wants to merge 2 commits into from

Conversation

SUPERCILEX
Copy link
Contributor

@SUPERCILEX SUPERCILEX commented Jan 1, 2024

Depends on #87.

This is a bit of a so preoccupied with whether they could, they didn’t stop to think if they should moment, but I realized we can make our primitive config options const generic params. The benefit is that the optimizer can eliminate a bunch of branches and dead code that would have been very hard to prove safe to remove before. There's precedent for doing something similar in other crates: https://docs.rs/sharded-slab/0.1.7/sharded_slab/pool/struct.Pool.html#method.new_with_config.

Also, I don't think this makes the API worse, it's basically just different syntax (unless you want this stuff to be dynamic in which case you're screwed):

   tracing_tracy::TracyLayer::new()
                        .with_stack_depth::<32>()
                        .with_fields_in_zone_name::<false>(),

Thoughts?

@nagisa
Copy link
Owner

nagisa commented Jan 2, 2024

Thoughts?

NACK if its an either/or ordeal. I know there are folks who configure their layers dynamically. But. We could make an API allow both dynamic and static configuration:

TracyLayer::<S, Z>::with_stack_depth<NewS: StackDepthOption>(s: NewS);
TracyLayer::<S, Z>::with_fields_in_zone_name<NewZ: FieldsInZoneNameOption>(s: NewZ);

impl StackDepthOption for u16 { /* dynamic */ }
impl StackDepthImpl for typenum::N0 { /* static */ }
impl StackDepthImpl for typenum::N1 { /* static */ }
// ...

impl FieldsInZoneNameOption for bool { /* dynamic */ }
struct True;
impl FieldsInZoneNameOption for True { /* static */ }
struct False;
impl FieldsInZoneNameOption for False { /* static */ }

etc. Or it could be a trait with methods. If those methods return constant values, I suspect the compiler will be able to figure how to inline them in.

@SUPERCILEX
Copy link
Contributor Author

Oooooh, that's quite clever! I went forward with that approach, but only differentiated between zero and non-zero u16s since the main optimizable difference for the stack depth param.

@nagisa
Copy link
Owner

nagisa commented Jan 6, 2024

Hm, yeah, I’m not sure this is worth it in this exact proposed shape. I’m going to cut a release shortly with all the changes so far and perhaps draft a counter proposal to this...

@SUPERCILEX
Copy link
Contributor Author

That works! I've been procrastinating on replying to #78, so I'll try and do that tonight. The main point is that I agree we should probably temporarily go back to not manual lifetimes until we figure out a proper fix.

@nagisa
Copy link
Owner

nagisa commented Jan 6, 2024

cf. #91

@SUPERCILEX
Copy link
Contributor Author

#91 is better than this as long as ConstConfig or something equivalent is provided out-of-the box.

@SUPERCILEX SUPERCILEX closed this Jan 6, 2024
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