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

Make InvariantDataProvider support AnyProvider #1494

Closed
1 task done
sffc opened this issue Jan 11, 2022 · 9 comments · Fixed by #2159
Closed
1 task done

Make InvariantDataProvider support AnyProvider #1494

sffc opened this issue Jan 11, 2022 · 9 comments · Fixed by #2159
Assignees
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) T-core Type: Required functionality

Comments

@sffc
Copy link
Member

sffc commented Jan 11, 2022

We should make the InvariantDataProvider support AnyProvider. The InvariantDataProvider is the one that returns Default impls for data structs.

Currently, Default returns a sized type and therefore requires that we box it in an Rc when putting it into an AnyProvider. We should be able to instead make InvariantDataProvider support the CrabBake-like style of AnyProvider, which is a static reference.

This would likely involve a trait that data structs implement which returns a &'static T. I propose that we use ConstDefault for this purpose. I tested and it works for our use case, assuming that we make all our objects const-constructible, which CrabBake requires anyway.

Needs approval from:

@sffc sffc added T-core Type: Required functionality C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) needs-approval One or more stakeholders need to approve proposal labels Jan 11, 2022
@Manishearth
Copy link
Member

Bit wary about the dependency, but this is a good idea. We can also use this as further impetus to zero-copy all of our types (note that non-zc types cannot ConstDefault)

@Manishearth
Copy link
Member

I think @iainireland had some concerns about CrabBake and stuff requiring all of our types to be zerocopy, which inhibits experimentation. I guess to solve that here we need to make it so that ConstDefaultProvider also only supports zero-copy structs?

@sffc
Copy link
Member Author

sffc commented Jan 11, 2022

Yeah, trait bounds take care of the experimentation use case. If you try to construct a ConstDefaultProvider with a data struct that doesn't implement the trait, your code won't compile.

@iainireland
Copy link
Contributor

My concern is about the learning curve of having a variety of different requirements, so overlapping requirements between this and crabbake are a good thing. It looks like we want zero-copy for many reasons, so I think we're priced in for that (and we're getting good value for the effort).

Are there any other likely obstacles to making all our objects const-constructible, beyond zero-copy?

@Manishearth
Copy link
Member

Manishearth commented Jan 11, 2022

Another slight concern is feature explosion. I think we need to merge up features and be careful about what we add. Or perhaps merge up the feature groups that we test.

@sffc
Copy link
Member Author

sffc commented Jan 11, 2022

Instead of using ConstDefault and adding that dependency, I think we should just introduce a trait in the data provider core crate like

trait UndData {
    const UND_DATA: Self;
}

And then we can rename "InvariantDataProvider" / "ConstDefaultProvider" to "UndProvider".

That trait should be implemented on data structs. If it is not implemented, then you won't be able to make a "UndProvider" for that particular data struct.

It is likely that Default impls will mostly look like this, which is fine:

impl Default for FooV1 {
    fn default() -> Self {
        Self::UND_DATA.clone()  // or zero-copy clone
    }
}

@Manishearth
Copy link
Member

This works for me and I think that's nicer: we will have a particular notion of "default" and it's not necessary that the "default" for a type used in, say, DecimalSymbolsV1 is the same as one in DateTimeSymbolsV1

@sffc sffc removed the needs-approval One or more stakeholders need to approve proposal label Jan 20, 2022
@sffc sffc added good first issue Good for newcomers help wanted Issue needs an assignee labels Apr 28, 2022
@sffc sffc added this to the ICU4X 0.6 milestone Apr 28, 2022
@sffc sffc modified the milestones: ICU4X 0.6, ICU4X 1.0 (Features) May 25, 2022
@sffc sffc added the discuss-priority Discuss at the next ICU4X meeting label Jun 9, 2022
@sffc
Copy link
Member Author

sffc commented Jun 17, 2022

  • @robertbastian - We'd be baking in data that could change across releases
  • @zbraniecki - It's last-resort fallback data. I think that's OK.
  • @sffc - What do we do about segmenter and properties data?
  • @robertbastian - You fall back to some databake data
  • @sffc - For properties, there's an issue about the struct having only one default impl, but multiple keys could point to it
  • @sffc - There are 2 use cases: bootstrapping, and last resort fallback
  • @robertbastian - The last resort fallback just results in incorrect behavior, especially if the structs don't contain the correct data, like for properties

Conclusion: Don't build a data provider around Default impls. Data structs can implement Default on a case-by-case basis and use AnyPayloadProvider to inject them.

@sffc sffc removed good first issue Good for newcomers help wanted Issue needs an assignee discuss-priority Discuss at the next ICU4X meeting labels Jun 17, 2022
@sffc sffc self-assigned this Jun 17, 2022
@sffc
Copy link
Member Author

sffc commented Jun 17, 2022

Action: Document this decision, and also make sure AnyPayloadProvider is understandable and discoverable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) T-core Type: Required functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants