Databake improvements #2906

robertbastian · 2022-12-20T16:46:49Z

Removed litemap in favour of separate key and value arrays (Explore alternate top level data structures #2885):
- Fewer binary search monomorphizations: before, we had one per data struct (on &[(&str, &DataStruct)]), now there's only one, &[&str].
- Key arrays can be deduplicated (there are only ~20 for ~200 keys)
- Key lookup algorithm can be automatically optimized by datagen:
  - We still use binary search on a sorted array, but this can now easily be swapped for perfect hashing or AsciiTrie
  - If there is only one locale, we don't use strict_cmp, but eq with a macro-constructed locale
  - If the only locale is und we just check is_empty()
Smaller files for less rust-analyzer load
- One file per (deduplicated) data struct
DataProvider and AnyProvider implementations are now behind a macro, which means they can be added to foreign types
icu_testdata can now be built in bin-mode even if databake doesn't compile

.gitattributes

provider/testdata/data/baked/plurals/cardinal_v1/mod.rs

docs/tutorials/data_management.md

sffc · 2022-12-20T23:55:32Z

provider/datagen/Cargo.toml

@@ -84,6 +84,7 @@ zip = "0.6"
 cached-path = "0.5"
 reqwest = { version = "0.11", features = ["blocking"] }
 lazy_static = "1"
+rust-format = { version = "0.3.4", features = ["token_stream"] }


Praise: It's much better to do this at codegen time; I thought there was a reason we hadn't been doing that before? Is this a new crate?

We've been calling rustfmt directly. This crate is just a nicer interface and also supports prettyplease, but that doesn't look good.

sffc · 2022-12-21T00:00:12Z

provider/testdata/data/baked/calendar/japanese_v1/und.rs.data

@@ -0,0 +1,14 @@
+::icu_calendar::provider::JapaneseErasV1 {


Suggestion: You can use the proper path now (the one used for JSON): baked/calendar/japanese@v1/und.rs.data

I can't because the mod file with the lookup function is also in this module

sffc · 2022-12-21T00:16:52Z

provider/testdata/data/baked/collator/data_v1/bn.rs.data

+        },
+        192u32,
+    ),
+    ces: unsafe { ::zerovec::ZeroVec::from_bytes_unchecked(&[]) },


Observation: if the ZeroVec is empty, you don't need an unsafe constructor

provider/testdata/data/baked/collator/meta_v1/es+es-u-co-trad+tr.rs.data

...er/testdata/data/baked/compactdecimal/short_v1/ar+ar-EG+ar-EG-u-nu-latn+ar-u-nu-latn.rs.data

sffc · 2022-12-21T00:24:56Z

provider/testdata/data/baked/mod.rs

+        impl DataProvider<::icu_calendar::provider::JapaneseErasV1Marker> for $provider {
+            fn load(&self, req: DataRequest) -> Result<DataResponse<::icu_calendar::provider::JapaneseErasV1Marker>, DataError> {
+                calendar::japanese_v1::lookup(&req.locale)
+                    .map(zerofrom::ZeroFrom::zero_from)


Thought: Although ZeroFrom doesn't allocate (except for the skeleton hack), it's not completely free, and it shows up in code size benches. We may want to think about making DataPayload copy-on-write where it can retain a reference to the static data that it returns from .get() and only call ZeroFrom when mutation needs to occur (rarer case).

I'd love to but Yoke doesn't currently support this afaik. @Manishearth

It doesn't need to be in Yoke; I think it can be in DataPayload. (Not now; maybe for a future PR)

sffc · 2022-12-21T00:28:49Z

provider/testdata/data/baked/mod.rs

+                const NARROWDAYRELATIVETIMEFORMATDATAV1MARKER: ::icu_provider::DataKeyHash =
+                    ::icu_relativetime::provider::NarrowDayRelativeTimeFormatDataV1Marker::KEY.hashed();
+                #[cfg(feature = "icu_relativetime")]
+                const NARROWHO


Suggestion: We don't really need to call this by default, either. It's kind-of elegant if the file defines two macros for you and nothing else.

It's a breaking change to not expose BakedDataProvider anymore.

It's also a breaking change that any.rs doesn't work anymore

True. Are we okay with this breakage? Othewise any.rs could contain the macro invocation.

How about if we add back any.rs with a single line to invoke the macro and then make a 2.0-breaking issue to clean this up and also change mod.rs to not invoke the normal macro

Manishearth · 2022-12-21T20:04:23Z

provider/datagen/src/databake.rs

+                true,
+            )?;
+            let file_name = format!("{file_name}.rs.data");
+            let statik = quote! { static #ident: DataStruct = include!(#file_name); };


suggestion: worth adding a comment to each line saying // used for locale "foo" and "bar" or something

I think the combined names are a better solution but Shane didn't like that.

I wouldn't mind each file having comments stating which locales it is used for, or comments in the mod.rs file. I think it's not really necessary though.

improvements

079e702

robertbastian requested review from a team, sffc and Manishearth as code owners December 20, 2022 16:46

robertbastian added 2 commits December 20, 2022 17:58

fix

29a79bf

fix

13b7a01

Manishearth reviewed Dec 20, 2022

View reviewed changes

.gitattributes Show resolved Hide resolved

Manishearth reviewed Dec 20, 2022

View reviewed changes

provider/testdata/data/baked/plurals/cardinal_v1/mod.rs Outdated Show resolved Hide resolved

sffc previously approved these changes Dec 21, 2022

View reviewed changes

comments

6843942

robertbastian dismissed sffc’s stale review via 6843942 December 21, 2022 12:11

robertbastian requested review from filmil, zbraniecki, gregtatum and nordzilla as code owners December 21, 2022 12:11

fix

d5eb079

robertbastian requested review from sffc and Manishearth and removed request for filmil, zbraniecki, gregtatum and nordzilla December 21, 2022 12:59

doc

a805b7f

sffc previously approved these changes Dec 21, 2022

View reviewed changes

non-breaking

37f2334

robertbastian dismissed sffc’s stale review via 37f2334 December 21, 2022 16:46

robertbastian requested a review from sffc December 21, 2022 16:46

sffc previously approved these changes Dec 21, 2022

View reviewed changes

Merge branch 'main' into bake

af3d99a

robertbastian dismissed sffc’s stale review via af3d99a December 21, 2022 19:46

sffc approved these changes Dec 21, 2022

View reviewed changes

Manishearth approved these changes Dec 21, 2022

View reviewed changes

robertbastian merged commit b74aafd into unicode-org:main Dec 21, 2022

robertbastian deleted the bake branch December 22, 2022 17:28

This was referenced Jan 23, 2023

1.1.0 #3018

Merged

Make DataResponse constructible from &'static M::Yokeable #3020

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Databake improvements #2906

Databake improvements #2906

robertbastian commented Dec 20, 2022 •

edited

Loading

sffc Dec 20, 2022

robertbastian Dec 21, 2022

sffc Dec 21, 2022

robertbastian Dec 21, 2022

sffc Dec 21, 2022

sffc Dec 21, 2022

robertbastian Dec 21, 2022

sffc Dec 21, 2022

sffc Dec 21, 2022

robertbastian Dec 21, 2022

robertbastian Dec 21, 2022

sffc Dec 21, 2022

robertbastian Dec 21, 2022

sffc Dec 21, 2022 •

edited

Loading

Manishearth Dec 21, 2022

robertbastian Dec 21, 2022

sffc Dec 21, 2022

Databake improvements #2906

Databake improvements #2906

Conversation

robertbastian commented Dec 20, 2022 • 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

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 Dec 21, 2022 • 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

robertbastian commented Dec 20, 2022 •

edited

Loading

sffc Dec 21, 2022 •

edited

Loading