Store Unicode keywords in ResourceOptions #1750

sffc · 2022-03-29T05:57:00Z

Part of #1109

This PR changes ResourceOptions to have private fields and store a Unicode Keywords object instead of a string. It centralizes the ResourceOptions constructors and leaves TODOs pointing to #1109.

This reverts commit fe2370c.

sffc · 2022-03-31T06:29:42Z

At first I tried using a straight Locale in the ResourceOptions. It increases binary size a bit. Old:

-rwxr-xr-x 1 runner docker    41784 Mar 29 05:17 optim4.elf
-rwxr-xr-x 1 runner docker    31864 Mar 29 05:17 optim5.elf

New:

-rwxr-xr-x 1 sffc primarygroup    46416 Mar 28 22:52 optim4.elf
-rwxr-xr-x 1 sffc primarygroup    35936 Mar 28 22:53 optim5.elf

I got the size regression down to something I am okay with:

-rwxr-xr-x 1 sffc primarygroup    42496 Mar 30 22:37 optim4.elf
-rwxr-xr-x 1 sffc primarygroup    32448 Mar 30 22:37 optim5.elf

I did this by storing LanguageIdentifier alongside Keywords, which are the two sections of Locale that we care about. This is nice in part because we can easily reconstruct the Locale if we need it.

Looking at the assembly, I think the sizes will go down further once we're using cmp_bytes (blocked on #1756).

Update 2022-04-12: The size on main is now

-rwxr-xr-x 1 runner docker    42264 Apr 12 22:05 optim4.elf
-rwxr-xr-x 1 runner docker    31640 Apr 12 22:05 optim5.elf

and this PR changes it to

-rwxr-xr-x 1 runner docker    43216 Apr 13 03:44 optim4.elf
-rwxr-xr-x 1 runner docker    32328 Apr 13 03:44 optim5.elf

zbraniecki

ah, right. Thank you! lgtm

Manishearth · 2022-04-13T16:54:58Z

@sffc What's the best way to review this? (Also, I haven't started yet, so if you're able to squash some of the smaller related commits to make it easier to review, would be appreciated)

robertbastian

lgtm overall, just some small things

components/datetime/src/calendar.rs

provider/core/src/resource.rs

provider/blob/src/blob_data_provider.rs

provider/datagen/src/cldr/transform/datetime/mod.rs

robertbastian · 2022-04-13T19:03:00Z

provider/core/src/resource.rs

+    ///
+    /// If you have ownership over the `ResourceOptions`, use [`ResourceOptions::into_locale()`]
+    /// and then access the `id` field.
+    pub fn langid(&self) -> LanguageIdentifier {


Why doesn't this borrow?

yeah we should probably return an &LanguageIdentifier and let the user clone if they need it; usually common in Rust for the user to have control over that

Till now LanguageIdentifier was expected to be a cheap clone as much as possible so cloning was more okay, but that's also changing with our vertical fallback plans, so i do think we should avoid unnecessary clones.

(interpreting this as a Question)

Mainly because I wanted to stop exposing the internals of ResourceOptions. I've introduced the following functions:

This method, which constructs a new LanguageIdentifier, which usually doesn't allocate

Direct accessors for the language, script, and region subtags, which never allocate

locale(self) which moves out of self without allocating memory and without exposing the internals directly

I'm willing to discuss this and explore alternatives if you have suggestions.

@Manishearth

You made another comment but it's not showing up in this thread.

The main reason I don't want to return &LanguageIdentifier is because I don't want to expose the internals of ResourceOptions. For example, we may consider a change that stores the subtags flat with a single variant, or something else that is hyper optimized for vertical fallback. I expect that most things users would need to do with ResourceOptions should be methods directly on it. It's not the business of the user of this class that we're able to return a &LanguageIdentifier.

provider/datagen/src/cldr/transform/datetime/mod.rs

sffc · 2022-04-13T22:00:15Z

Review notes for @Manishearth:

Start with resource.rs; this contains the actual data model changes
You might look next at the changes to BlobDataProvider and FsDataProvider, and note the changes in the filenames to the JSON files
Most of the rest of the changes are minor, with a few things to point out:
- In DateTimeFormat, I set the Calendar extension into the locale early, and remove it on the data requests that don't need it (I left TODOs for vertical fallback to do that part more automatically)
- I fixed a bug in locale filtering in datagen where it was filtering out week data since it has language "und"

Manishearth

praise: i really like this change!

provider/core/src/resource.rs

Manishearth · 2022-04-14T23:56:56Z

provider/core/src/resource.rs

+    ///
+    /// If you have ownership over the `ResourceOptions`, use [`ResourceOptions::into_locale()`]
+    /// and then access the `id` field.
+    pub fn langid(&self) -> LanguageIdentifier {


yeah we should probably return an &LanguageIdentifier and let the user clone if they need it; usually common in Rust for the user to have control over that

Till now LanguageIdentifier was expected to be a cheap clone as much as possible so cloning was more okay, but that's also changing with our vertical fallback plans, so i do think we should avoid unnecessary clones.

Manishearth · 2022-04-15T00:03:58Z

provider/core/src/resource.rs

+    }
+
+    /// Returns the [`Locale`] for this [`ResourceOptions`].
+    ///


nit: doc that this clones the langid and perhaps suggest folks use the proposed-non-cloning .langid() if they just need a langid

updated the docs.

Actually, I updated the docs, and in the process removed this function. There were not many call sites.

provider/datagen/src/cldr/transform/datetime/mod.rs

components/datetime/src/calendar.rs

sffc · 2022-04-19T06:33:33Z

Changes in the latest commits:

2943e21 = fix docstest
33c384c = rename langid() to get_langid() to emphasize that there is potentially work going on; also delete the unused locale() method
540d0c9 = fix provider_adapters (docstest and a bug uncovered by docstest)
92156d5 = add docstests to locid crate for "true" values (this is unrelated to this PR and could be excluded if you prefer)
a1fe886 = rename unicode_ext() to get_unicode_ext() and add matches_unicode_ext()
ad4dff4 = addcoverage for the functions added in the previous commit

If any of these changes are controversial, I may revert those commits in order to get this PR landed.

robertbastian · 2022-04-19T09:06:59Z

provider/core/src/resource.rs

+    ///     &unicode_ext_value!("coptic"),
+    /// ));
+    /// ```
+    pub fn matches_unicode_ext(&self, key: &unicode_ext::Key, value: &unicode_ext::Value) -> bool {


nit: do we not usually pass tinystrs/subtags by value?

I think the accepted convention is to pass Copy types by value; but there are exceptions both ways. At play here:

In this case, Key but not Value is Copy. I could pass one by value and the other by reference, or both by reference.

Many map interfaces pass by reference all the time, even for Copy types.

I don't have a strong opinion.

sffc added 2 commits March 28, 2022 22:12

Change from str to bytes in blob schema

bf1a343

First draft of merging langid and variant into locale

6499028

robertbastian mentioned this pull request Mar 29, 2022

locale! macro #1751

Merged

sffc added 14 commits March 29, 2022 21:57

Make ResourceOptions fields private

4d34526

Migrate more call sites

5dc6978

Move TODO functions into provider core crate

be0b030

Migrate more call sites

665faab

Migrate more call sites

3ad1fd1

Unused imports

e2f7c29

Docs test

8733eab

fmt

535b1a2

clippy

53a9d0f

Bring code size back down

fe2370c

Merge remote-tracking branch 'upstream/main' into blob-locale

b989179

clippy

8f65c2b

Use Locale

2112c67

This reverts commit fe2370c.

Use LangID + Keywords

6ae5cbf

sffc added 6 commits March 30, 2022 23:32

fix

18e79ea

fix

754996b

fix

e83a670

Re-generate testdata

7b3fa2c

fmt

a844561

clippy

007dd04

sffc changed the title ~~Store variants in the locale~~ Store Unicode keywords in ResourceOptions Mar 31, 2022

sffc added 5 commits March 31, 2022 09:27

Fix week data generation

74ad051

Fix FsDataProvider to be consistent with new file layout

97a9a5b

Merge branch 'main' into blob-locale

912f6e2

Fix build failures

ce86723

Add some helper methods

265a3d9

zbraniecki previously approved these changes Apr 13, 2022

View reviewed changes

robertbastian reviewed Apr 13, 2022

View reviewed changes

sffc added 6 commits April 14, 2022 18:24

IDENTIFIER -> BCP_47_IDENTIFIER

5b9132d

Writeable clarification

10a3557

Add impl From<&Locale> for ResourceOptions

bf85e4c

rm DataRequest::langid()

c80175b

remove extra deref

81516ab

Parse test locales at runtime

9c9b320

sffc dismissed zbraniecki’s stale review via 9c9b320 April 15, 2022 01:49

sffc requested a review from robertbastian April 15, 2022 01:50

Manishearth previously approved these changes Apr 15, 2022

View reviewed changes

robertbastian previously approved these changes Apr 16, 2022

View reviewed changes

sffc added 2 commits April 18, 2022 17:51

Fix docstest

2943e21

Remove cloning locale() method; rename to get_langid(); add more docs

33c384c

sffc dismissed stale reviews from robertbastian and Manishearth via 33c384c April 19, 2022 01:16

sffc added 3 commits April 18, 2022 23:22

provider_adapters fixes

540d0c9

Add docs tests for "true" subtag value

92156d5

Change function name to "get" and return clone; add "matches" function

a1fe886

sffc requested a review from nciric as a code owner April 19, 2022 06:22

sffc removed the request for review from nciric April 19, 2022 06:25

Add docs test for increased coverage

ad4dff4

robertbastian approved these changes Apr 19, 2022

View reviewed changes

sffc merged commit 74cce87 into unicode-org:main Apr 19, 2022

sffc deleted the blob-locale branch April 19, 2022 22:33

sffc mentioned this pull request Apr 30, 2022

When icu::locid::extensions::unicode::Value has a single-segment value, make it borrowable as &str #1764

Closed

sffc mentioned this pull request Jun 13, 2022

Rename ResourceOptions to ResourceLocale/DataLocale #1995

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store Unicode keywords in ResourceOptions #1750

Store Unicode keywords in ResourceOptions #1750

sffc commented Mar 29, 2022 •

edited

Loading

sffc commented Mar 31, 2022 •

edited

Loading

zbraniecki left a comment

Manishearth commented Apr 13, 2022 •

edited

Loading

robertbastian left a comment

robertbastian Apr 13, 2022

Manishearth Apr 14, 2022

sffc Apr 15, 2022

sffc Apr 16, 2022

sffc commented Apr 13, 2022

Manishearth left a comment

Manishearth Apr 14, 2022

Manishearth Apr 15, 2022

sffc Apr 19, 2022

sffc Apr 19, 2022 •

edited

Loading

sffc commented Apr 19, 2022

robertbastian Apr 19, 2022

sffc Apr 19, 2022

Store Unicode keywords in ResourceOptions #1750

Store Unicode keywords in ResourceOptions #1750

Conversation

sffc commented Mar 29, 2022 • edited Loading

sffc commented Mar 31, 2022 • edited Loading

zbraniecki left a comment

Choose a reason for hiding this comment

Manishearth commented Apr 13, 2022 • edited Loading

robertbastian 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

sffc commented Apr 13, 2022

Manishearth 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

sffc Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

sffc commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Mar 29, 2022 •

edited

Loading

sffc commented Mar 31, 2022 •

edited

Loading

Manishearth commented Apr 13, 2022 •

edited

Loading

sffc Apr 19, 2022 •

edited

Loading