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

Rework structured value casting #396

Merged
merged 24 commits into from
Aug 3, 2020
Merged

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented May 22, 2020

This is a spike to see if we can improve the way we cast values to concrete types. It does more work when capturing Debug and Display to retain the underlying primitive so that a new macro could reasonably default to creating values through these std traits while retaining numbers.

It does some wacky build script code generation to create a static sorted set of type ids we care about so they can be binary searched quickly.

build.rs Outdated
#[cfg(feature = "kv_unstable")]
into_primitive::generate();

println!("cargo:rustc-cfg=src_build");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m using this so the into_primitive module can tell if it’s in the build script or not. Is there a cfg that exists already for this?

src/lib.rs Outdated
@@ -276,6 +276,9 @@
#![cfg_attr(rustbuild, feature(staged_api, rustc_private))]
#![cfg_attr(rustbuild, unstable(feature = "rustc_private", issue = "27812"))]

#![feature(is_sorted)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems nice for debug assertions. I’ll look into where stabilisation is at.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/lib.rs Outdated
@@ -276,6 +276,9 @@
#![cfg_attr(rustbuild, feature(staged_api, rustc_private))]
#![cfg_attr(rustbuild, unstable(feature = "rustc_private", issue = "27812"))]

#![feature(is_sorted)]
#![feature(const_type_id)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we just stabilise these const methods on-demand. I can’t see why this one shouldn’t be stabilised, but will look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,109 @@
// NOTE: With specialization we could potentially avoid this call using a blanket
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specialization is possible because we use impl Trait instead of dyn Trait for inputs. This approach would still be necessary if we switched to dyn Trait. The trade-off would be that with dyn Trait we could generate less code in calling libraries at the cost of still requiring this runtime check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @hawkw sorry for the random ping but thought you might find this interesting for tracing 🙂

This is using a static set of type ids sorted at build time and binary searches them for interesting types given a generic T: Debug + ‘static input and treats the input as that concrete type. The runtime cost per argument is small (I measured ~9ns on my machine for a hit and ~5ns for a miss) but is not negligible. It could also use specialisation to remove that runtime cost, which I think might land as min_specialization sometime over the next year. Maybe you could consider it for something like the #[instrument] macro, to keep the nice compatible Debug bound, but also retain numbers and booleans as structured values?

Copy link

Choose a reason for hiding this comment

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

This is fascinating, thanks for pinging me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So...

If rust-lang/rust#72437 and rust-lang/rust#72488 are both merged then you’d be able to do this at compile time without specialization :)

There’s an example in the const_type_id PR.

Cargo.toml Outdated
@@ -47,6 +47,7 @@ std = []
# this will have a tighter MSRV before stabilization
kv_unstable = []
kv_unstable_sval = ["kv_unstable", "sval/fmt"]
kv_unstable_const_primitive = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just testing different strategies for destructuring.

src/kv/value/internal/cast/into_primitive.rs Outdated Show resolved Hide resolved
// Use consts to match a type with a conversion fn
// Pros: fast, will work on stable soon (possibly 1.45.0)
// Cons: requires a `'static` bound
#[cfg(all(src_build, feature = "kv_unstable_const_primitive"))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hawkw I’ve got a few different strategies for capturing the value from a generic T here now.

For tracing, if this is something you wanted to do you might end up needing to look more at the specialization one, because the others require a ’static bound in order to get the type id, which from what I can see isn’t compatible with how the #[instrument] macro currently works.

@KodrAus KodrAus marked this pull request as ready for review July 1, 2020 01:42
src/lib.rs Outdated
@@ -275,6 +275,8 @@
// an unstable crate
#![cfg_attr(rustbuild, feature(staged_api, rustc_private))]
#![cfg_attr(rustbuild, unstable(feature = "rustc_private", issue = "27812"))]
// TODO: Remove once https://github.com/rust-lang/rust/pull/72488 is merged
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll wait for rust-lang/rust#72488 before merging this PR so this unstable feature can be removed.

@KodrAus
Copy link
Contributor Author

KodrAus commented Jul 30, 2020

Once a new nightly goes out this should be good to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants