FFI baked data #2947

robertbastian · 2023-01-03T23:18:31Z

Added a COMPLETE option to databake:
```
struct Provider;
include!(...);
impl_data_provider!(Provider, COMPLETE);
```
This will implement all keys on Provider, even those that weren't generated. We need this as otherwise icu_capi won't compile unless it has complete data, but we don't want to rely just on DCE.
Added a baked provider to icu_capi. This uses data from the ICU4_FFI_BAKED_ROOT environment variable, or falls back to empty data
Split the Empty variant into Empty and Destroyed
- Destroyed always returns errors (I wonder if we should just make it UB). We should teach Diplomat about ownership so we can let the compatibility layer deal with this
- Empty now supports fork_by_key, fork_by_locale and enable_fallback (basically no-ops)
Updated the fixeddecimal_tiny example to use databake: 26k -> 21k
Removed any_provider as a default feature. The baked provider doesn't go through dyn AnyProvider, unless locale fallback is enabled, then we need the feature.

robertbastian · 2023-01-05T17:36:20Z

Commits before 32afdd1 are from #2950

provider/datagen/src/databake.rs

Manishearth · 2023-01-06T20:23:06Z

ffi/diplomat/build.rs

+        // Empty data generated with
+        // cargo run -p icu_datagen --features bin,icu_segmenter -- --format mod --use-separate-crates --keys none --out empty_bake
+        println!(
+            "cargo:rustc-env=ICU4X_FFI_BAKED_ROOT={}/empty_bake",


issue: we shouldn't break the ability to build this crate without build scripts working; there are a bunch of build environments that are bad at running build scripts and ideally this should be something completely unnecessary to run 99% of the time.

I guess we can just ask people using strange build systems to just set this env var, but idk.

I was under the impression baked data was going to be behind a feature?

Yeah this is why the GN CI is failing. I was hoping we wouldn't need to feature-gate.

I don't immediately see why we need a build.rs to do this computation? Can we use std::option_env instead?

include! requires a string literal argument, which core::env!("FOO") is, and core::option_env!("FOO").unwrap_or_else("bar") isn't.

rust-lang/rust#48952

We could use https://crates.io/crates/default-env, but that's a proc macro. Still better than a build script I guess?

Clients would just have to pass in env=empty.rs for that to work. I don't consider that a huge change given that data slicing is anyway a weird cyclic part of the build with baked data.

(Literally can't see a clean way to do it with baked data in build systems like Blaze, for example)

I would like it if we got automatic DCE-based data slicing in Bazel/GN

Bazel/etc really do not like cyclic deps, this would involve a genrule that is able to analyze the build graph, copy it over, and run it once. The compiled binary cannot be an input for generating code

I strongly suggest we make the recommended workflow there to use key files, and provide tools for manually generating that key file (which is fine!)

Anyway, we should have this discussion elsewhere I think (i'm filing an issue for a generic discussion that keeps cropping up, but we need another for how we plan to make keyextract work with such build systems

What I mean is that it would be nice if in Bazel/GN using baked data that we don't need to run datagen against a key file or the binary; that the dataful DataProvider<M> impls can be DCEd.

Ah, I see. We could, though that's going to become a problem again if combinators are used.

sffc

Maybe we should solve #2945 first?

sffc · 2023-01-10T06:02:50Z

ffi/diplomat/src/provider.rs

+                ICU4XDataProviderInner::Baked => panic!(
+                    "Locale fallback for baked providers requires the 'any_provider' feature"
+                ),


Thought/Suggestion: You can do this without any_provider by adding another variant to the enum, BakedFallback or something like that. I think it's a common enough case to warrant that.

Nit: I think panic! might be too strong for this case

I tried that. Problem is that fallback doesn't get DCE'd then.

Ah I didn't try exactly that, I tried Baked(Option<LocaleFallbacker>). I guess having a separate variant will still work with DCE, but we get exponential blowup of forking.

We need to teach diplomat about cfg, then this whole method can be cfg(any(feature = "any_provider", feature = "buffer_provider"))

For now we do cfgs inside the methods

I tried again with an extra BakedFallback(Vec<LocaleFallbacker>) option, but the compiler cannot figure out that a variant is never used, so it always pulls in the fallback code (which has to be in load).

the variant should also be cfg'd off, yes?

That would require a separate baked_fallback feature. I'm trying to reduce feature complexity these days...

sffc · 2023-01-10T06:05:47Z

ffi/diplomat/src/provider.rs

+                    ICU4XDataProvider(ICU4XDataProviderInner::Baked)
+                }
+                #[cfg(feature = "any_provider")]
+                (ICU4XDataProviderInner::Any(a), ICU4XDataProviderInner::Baked) => {


Thought/Issue: I think this means that if the caller invokes fork_by_key, even if they aren't using the baked provider, they get the massive Baked AnyProvider impl, with no way to disable it; even if it is an empty baked provider, that's still a lot of vtable and ZeroFrom

Sure, but the AnyProvider itself will be the baked provider wrapped in locale fallback or forking, so there's no additional vtable.

sffc · 2023-01-10T06:09:32Z

ffi/diplomat/build.rs

+        // Empty data generated with
+        // cargo run -p icu_datagen --features bin,icu_segmenter -- --format mod --use-separate-crates --keys none --out empty_bake
+        println!(
+            "cargo:rustc-env=ICU4X_FFI_BAKED_ROOT={}/empty_bake",


I don't immediately see why we need a build.rs to do this computation? Can we use std::option_env instead?

jira-pull-request-webhook · 2023-01-10T10:32:38Z

Notice: the branch changed across the force-push!

.github/workflows/build-test.yml is no longer changed in the branch
docs/tutorials/data_management.md is no longer changed in the branch
ffi/diplomat/js/examples/node/build.sh is no longer changed in the branch
ffi/tinywasm/build.sh is no longer changed in the branch
provider/adapters/src/empty.rs is no longer changed in the branch
provider/blob/README.md is no longer changed in the branch
provider/blob/src/lib.rs is no longer changed in the branch
provider/datagen/Cargo.toml is no longer changed in the branch
provider/datagen/README.md is no longer changed in the branch
provider/datagen/src/bin/datagen.rs is no longer changed in the branch
provider/datagen/src/lib.rs is no longer changed in the branch
provider/datagen/src/source.rs is no longer changed in the branch
provider/datagen/src/transform/cldr/source.rs is no longer changed in the branch
provider/fs/README.md is no longer changed in the branch
provider/fs/src/lib.rs is no longer changed in the branch
provider/testdata/src/bin/datagen.rs is no longer changed in the branch
tools/scripts/data.toml is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Manishearth · 2023-01-12T19:56:44Z

ffi/gn/icu4x/BUILD.gn

  if (current_cpu == "wasm32") {
    deps += [ ":log-v0_4_14" ]
  }

-  rustenv = []
+  rustenv = [ "ICU4X_FFI_BAKED_ROOT=../empty_bake" ]


i'd still like there to be a feature so that this isn't necessary

robertbastian · 2023-01-19T15:29:55Z

I'm blocking this on #3005 because it would add another feature to datagen.

sffc · 2023-02-22T01:05:43Z

As discussed: start with the approach listed in #2992 and maybe revisit this later.

robertbastian force-pushed the ffibake branch from a6434d0 to 4d1111a Compare January 4, 2023 15:04

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 4, 2023

robertbastian force-pushed the ffibake branch 3 times, most recently from 4b77e6f to 0eecb14 Compare January 4, 2023 21:14

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 4, 2023

robertbastian force-pushed the ffibake branch 2 times, most recently from 029374e to 4c77d6f Compare January 4, 2023 21:55

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 4, 2023

robertbastian force-pushed the ffibake branch from 4c77d6f to f279f87 Compare January 4, 2023 23:00

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 4, 2023

robertbastian force-pushed the ffibake branch from f279f87 to e5d57fc Compare January 4, 2023 23:15

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 5, 2023

robertbastian force-pushed the ffibake branch from c8ff632 to 3a5b5b4 Compare January 5, 2023 14:45

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 5, 2023

robertbastian force-pushed the ffibake branch from 3a5b5b4 to d905473 Compare January 5, 2023 16:31

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 5, 2023

robertbastian force-pushed the ffibake branch from d905473 to 32afdd1 Compare January 5, 2023 17:14

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 5, 2023

robertbastian marked this pull request as ready for review January 5, 2023 17:35

robertbastian requested review from Manishearth, sffc and a team as code owners January 5, 2023 17:35

Manishearth reviewed Jan 6, 2023

View reviewed changes

robertbastian force-pushed the ffibake branch from 32afdd1 to c2d5a12 Compare January 9, 2023 15:46

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 9, 2023

robertbastian force-pushed the ffibake branch from c2d5a12 to 26b683e Compare January 9, 2023 22:04

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 9, 2023

sffc reviewed Jan 10, 2023

View reviewed changes

ffibake

0f1e796

robertbastian force-pushed the ffibake branch from 26b683e to 0f1e796 Compare January 10, 2023 10:32

Manishearth mentioned this pull request Jan 11, 2023

How supported do we consider non-keyextract users? #2975

Closed

robertbastian force-pushed the ffibake branch from 5b0c7ea to 0f1e796 Compare January 11, 2023 13:30

robertbastian added 4 commits January 11, 2023 15:09

asdf

aadea20

Merge branch 'main' into ffibake

41ff8a3

gn?

702b521

Merge branch 'main' into ffibake

53b71ce

Manishearth reviewed Jan 12, 2023

View reviewed changes

robertbastian added 4 commits January 19, 2023 14:23

Merge branch 'main' into ffibake

c0942e6

feature

5326fd8

diplomat-gen

01f9876

gn

e4d5e2d

Merge main

ff859b6

robertbastian requested review from filmil, zbraniecki, gregtatum and nordzilla as code owners January 23, 2023 21:51

Merge branch 'main' into ffibake

05487af

robertbastian requested a review from Manishearth January 23, 2023 22:06

gregtatum removed their request for review January 25, 2023 19:39

robertbastian closed this Feb 22, 2023

Manishearth mentioned this pull request Feb 22, 2023

C/C++ FFI for BakedDataProvider #2743

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FFI baked data #2947

FFI baked data #2947

robertbastian commented Jan 3, 2023 •

edited

Loading

robertbastian commented Jan 5, 2023

Manishearth Jan 6, 2023

robertbastian Jan 6, 2023

sffc Jan 10, 2023

robertbastian Jan 10, 2023

robertbastian Jan 10, 2023

Manishearth Jan 10, 2023 •

edited

Loading

sffc Jan 11, 2023 •

edited

Loading

Manishearth Jan 11, 2023

sffc Jan 11, 2023 •

edited

Loading

Manishearth Jan 11, 2023

sffc left a comment

sffc Jan 10, 2023

robertbastian Jan 10, 2023

robertbastian Jan 10, 2023

robertbastian Jan 10, 2023

Manishearth Jan 10, 2023

robertbastian Jan 11, 2023

Manishearth Jan 19, 2023

robertbastian Jan 19, 2023

sffc Jan 10, 2023

robertbastian Jan 10, 2023

sffc Jan 10, 2023

jira-pull-request-webhook bot commented Jan 10, 2023

Manishearth Jan 12, 2023

robertbastian Jan 19, 2023

robertbastian commented Jan 19, 2023

sffc commented Feb 22, 2023

FFI baked data #2947

FFI baked data #2947

Conversation

robertbastian commented Jan 3, 2023 • edited Loading

robertbastian commented Jan 5, 2023

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 Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

sffc Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc Jan 11, 2023 • edited Loading

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

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

jira-pull-request-webhook bot commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian commented Jan 19, 2023

sffc commented Feb 22, 2023

robertbastian commented Jan 3, 2023 •

edited

Loading

Manishearth Jan 10, 2023 •

edited

Loading

sffc Jan 11, 2023 •

edited

Loading

sffc Jan 11, 2023 •

edited

Loading