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
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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.


[dependencies]
cfg-if = "0.1.2"
Expand Down
38 changes: 38 additions & 0 deletions benches/value.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#![cfg(feature = "kv_unstable")]
#![feature(test)]

extern crate test;
extern crate log;

use log::kv::Value;

#[bench]
fn u8_to_value(b: &mut test::Bencher) {
b.iter(|| {
Value::from(1u8)
})
}

#[bench]
fn u8_to_value_debug(b: &mut test::Bencher) {
b.iter(|| {
Value::from_debug(&1u8)
})
}

#[bench]
fn str_to_value_debug(b: &mut test::Bencher) {
b.iter(|| {
Value::from_debug(&"a string")
})
}

#[bench]
fn custom_to_value_debug(b: &mut test::Bencher) {
#[derive(Debug)]
struct A;

b.iter(|| {
Value::from_debug(&A)
})
}
9 changes: 9 additions & 0 deletions build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,21 @@

use std::env;

#[cfg(feature = "kv_unstable")]
#[path = "src/kv/value/internal/cast/into_primitive.rs"]
mod into_primitive;

fn main() {
let target = env::var("TARGET").unwrap();

if !target.starts_with("thumbv6") {
println!("cargo:rustc-cfg=atomic_cas");
}

#[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?


println!("cargo:rerun-if-changed=build.rs");
}
4 changes: 2 additions & 2 deletions src/kv/value/fill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use std::fmt;

use super::internal::{Erased, Inner, Visitor};
use super::internal::{Inner, Visitor};
use super::{Error, Value};

impl<'v> Value<'v> {
Expand All @@ -12,7 +12,7 @@ impl<'v> Value<'v> {
T: Fill + 'static,
{
Value {
inner: Inner::Fill(unsafe { Erased::new_unchecked::<T>(value) }),
inner: Inner::Fill(value),
}
}
}
Expand Down
66 changes: 33 additions & 33 deletions src/kv/value/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,6 @@ use std::fmt;

use super::{Primitive, ToValue, Value};

macro_rules! impl_into_owned {
($($into_ty:ty => $convert:ident,)*) => {
$(
impl ToValue for $into_ty {
fn to_value(&self) -> Value {
Value::from(*self)
}
}

impl<'v> From<$into_ty> for Value<'v> {
fn from(value: $into_ty) -> Self {
Value::from_primitive(value as $convert)
}
}
)*
};
}

impl<'v> ToValue for &'v str {
fn to_value(&self) -> Value {
Value::from(*self)
Expand Down Expand Up @@ -67,24 +49,42 @@ where
}
}

impl_into_owned! [
usize => u64,
u8 => u64,
u16 => u64,
u32 => u64,
u64 => u64,
macro_rules! impl_to_value_primitive {
($($into_ty:ty,)*) => {
$(
impl ToValue for $into_ty {
fn to_value(&self) -> Value {
Value::from(*self)
}
}

impl<'v> From<$into_ty> for Value<'v> {
fn from(value: $into_ty) -> Self {
Value::from_primitive(value)
}
}
)*
};
}

impl_to_value_primitive! [
usize,
u8,
u16,
u32,
u64,

isize => i64,
i8 => i64,
i16 => i64,
i32 => i64,
i64 => i64,
isize,
i8,
i16,
i32,
i64,

f32 => f64,
f64 => f64,
f32,
f64,

char => char,
bool => bool,
char,
bool,
];

#[cfg(feature = "std")]
Expand Down
159 changes: 159 additions & 0 deletions src/kv/value/internal/cast/into_primitive.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
#[cfg(all(src_build, feature = "kv_unstable_const_primitive"))]
pub(super) fn into_primitive<'v, T: 'static>(value: &'v T) -> Option<crate::kv::value::internal::Primitive<'v>> {
use std::any::TypeId;

use crate::kv::value::internal::Primitive;

trait ToPrimitive where Self: 'static {
KodrAus marked this conversation as resolved.
Show resolved Hide resolved
const CALL: fn(&Self) -> Option<Primitive> = {
const U8: TypeId = TypeId::of::<u8>();
const U16: TypeId = TypeId::of::<u16>();
const U32: TypeId = TypeId::of::<u32>();
const U64: TypeId = TypeId::of::<u64>();

const I8: TypeId = TypeId::of::<i8>();
const I16: TypeId = TypeId::of::<i16>();
const I32: TypeId = TypeId::of::<i32>();
const I64: TypeId = TypeId::of::<i64>();

const STR: TypeId = TypeId::of::<&'static str>();

match TypeId::of::<Self>() {
U8 => |v| Some(Primitive::from(unsafe { *(v as *const Self as *const u8) })),
U16 => |v| Some(Primitive::from(unsafe { *(v as *const Self as *const u16) })),
U32 => |v| Some(Primitive::from(unsafe { *(v as *const Self as *const u32) })),
U64 => |v| Some(Primitive::from(unsafe { *(v as *const Self as *const u64) })),

I8 => |v| Some(Primitive::from(unsafe { *(v as *const Self as *const i8) })),
I16 => |v| Some(Primitive::from(unsafe { *(v as *const Self as *const i16) })),
I32 => |v| Some(Primitive::from(unsafe { *(v as *const Self as *const i32) })),
I64 => |v| Some(Primitive::from(unsafe { *(v as *const Self as *const i64) })),

STR => |v| Some(Primitive::from(unsafe { *(v as *const Self as *const &'static str) })),

_ => |_| None,
}
};

fn to_primitive(&self) -> Option<Primitive> {
(Self::CALL)(self)
}
}

impl<T: 'static> ToPrimitive for T { }

value.to_primitive()
}

#[cfg(all(not(src_build), feature = "kv_unstable_const_primitive"))]
pub fn generate() { }

// 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.

// `ToPrimitive` trait that defaults to `None` except for these specific types
// It won't work with `&str` though in the `min_specialization` case
#[cfg(all(src_build, not(feature = "kv_unstable_const_primitive")))]
pub(in kv::value) fn into_primitive<'v>(value: &'v (dyn std::any::Any + 'static)) -> Option<crate::kv::value::internal::Primitive<'v>> {
// The set of type ids that map to primitives are generated at build-time
// by the contents of `sorted_type_ids.expr`. These type ids are pre-sorted
// so that they can be searched efficiently. See the `sorted_type_ids.expr.rs`
// file for the set of types that appear in this list
const TYPE_IDS: [(std::any::TypeId, for<'a> fn(&'a (dyn std::any::Any + 'static)) -> crate::kv::value::internal::Primitive<'a>); 30] = include!(concat!(env!("OUT_DIR"), "/into_primitive.rs"));

debug_assert!(TYPE_IDS.is_sorted_by_key(|&(k, _)| k));
if let Ok(i) = TYPE_IDS.binary_search_by_key(&value.type_id(), |&(k, _)| k) {
Some((TYPE_IDS[i].1)(value))
} else {
None
}
}

// When the `src_build` config is not set then we're in the build script
// This function will generate an expression fragment used by `into_primitive`
#[cfg(all(not(src_build), not(feature = "kv_unstable_const_primitive")))]
pub fn generate() {
use std::path::Path;
use std::{fs, env};

macro_rules! type_ids {
($($ty:ty,)*) => {
[
$(
(
std::any::TypeId::of::<$ty>(),
stringify!(
(
std::any::TypeId::of::<$ty>(),
(|value| unsafe {
debug_assert_eq!(value.type_id(), std::any::TypeId::of::<$ty>());

// SAFETY: We verify the value is $ty before casting
let value = *(value as *const dyn std::any::Any as *const $ty);
value.into()
}) as for<'a> fn(&'a (dyn std::any::Any + 'static)) -> crate::kv::value::internal::Primitive<'a>
)
)
),
)*
$(
(
std::any::TypeId::of::<Option<$ty>>(),
stringify!(
(
std::any::TypeId::of::<Option<$ty>>(),
(|value| unsafe {
debug_assert_eq!(value.type_id(), std::any::TypeId::of::<Option<$ty>>());

// SAFETY: We verify the value is Option<$ty> before casting
let value = *(value as *const dyn std::any::Any as *const Option<$ty>);
if let Some(value) = value {
value.into()
} else {
crate::kv::value::internal::Primitive::None
}
}) as for<'a> fn(&'a (dyn std::any::Any + 'static)) -> crate::kv::value::internal::Primitive<'a>
)
)
),
)*
]
};
}

let mut type_ids = type_ids![
usize,
u8,
u16,
u32,
u64,

isize,
i8,
i16,
i32,
i64,

f32,
f64,

char,
bool,

&str,
];

type_ids.sort_by_key(|&(k, _)| k);

let mut ordered_type_ids_expr = String::new();

ordered_type_ids_expr.push('[');

for (_, v) in &type_ids {
ordered_type_ids_expr.push_str(v);
ordered_type_ids_expr.push(',');
}

ordered_type_ids_expr.push(']');

let path = Path::new(&env::var_os("OUT_DIR").unwrap()).join("into_primitive.rs");
fs::write(path, ordered_type_ids_expr).unwrap();
}
Loading