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.

Sign up for GitHub

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

Jump to bottom

Make DataPayload constructible from &'static M::Yokeable #3467

Merged
merged 8 commits into from
Jun 7, 2023
Merged

Make DataPayload constructible from &'static M::Yokeable #3467

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b200ea5
ref
robertbastian May 31, 2023
3cd7c5b
sizes
robertbastian May 31, 2023
fc9635f
sizes
robertbastian May 31, 2023
3833d6c
simplify
robertbastian May 31, 2023
4e95e9e
comments
robertbastian Jun 1, 2023
229d7b7
const
robertbastian Jun 1, 2023
64c8173
fmt
robertbastian Jun 1, 2023
810fab7
reset datagen
robertbastian Jun 6, 2023
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
24 changes: 12 additions & 12 deletions components/datetime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,12 @@ mod tests {

#[test]
fn check_sizes() {
check_size_of!(5784 | 4576, DateFormatter);
check_size_of!(6784 | 5448, DateTimeFormatter);
check_size_of!(7896 | 6464, ZonedDateTimeFormatter);
check_size_of!(5800 | 4592, DateFormatter);
check_size_of!(6792 | 5456, DateTimeFormatter);
check_size_of!(7904 | 6480, ZonedDateTimeFormatter);
check_size_of!(1496 | 1320, TimeFormatter);
check_size_of!(1112 | 1016, TimeZoneFormatter);
check_size_of!(5744 | 4536, TypedDateFormatter::<Gregorian>);
check_size_of!(1112 | 1024, TimeZoneFormatter);
check_size_of!(5752 | 4544, TypedDateFormatter::<Gregorian>);
check_size_of!(6744 | 5408, TypedDateTimeFormatter::<Gregorian>);

check_size_of!(80, DateTimeError);
Expand All @@ -224,19 +224,19 @@ mod tests {
type DP<M> = DataPayload<M>;
check_size_of!(208, DP::<PatternPluralsFromPatternsV1Marker>);
check_size_of!(1032 | 904, DP::<TimeSymbolsV1Marker>);
check_size_of!(32, DP::<GenericPatternV1Marker>);
check_size_of!(40, DP::<GenericPatternV1Marker>);
check_size_of!(208, DP::<PatternPluralsFromPatternsV1Marker>);
check_size_of!(5064 | 3904, DP::<ErasedDateSymbolsV1Marker>);
check_size_of!(16, DP::<WeekDataV1Marker>);
check_size_of!(288 | 224, DP::<TimeZoneFormatsV1Marker>);
check_size_of!(288 | 232, DP::<TimeZoneFormatsV1Marker>);
check_size_of!(64 | 56, DP::<ExemplarCitiesV1Marker>);
check_size_of!(120 | 104, DP::<MetazoneGenericNamesLongV1Marker>);
check_size_of!(120 | 104, DP::<MetazoneGenericNamesShortV1Marker>);
check_size_of!(216 | 200, DP::<MetazoneSpecificNamesLongV1Marker>);
check_size_of!(216 | 200, DP::<MetazoneSpecificNamesShortV1Marker>);
check_size_of!(120 | 112, DP::<MetazoneGenericNamesLongV1Marker>);
check_size_of!(120 | 112, DP::<MetazoneGenericNamesShortV1Marker>);
check_size_of!(216 | 208, DP::<MetazoneSpecificNamesLongV1Marker>);
check_size_of!(216 | 208, DP::<MetazoneSpecificNamesShortV1Marker>);
check_size_of!(168, PluralRules);
check_size_of!(256 | 208, FixedDecimalFormatter);
check_size_of!(1024 | 928, TimeZoneDataPayloads);
check_size_of!(1024 | 936, TimeZoneDataPayloads);
check_size_of!(3, TimeZoneFormatterUnit);
}
}
11 changes: 5 additions & 6 deletions provider/core/src/datagen/payload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,17 @@ use alloc::boxed::Box;
use databake::{Bake, CrateEnv, TokenStream};
use yoke::*;

trait ExportableYoke {
trait ExportableDataPayload {
fn bake_yoke(&self, env: &CrateEnv) -> TokenStream;
fn serialize_yoke(
&self,
serializer: &mut dyn erased_serde::Serializer,
) -> Result<(), DataError>;
}

impl<Y, C> ExportableYoke for Yoke<Y, C>
impl<M: DataMarker> ExportableDataPayload for DataPayload<M>
where
Y: for<'a> Yokeable<'a>,
for<'a> <Y as Yokeable<'a>>::Output: Bake + serde::Serialize,
for<'a> <M::Yokeable as Yokeable<'a>>::Output: Bake + serde::Serialize,
{
fn bake_yoke(&self, ctx: &CrateEnv) -> TokenStream {
self.get().bake(ctx)
Expand All @@ -40,7 +39,7 @@ where
#[doc(hidden)] // exposed for make_exportable_provider
#[derive(yoke::Yokeable)]
pub struct ExportBox {
payload: Box<dyn ExportableYoke + Sync>,
payload: Box<dyn ExportableDataPayload + Sync>,
}

impl core::fmt::Debug for ExportBox {
Expand All @@ -59,7 +58,7 @@ where
{
fn upcast(other: DataPayload<M>) -> DataPayload<ExportMarker> {
DataPayload::from_owned(ExportBox {
payload: Box::new(other.yoke),
payload: Box::new(other),
})
}
}
Expand Down
109 changes: 75 additions & 34 deletions provider/core/src/response.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,11 @@ pub struct DataResponseMetadata {
///
/// assert_eq!("Demo", payload.get().message);
/// ```
pub struct DataPayload<M>
where
M: DataMarker,
{
pub(crate) yoke: Yoke<M::Yokeable, Option<Cart>>,
pub struct DataPayload<M: DataMarker>(pub(crate) DataPayloadInner<M>);

pub(crate) enum DataPayloadInner<M: DataMarker> {
Yoke(Yoke<M::Yokeable, Option<Cart>>),
Copy link
Member

Choose a reason for hiding this comment

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

issue: worried about stack size increase, and worried about runtime cost

Admittedly I do not know if there is a way around this, I just want to register the concern. It may be insurmountable and we can decide to do this anyway

Copy link
Member

Choose a reason for hiding this comment

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

though this does become easy to cfg

Copy link
Member

Choose a reason for hiding this comment

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

Runtime cost: solved by using as_borrowed, and the fact that we don't need to ZeroFrom everything.

Stack size: it seems to be, at worst, one extra word per payload. Maybe if we make Yoke have better niches, we could get this to be stack-neutral.

StaticRef(&'static M::Yokeable),
}

/// The type of the "cart" that is used by `DataPayload`.
Expand Down Expand Up @@ -136,9 +136,10 @@ where
for<'a> YokeTraitHack<<M::Yokeable as Yokeable<'a>>::Output>: Clone,
{
fn clone(&self) -> Self {
Self {
yoke: self.yoke.clone(),
}
Self(match &self.0 {
DataPayloadInner::Yoke(yoke) => DataPayloadInner::Yoke(yoke.clone()),
DataPayloadInner::StaticRef(r) => DataPayloadInner::StaticRef(*r),
})
}
}

Expand Down Expand Up @@ -193,17 +194,23 @@ where
/// ```
#[inline]
pub fn from_owned(data: M::Yokeable) -> Self {
Self {
yoke: Yoke::new_owned(data),
}
Self(DataPayloadInner::Yoke(Yoke::new_owned(data)))
}

#[doc(hidden)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be public?

#[inline]
pub const fn from_static_ref(data: &'static M::Yokeable) -> Self {
Self(DataPayloadInner::StaticRef(data))
}

/// Convert a DataPayload that was created via [`DataPayload::from_owned()`] back into the
/// concrete type used to construct it.
pub fn try_unwrap_owned(self) -> Result<M::Yokeable, DataError> {
self.yoke
.try_into_yokeable()
.map_err(|_| DataErrorKind::InvalidState.with_str_context("try_unwrap_owned"))
match self.0 {
DataPayloadInner::Yoke(yoke) => yoke.try_into_yokeable().ok(),
DataPayloadInner::StaticRef(_) => None,
}
.ok_or(DataErrorKind::InvalidState.with_str_context("try_unwrap_owned"))
}

/// Mutate the data contained in this DataPayload.
Expand Down Expand Up @@ -244,8 +251,15 @@ where
pub fn with_mut<'a, F>(&'a mut self, f: F)
where
F: 'static + for<'b> FnOnce(&'b mut <M::Yokeable as Yokeable<'a>>::Output),
M::Yokeable: zerofrom::ZeroFrom<'static, M::Yokeable>,
{
self.yoke.with_mut(f)
if let DataPayloadInner::StaticRef(r) = self.0 {
self.0 = DataPayloadInner::Yoke(Yoke::new_owned(zerofrom::ZeroFrom::zero_from(r)));
}
match &mut self.0 {
DataPayloadInner::Yoke(yoke) => yoke.with_mut(f),
_ => unreachable!(),
}
}

/// Borrows the underlying data.
Expand All @@ -266,7 +280,10 @@ where
#[inline]
#[allow(clippy::needless_lifetimes)]
pub fn get<'a>(&'a self) -> &'a <M::Yokeable as Yokeable<'a>>::Output {
self.yoke.get()
match &self.0 {
DataPayloadInner::Yoke(yoke) => yoke.get(),
DataPayloadInner::StaticRef(r) => Yokeable::transform(*r),
}
}

/// Maps `DataPayload<M>` to `DataPayload<M2>` by projecting it with [`Yoke::map_project`].
Expand Down Expand Up @@ -318,10 +335,15 @@ where
<M::Yokeable as Yokeable<'a>>::Output,
PhantomData<&'a ()>,
) -> <M2::Yokeable as Yokeable<'a>>::Output,
M::Yokeable: zerofrom::ZeroFrom<'static, M::Yokeable>,
{
DataPayload {
yoke: self.yoke.map_project(f),
}
DataPayload(DataPayloadInner::Yoke(
match self.0 {
DataPayloadInner::Yoke(yoke) => yoke,
DataPayloadInner::StaticRef(r) => Yoke::new_owned(zerofrom::ZeroFrom::zero_from(r)),
Copy link
Member

Choose a reason for hiding this comment

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

ah, this isn't expensive anymore, neat

}
.map_project(f),
))
}

/// Version of [`DataPayload::map_project()`] that borrows `self` instead of moving `self`.
Expand Down Expand Up @@ -362,9 +384,16 @@ where
PhantomData<&'a ()>,
) -> <M2::Yokeable as Yokeable<'a>>::Output,
{
DataPayload {
yoke: self.yoke.map_project_cloned(f),
}
DataPayload(DataPayloadInner::Yoke(match &self.0 {
DataPayloadInner::Yoke(yoke) => yoke.map_project_cloned(f),
DataPayloadInner::StaticRef(r) => {
let output: <M2::Yokeable as Yokeable<'static>>::Output =
f(Yokeable::transform(*r), PhantomData);
// Safety: <M2::Yokeable as Yokeable<'static>>::Output is the same type as M2::Yokeable
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: just downgrade the thing to a Yoke via ZeroFrom instead of doing these unsafe gymnastics

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? f operates on a reference so we can use that to avoid doing the expensive ZeroFrom which we're trying to avoid with this PR. It's not gymnastics if it's correct.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, well there's still necessarily a Clone or ZeroFrom happening inside of the f function, but the advantage is that in map_project_cloned we only need to do that on a subset of fields instead of the whole thing.

I think the safety comment is not quite right; the invariant is "The returned value must be destroyed before the data from was borrowing from is." The fact that you're setting 'a to 'static on line 390 is important to the safety of the borrowed data.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think "The returned value must be destroyed before the data from was borrowing from is." is irrelevant. The point here is that the types are the same but the compiler doesn't know, I could also use a transmute.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I can't use a transmute because the compiler doesn't know they're the same size

Copy link
Member

Choose a reason for hiding this comment

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

I would ideally like to avoid having any unsafety here.

I think all we need here is fn upgrade_output<Y: Yokeable<'static>>(output: Y::Output) -> Y, which internally calls Yokeable::make(). Does that make sense? Yokeable::make() is only scarily unsafe when the lifetimes involved aren't 'static.

let yokeable: M2::Yokeable = unsafe { M2::Yokeable::make(output) };
Yoke::new_owned(yokeable)
}
}))
}

/// Version of [`DataPayload::map_project()`] that bubbles up an error from `f`.
Expand Down Expand Up @@ -411,10 +440,15 @@ where
<M::Yokeable as Yokeable<'a>>::Output,
PhantomData<&'a ()>,
) -> Result<<M2::Yokeable as Yokeable<'a>>::Output, E>,
M::Yokeable: zerofrom::ZeroFrom<'static, M::Yokeable>,
{
Ok(DataPayload {
yoke: self.yoke.try_map_project(f)?,
})
Ok(DataPayload(DataPayloadInner::Yoke(
match self.0 {
DataPayloadInner::Yoke(yoke) => yoke,
DataPayloadInner::StaticRef(r) => Yoke::new_owned(zerofrom::ZeroFrom::zero_from(r)),
}
.try_map_project(f)?,
)))
}

/// Version of [`DataPayload::map_project_cloned()`] that bubbles up an error from `f`.
Expand Down Expand Up @@ -465,9 +499,15 @@ where
PhantomData<&'a ()>,
) -> Result<<M2::Yokeable as Yokeable<'a>>::Output, E>,
{
Ok(DataPayload {
yoke: self.yoke.try_map_project_cloned(f)?,
})
Ok(DataPayload(DataPayloadInner::Yoke(match &self.0 {
DataPayloadInner::Yoke(yoke) => yoke.try_map_project_cloned(f)?,
DataPayloadInner::StaticRef(r) => {
let output: <M2::Yokeable as Yokeable<'static>>::Output =
f(Yokeable::transform(*r), PhantomData)?;
// Safety: <M2::Yokeable as Yokeable<'static>>::Output is the same type as M2::Yokeable
sffc marked this conversation as resolved.
Show resolved Hide resolved
Yoke::new_owned(unsafe { M2::Yokeable::make(output) })
}
})))
}

/// Convert between two [`DataMarker`] types that are compatible with each other.
Expand Down Expand Up @@ -497,7 +537,10 @@ where
where
M2: DataMarker<Yokeable = M::Yokeable>,
{
DataPayload { yoke: self.yoke }
DataPayload(match self.0 {
DataPayloadInner::Yoke(yoke) => DataPayloadInner::Yoke(yoke),
DataPayloadInner::StaticRef(r) => DataPayloadInner::StaticRef(r),
})
}
}

Expand All @@ -507,19 +550,17 @@ impl DataPayload<BufferMarker> {
let yoke = Yoke::attach_to_cart(SelectedRc::new(buffer), |b| &**b);
// Safe because cart is wrapped
let yoke = unsafe { yoke.replace_cart(|b| Some(Cart(b))) };
Self { yoke }
Self(DataPayloadInner::Yoke(yoke))
}

/// Converts a yoked byte buffer into a `DataPayload<BufferMarker>`.
pub fn from_yoked_buffer(yoke: Yoke<&'static [u8], Option<Cart>>) -> Self {
Self { yoke }
Self(DataPayloadInner::Yoke(yoke))
}

/// Converts a static byte buffer into a `DataPayload<BufferMarker>`.
pub fn from_static_buffer(buffer: &'static [u8]) -> Self {
Self {
yoke: Yoke::new_owned(buffer),
}
Self(DataPayloadInner::Yoke(Yoke::new_owned(buffer)))
}
}

Expand Down
22 changes: 19 additions & 3 deletions provider/datagen/src/baked_exporter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ struct ImplData {
feature: SyncTokenStream,
macro_ident: SyncTokenStream,
hash_ident: SyncTokenStream,
// These are required for the skeletons special case
into_data_payload: SyncTokenStream,
into_any_payload: SyncTokenStream,
}

Expand Down Expand Up @@ -399,10 +401,20 @@ impl DataExporter for BakedExporter {
}
};

let into_any_payload = if is_datetime_skeletons {
let into_data_payload = if is_datetime_skeletons {
quote! {
.map(icu_provider::prelude::zerofrom::ZeroFrom::zero_from)
.map(icu_provider::DataPayload::<#marker>::from_owned)
}
} else {
quote! {
.map(icu_provider::DataPayload::from_static_ref)
}
};

let into_any_payload = if is_datetime_skeletons {
quote! {
#into_data_payload
.map(icu_provider::DataPayload::wrap_into_any_payload)
}
} else {
Expand All @@ -418,6 +430,7 @@ impl DataExporter for BakedExporter {
singleton: singleton.map(|t| t.to_string()),
macro_ident: format!("impl_{ident}"),
hash_ident: ident.to_ascii_uppercase(),
into_data_payload: into_data_payload.to_string(),
into_any_payload: into_any_payload.to_string(),
};

Expand Down Expand Up @@ -479,6 +492,10 @@ impl DataExporter for BakedExporter {
.values()
.map(|data| data.hash_ident.parse::<TokenStream>().unwrap())
.collect::<Vec<_>>();
let into_data_payloads = data
.values()
.map(|data| data.into_data_payload.parse::<TokenStream>().unwrap())
.collect::<Vec<_>>();
let into_any_payloads = data
.values()
.map(|data| data.into_any_payload.parse::<TokenStream>().unwrap())
Expand Down Expand Up @@ -550,8 +567,7 @@ impl DataExporter for BakedExporter {
req: icu_provider::DataRequest,
) -> Result<icu_provider::DataResponse<#markers>, icu_provider::DataError> {
#lookups
.map(icu_provider::prelude::zerofrom::ZeroFrom::zero_from)
.map(icu_provider::DataPayload::from_owned)
#into_data_payloads
.map(|payload| {
icu_provider::DataResponse {
metadata: Default::default(),
Expand Down
2 changes: 1 addition & 1 deletion provider/testdata/data/baked/macros.rs
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions utils/zerofrom/src/zero_from.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,9 @@ impl<'zf, B: ToOwned + ?Sized> ZeroFrom<'zf, Cow<'_, B>> for Cow<'zf, B> {
}
}

impl<'zf> ZeroFrom<'zf, &'_ str> for &'zf str {
impl<'zf, T: ?Sized> ZeroFrom<'zf, &'_ T> for &'zf T {
robertbastian marked this conversation as resolved.
Show resolved Hide resolved
#[inline]
fn zero_from(other: &'zf &'_ str) -> &'zf str {
fn zero_from(other: &'zf &'_ T) -> &'zf T {
other
}
}
Expand Down