Add icu4x-key-extract for Static Data Slicing #1460

sffc · 2021-12-31T02:12:23Z

It works!

$ cargo build --example work_log
$ cargo run --bin icu4x-key-extract -- \
    -i target/debug/examples/work_log \
    -o /tmp/icu4x_data/work_log+keys.txt
$ cat /tmp/icu4x_data/work_log+keys.txt
datetime/lengths@1
datetime/skeletons@1
datetime/symbols@1
plurals/cardinal@1
plurals/ordinal@1

You can even see DCE doing its job. On a release build, the extra plurals key goes away:

$ cargo build --example work_log --release
$ cargo run --bin icu4x-key-extract -- \
    -i target/release/examples/work_log \
    -o /tmp/icu4x_data/work_log+keys.txt
$ cat /tmp/icu4x_data/work_log+keys.txt
datetime/lengths@1
datetime/skeletons@1
datetime/symbols@1
plurals/ordinal@1

The same tool works on both x86 binaries and WASM binaries (and presumably most other binary formats as well). You can run the following command to generate key files for all examples from WASM files:

$ cargo make wasm-key-extract-examples

robertbastian · 2021-12-31T09:48:34Z

provider/core/src/resource.rs

+        version: u8,
+    ) -> ResourceKey {
+        ResourceKey {
+            _tag0: tinystr8!("ICURES[["),


Is this going to work for a BE binary?

It will work so long as the fields are endian-agnostic. I changed the version field to be u8 instead of u16 to address this problem. The only field I'm not sure about is ResourceCategory, but I left a TODO to fix that.

For TinyStr in particular, there's a conversation about it. Short answer is that it actually is endian-safe.

zbraniecki/tinystr#45

For ResourceCategory it will be fine as long as it's repr(u8).

ResourceCategory is dataful, which is why it is hard. The other problem is that I intend to remove ResourceCategory, so I don't want to spend a lot of time solving it. Also, even if ResourceCategory were repr(u8), it's still not safe to transmute, because the u8 could be out-of-range.

What do you suggest as the short-term solution?

Check in this unsafe code first and remove ResourceCategory later

Remove ResourceCategory first before merging this PR

Make ResourceCategory safe, and then delete that code when I remove ResourceCategory

So tinystr8!("foo") has the same bit pattern whether it's compiled for LE or BE?

Regarless of the answer to that question, I think it would be better to use byte arrays (and byte string literals). They are a primitive, are endian-agnostic, and don't require a future reader to be familiar with tinystr's implementation details.

Ah, I did not realize ResourceCategory is dataful.

I'm okay with having this code for now but I'd prioritize fixing this up.

@robertbastian yes, because it has the same bit pattern as the string, and strings are endianness-independent. In fact I've thought about making tinystr internally use byte arrays in the first place.

I still think it overcomplicates things to write a TinyStr but then compare to a byte array.

robertbastian · 2021-12-31T09:49:34Z

tools/datagen/src/bin/datagen.rs

+        allowed_keys.extend(keys.map(|s| Cow::Borrowed(s)));
+    }
+    if let Some(key_file_path) = matches.value_of_os("KEY_FILE") {
+        // eyre::bail!("Key file is not yet supported (see #192)",);


robertbastian · 2022-01-03T09:59:30Z

provider/core/src/resource.rs

+#[repr(C)]
 pub struct ResourceKey {
+    _tag0: TinyStr8,
    pub category: ResourceCategory,
    pub sub_category: TinyStr16,
-    pub version: u16,
+    pub version: u8,
+    _tag1: TinyStr4,
+}


Do we want to put these changes behind a feature? If this is going to be transparent to the client, we can also set the feature correctly and avoid the tags in production code.

Manishearth

Overall looks good, I would like from_repr_c to be made safe though.

Manishearth · 2022-01-04T21:33:01Z

provider/core/src/resource.rs

+        version: u8,
+    ) -> ResourceKey {
+        ResourceKey {
+            _tag0: tinystr8!("ICURES[["),


For ResourceCategory it will be fine as long as it's repr(u8).

Manishearth · 2022-01-04T21:33:52Z

provider/core/src/resource.rs

 pub struct ResourceKey {
+    _tag0: TinyStr8,


thought: not a huge fan of this, but I guess it's no big deal and it does seem to be the best way to achieve this

Manishearth · 2022-01-04T21:50:10Z

tools/datagen/README.md

 ```

-Generate ICU4X Postcard blob (single file):
+Extract the keys from an executable into a key file:


nit: "Extract the keys used by an executable into a key file"

nordzilla

This generally looks good to me.

Interesting to read the concerns about the endianness, but we seem to be aware of the potential/existing issues there.

I'm still unable to run datagen locally via:

cargo run --bin icu4x-datagen -- \        
    --cldr-tag 39.0.0 \
    --all-keys \
    --all-locales \
    --format blob \
    --out /tmp/icu4x_data/icu4x_data.postcard

I still get the "Please call close before dropping FilesystemExporter" bracktrace.

nordzilla · 2022-01-04T23:48:16Z

tools/datagen/src/bin/datagen.rs

+    let mut allowed_keys: Option<HashSet<Cow<str>>> = None;
+    if let Some(keys) = matches.values_of("KEYS") {
+        let allowed_keys = allowed_keys.get_or_insert_with(Default::default);
+        allowed_keys.extend(keys.map(|s| Cow::Borrowed(s)));


Suggestion (non-blocking)

This could probably be just

let mut allowed_keys = matches .values_of("KEYS") .map(|keys| keys.map(Cow::Borrowed).collect::<HashSet<_>>()); if let Some(key_file_path) = matches.value_of_os("KEY_FILE") { ...

Since I don't think get get case of get_or_insert_with can ever happen here when we always initialize it to None.

robertbastian · 2022-01-07T10:36:37Z

provider/core/src/resource.rs

+        version: u8,
+    ) -> ResourceKey {
+        ResourceKey {
+            _tag0: tinystr8!("ICURES[["),


So tinystr8!("foo") has the same bit pattern whether it's compiled for LE or BE?

Regarless of the answer to that question, I think it would be better to use byte arrays (and byte string literals). They are a primitive, are endian-agnostic, and don't require a future reader to be familiar with tinystr's implementation details.

robertbastian · 2022-01-07T11:34:49Z

provider/core/src/resource.rs

+    /// ```
+    pub fn from_repr_c(bytes: &[u8; 40]) -> Option<ResourceKey> {
+        // Smoke check
+        if &bytes[0..8] != b"ICURES[[" || &bytes[36..40] != b"]]**" {


b"ICURES[[" and b"]]** are hardcoded in three places (new, from_repr_c, and extract.rs), maybe define a const?

sffc · 2022-01-14T21:30:02Z

I'm going to let this PR bitrot a little and re-do it after the ResourceKey changes from #1148 are implemented.

sffc · 2022-02-03T18:13:41Z

Superseded by #1480

sffc added 6 commits December 30, 2021 14:48

Add repr(C) for ResourceKey

9ae42b6

Add icu4x-key-extract

b4c75ae

Add icu4x-key-extract test to CI

c511e8d

Add support for and test --key-file in icu4x-datagen

8466025

Update docs for new icu4x-key-extract tool

22cabee

Update readmes again

596e252

sffc requested review from zbraniecki, Manishearth, robertbastian and nordzilla December 31, 2021 02:12

sffc requested a review from a team as a code owner December 31, 2021 02:12

sffc requested a review from iainireland December 31, 2021 02:13

sffc added 4 commits December 30, 2021 18:16

fmt

87c93d1

Add missing docs

6e8b75d

Fix up bincode jobs

2f7e1c2

Fix docs test

63b08a5

robertbastian reviewed Dec 31, 2021

View reviewed changes

Remove obsolete comment

b665dbb

robertbastian reviewed Jan 3, 2022

View reviewed changes

Manishearth reviewed Jan 4, 2022

View reviewed changes

nordzilla removed their request for review January 4, 2022 23:12

nordzilla reviewed Jan 5, 2022

View reviewed changes

robertbastian requested changes Jan 7, 2022

View reviewed changes

robertbastian mentioned this pull request Jan 20, 2022

Datagen key file support and test scaffolding #1526

Merged

sffc marked this pull request as draft January 20, 2022 18:25

sffc added the discuss-priority Discuss at the next ICU4X meeting label Jan 20, 2022

sffc mentioned this pull request Jan 20, 2022

Implement icu4x-key-extract by string tagging #1480

Merged

sffc closed this Feb 3, 2022

sffc removed the discuss-priority Discuss at the next ICU4X meeting label Feb 3, 2022

sffc deleted the key-extract branch August 24, 2022 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add icu4x-key-extract for Static Data Slicing #1460

Add icu4x-key-extract for Static Data Slicing #1460

sffc commented Dec 31, 2021 •

edited

Loading

robertbastian Dec 31, 2021

sffc Dec 31, 2021

sffc Dec 31, 2021

Manishearth Jan 4, 2022

sffc Jan 7, 2022

robertbastian Jan 7, 2022

Manishearth Jan 7, 2022

robertbastian Jan 7, 2022

robertbastian Dec 31, 2021

robertbastian Jan 3, 2022

Manishearth left a comment

Manishearth Jan 4, 2022

Manishearth Jan 4, 2022

Manishearth Jan 4, 2022

nordzilla left a comment

nordzilla Jan 4, 2022

robertbastian Jan 7, 2022

robertbastian Jan 7, 2022

sffc commented Jan 14, 2022

sffc commented Feb 3, 2022

Add icu4x-key-extract for Static Data Slicing #1460

Add icu4x-key-extract for Static Data Slicing #1460

Conversation

sffc commented Dec 31, 2021 • 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

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

nordzilla 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 commented Jan 14, 2022

sffc commented Feb 3, 2022

sffc commented Dec 31, 2021 •

edited

Loading