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

robertbastian · 2023-05-31T20:05:43Z

Part of #3020

This adds M::Yokeable: ZeroFrom<'static, M::Yokeable> bounds to DataPayload::map_project and DataPayload::try_map_project. This is a breaking change, however ZeroFrom is implemented for all data structs (through #[data_struct] or convention).

Manishearth

approach seems close to what we discussed.

Am hoping we can potentially reduce this down to Yoke<&'static M, C> with some tricks. I think it's possible by making the cart type more complex, and having with_mut() extract the cart object

Manishearth · 2023-06-01T05:24:39Z

provider/core/src/response.rs

+pub struct DataPayload<M: DataMarker>(pub(crate) DataPayloadInner<M>);
+
+pub(crate) enum DataPayloadInner<M: DataMarker> {
+    Yoke(Yoke<M::Yokeable, Option<Cart>>),


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

though this does become easy to cfg

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.

Manishearth · 2023-06-01T05:36:11Z

provider/core/src/response.rs

+        DataPayload(match self.0 {
+            DataPayloadInner::Yoke(yoke) => DataPayloadInner::Yoke(yoke.map_project(f)),
+            DataPayloadInner::Ref(r) => {
+                DataPayloadInner::Yoke(Yoke::new_owned(r.clone()).map_project(f))


issue: this does have a cost. I'm not sure how we can avoid this cost. However, there are a bunch of cases in the code where we don't need owned mapping: ought we introduce map_project_ref() that does not require cloning carts, but does operate on references?

I guess there are a few places in datetime where we map_project in runtime code, so I agree it would be nice if we didn't need to downgrade to ZeroFrom, but I'm not sure if that's avoidable in the general case?

It's not avoidable in the general case, it is avoidable in DateTime.

Unfortunately it's not avoidable in the map property FFI code without significant changes :/ But we might be able to figure out a different way to type-erase there

Currently we pay this cost on every DataPayload construction. With this PR, we only pay the cost when we map_project, so this is a strict improvement.

map_project_cloned() already operates on references.

Yes, but it also clones the cart, which isn't great.

Something that operates on self, uses &Y and doesn't clone the cart is the missing niche here

sffc

I only looked at the files in provider/core/src because I think the rest is from another PR

sffc · 2023-06-01T08:04:06Z

provider/core/src/response.rs

+pub struct DataPayload<M: DataMarker>(pub(crate) DataPayloadInner<M>);
+
+pub(crate) enum DataPayloadInner<M: DataMarker> {
+    Yoke(Yoke<M::Yokeable, Option<Cart>>),


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.

provider/core/src/response.rs

sffc · 2023-06-01T08:10:29Z

provider/core/src/response.rs

+            DataPayloadInner::Ref(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


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

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.

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.

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.

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

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.

provider/core/src/response.rs

robertbastian · 2023-06-01T09:50:02Z

This PR starts at the ref commit.

sffc

LGTM for the changes not in the diffbase

utils/zerofrom/src/zero_from.rs

provider/core/src/response.rs

robertbastian · 2023-06-02T07:58:52Z

provider/core/src/response.rs

+        Self(DataPayloadInner::Yoke(Yoke::new_owned(data)))
+    }
+
+    #[doc(hidden)]


Should this be public?

sffc · 2023-06-02T20:29:00Z

Discussed with @robertbastian: this PR is not a strict improvement. There are pros and cons:

Pro: Makes BakedDataProvider faster, which shows up in a 5-15% improvement in the end-to-end datetime formatting benches
Pro: Allows for const construction of DataPayload from a baked data provider
Con: Some loss of performance of the postcard path
Con: Some increase of code size, whether or not baked or postcard is used
Note: We can't disable the Yoke variant across the board so long as there is library code using map_project

Manishearth · 2023-06-03T00:23:02Z

I think the way to do this is probably Yoke<&'static Y, C> where the cart type is an enum between Box<Y> and the Rc/None variants. Projection turns things into a box variant

Manishearth · 2023-06-03T00:23:27Z

I think the way to do this is probably Yoke<&'static Y, C> where the cart type is an enum between Box<Y> and the Rc/None variants. Projection turns things into a box variant if necessary

robertbastian · 2023-06-05T08:26:50Z

It would have to be Box<(Y, Cart)>, right?

Manishearth · 2023-06-05T12:18:52Z

Hmm. Yeah maybe, depends on what we want to achieve.

sffc · 2023-06-05T18:17:35Z

If we can get map_project to work and keep the DataPayload in the StaticRef state, then we can actually disable the Yoke variant for non-postcard payloads, which I think was part of the motivation behind the issue. Basically it just means that DataPayload::map_project needs to be more restrictive than Yoke::map_project. You can return any field of the input, but you can't construct a new object and return it: you can return &T but not T.

The call sites of map_project in the datetime crate really take full advantage of the flexibility of map_project and use it to construct custom objects, though.

I have long thought that we need to rethink the data model of datetime formatters, and I made a comment recently that we should just resolve patterns into SmallVec<PatternItem> instead of doing Yoke gymnastics. This could be an impetus to make that change.

Manishearth · 2023-06-06T03:09:02Z

Yeah though we also map_project() to type-erase CodePointTries in map property FFI.

But yes, that's what I was saying when I was talking about a map_project that still operates on references

sffc · 2023-06-06T06:19:30Z

Another thing I was thinking was that users who really want to map_project to a new type should use a new type like DataPayloadAlwaysYoke, or just Yoke directly. It doesn't solve the problem for datetime since those structs have really big stack size, but it works great for properties where the stack is not very big.

robertbastian · 2023-06-06T17:16:39Z

I've added the databake AnyProvider simplification that this unlocks.

sffc

I'm satisfied with the short-term and medium-term work that this change unlocks.

robertbastian · 2023-06-06T23:41:12Z

I removed the datagen changes to keep this PR focused.

Manishearth · 2023-06-07T05:42:24Z

provider/core/src/response.rs

+        DataPayload(DataPayloadInner::Yoke(
+            match self.0 {
+                DataPayloadInner::Yoke(yoke) => yoke,
+                DataPayloadInner::StaticRef(r) => Yoke::new_owned(zerofrom::ZeroFrom::zero_from(r)),


ah, this isn't expensive anymore, neat

Manishearth · 2023-06-07T06:20:40Z

provider/core/src/response.rs

+            DataPayloadInner::Ref(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


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.

robertbastian requested review from sffc, Manishearth and a team as code owners May 31, 2023 20:05

robertbastian force-pushed the ref branch from 66f2efa to 78c623d Compare May 31, 2023 20:11

This comment was marked as spam.

Sign in to view

robertbastian force-pushed the ref branch from 78c623d to 947c67e Compare May 31, 2023 20:46

robertbastian requested review from zbraniecki, gregtatum and nordzilla as code owners May 31, 2023 20:46

This comment was marked as spam.

Sign in to view

robertbastian removed request for zbraniecki, gregtatum and nordzilla May 31, 2023 21:28

Manishearth reviewed Jun 1, 2023

View reviewed changes

sffc reviewed Jun 1, 2023

View reviewed changes

robertbastian requested review from sffc and Manishearth June 1, 2023 09:49

sffc reviewed Jun 2, 2023

View reviewed changes

utils/zerofrom/src/zero_from.rs Show resolved Hide resolved

provider/core/src/response.rs Outdated Show resolved Hide resolved

provider/core/src/response.rs Outdated Show resolved Hide resolved

robertbastian added 7 commits June 2, 2023 09:54

ref

b200ea5

sizes

3cd7c5b

sizes

fc9635f

simplify

3833d6c

comments

4e95e9e

const

229d7b7

fmt

64c8173

robertbastian force-pushed the ref branch from 818907f to 64c8173 Compare June 2, 2023 07:54

This comment was marked as spam.

Sign in to view

robertbastian commented Jun 2, 2023

View reviewed changes

robertbastian requested a review from sffc June 2, 2023 08:24

robertbastian mentioned this pull request Jun 2, 2023

Baked data for icu_plurals, icu_timezone #3462

Merged

robertbastian added the discuss Discuss at a future ICU4X-SC meeting label Jun 5, 2023

sffc previously approved these changes Jun 6, 2023

View reviewed changes

robertbastian dismissed sffc’s stale review via 64c8173 June 6, 2023 23:37

robertbastian force-pushed the ref branch from 21b6918 to 64c8173 Compare June 6, 2023 23:37

reset datagen

810fab7

robertbastian requested a review from sffc June 6, 2023 23:41

sffc approved these changes Jun 7, 2023

View reviewed changes

Manishearth approved these changes Jun 7, 2023

View reviewed changes

robertbastian merged commit a576f41 into unicode-org:main Jun 7, 2023

robertbastian deleted the ref branch June 7, 2023 08:36

sffc mentioned this pull request Jun 8, 2023

Add Yokeable::upgrade_output function #3507

Open

Manishearth mentioned this pull request Sep 21, 2023

1.3 utils release list #4066

Closed

13 tasks

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

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

Conversation

robertbastian commented May 31, 2023 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

Manishearth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian commented Jun 1, 2023

sffc left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

sffc commented Jun 2, 2023

Manishearth commented Jun 3, 2023

Manishearth commented Jun 3, 2023

robertbastian commented Jun 5, 2023

Manishearth commented Jun 5, 2023

sffc commented Jun 5, 2023 • edited Loading

Manishearth commented Jun 6, 2023

sffc commented Jun 6, 2023

robertbastian commented Jun 6, 2023

sffc left a comment

Choose a reason for hiding this comment

robertbastian commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

robertbastian commented May 31, 2023 •

edited

Loading

Manishearth left a comment •

edited

Loading

sffc commented Jun 5, 2023 •

edited

Loading