Add payload conversion and heap measurement tool #1670

Manishearth · 2022-03-05T03:16:52Z

This adds icu4x-verify-zero-copy, that loads testdata.postcard and deserializes each entry that it knows to look for to its specific data struct type, while measuring heap usage. It then reports which keys have non-zero-copy data structs. (this is #1661)

This also updates impl_dyn_provider!() to be able to provide DataConverter impls that act as a "data registry" (#1512), allowing one to convert between different dynamic marker types via intermediate downcasting.

Todo:

docs and comments
run this in CI

Manishearth · 2022-03-05T05:10:40Z

cc @robertbastian @zbraniecki

Manishearth · 2022-03-08T17:00:00Z

cc @robertbastian btw

Manishearth · 2022-03-08T17:53:34Z

tools/datagen/src/bin/verify-zero-copy.rs

+                .help("Load input data from the icu_testdata project."),
+        )
+        .arg(
+            Arg::with_name("KEYS")


I added this because I found it useful to be --keys specifickey -v to debug things

Also add ALL_KEYS?

There's currently no difference and we can add that as needed I think

The difference with ALL_KEYS over TEST_KEYS is that in line 220 we wouldn't ignore key errors.

That means all-keys would always error, datagen doesn't have proper support for this either. I'm only adding features that'll actually be useful.

Overall I don't want this to just duplicate that other set of flags until we need those features

jira-pull-request-webhook · 2022-03-08T18:33:20Z

Notice: the branch changed across the force-push!

Cargo.lock is different
tools/datagen/src/bin/verify-zero-copy.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

sffc

First batch; more in the morning

provider/core/src/dynutil.rs

provider/core/src/datagen/heap_measure.rs

provider/core/src/datagen/data_conversion.rs

tools/datagen/src/bin/datagen.rs.orig

tools/datagen/src/bin/datagen.rs

tools/datagen/src/bin/verify-zero-copy.rs

provider/core/src/datagen/data_conversion.rs

provider/core/src/datagen/heap_measure.rs

robertbastian · 2022-03-09T09:28:32Z

provider/core/src/datagen/mod.rs

+/// A blanket-implemented trait that allows for all the datagen-relevant traits
+/// to be used in trait objects. The type parameter is the marker type needed for
+/// [`IterableDynProvider`].
+pub trait OmnibusDatagenProvider<M: DataMarker>:


Can we just call this DatagenProvider?

Actually in #1092 we said that IterableDynProvider is not the right trait for runtime, so maybe we can just rename that DatagenProvider and make it extend the DataConverter impls? It should also be behind the datagen feature now that that exists.

I'd rather do wider refactors around what's under what feature as a result of #1320 (which I plan to get done soon)

Still, what do buses have to do with this

"Omnibus" is just a term used in English to talk about things that contain more things than usual, typically in the case of media around anthologies and "3-in-1 books".

In this case, this is really just "a lot of traits in one trait", hence the trait name.

I have never seen it used that way, maybe it's an American thing. I'd prefer if we avoid such words, we should keep our code accessible to non-Americans and non-native English speakers.

Ah, gotcha. It is convenient to be able to have the list of all sub-providers in one place though

(perhaps we could have that list be in the datagen crate)

(perhaps we could have that list be in the datagen crate)

This would be fine with me. Alternatively: #1684

That is a cool solution, can you incorporate #1684 ?

It's not sufficient in and of itself without further work: you either need to name the really complex type over the boundary or you need to do a bunch of manual passthrough implementations. I'd rather land this first, I don't consider this additional trait to be that much overhead.

robertbastian · 2022-03-09T09:35:25Z

provider/core/src/dynutil.rs

@@ -189,6 +204,52 @@ macro_rules! impl_dyn_provider {
            $crate::serde::SerializeMarker
        );
    };
+    ($provider:ty, [ $($struct_m:ty),+, ], SERDE_SE, impl DataConverter) => {


This macro is getting massive. Now that we have a datagen feature we could split it into what it was before #1595, and a second macro (impl_datagen_provider) that additionally implements IterableDynProvider and DataConverter (and which we can also put behind the datagen feature).

I'm waiting for the ResourceProvider stuff to happen before making further changes here

What ResourceProvider stuff?

#1554 was the main thing, but I believe there's more stuff to be done here around Buffer and Any providers, especially using the new Convert infrastructure. Shane's got a whole plan 😄 (I can't find the right issue at the moment)

robertbastian · 2022-03-09T09:44:52Z

tools/datagen/src/bin/verify-zero-copy.rs

+                .help("Load input data from the icu_testdata project."),
+        )
+        .arg(
+            Arg::with_name("KEYS")


Also add ALL_KEYS?

tools/datagen/src/bin/verify-zero-copy.rs

sffc

(touch)

tools/datagen/src/bin/datagen.rs

jira-pull-request-webhook · 2022-03-09T16:27:36Z

Notice: the branch changed across the force-push!

Cargo.lock is different
components/properties/src/provider.rs is different
provider/cldr/src/transform/list/mod.rs is different
provider/uprops/src/enum_uniset.rs is different
tools/datagen/Cargo.toml is different
tools/datagen/src/bin/datagen.rs is different
tools/datagen/src/lib.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Manishearth · 2022-03-09T16:28:31Z

Rebased over segmenter changes, no changes between your last review and this except for 4c8b208 and merge conflict resolution

robertbastian · 2022-03-10T09:32:01Z

tools/datagen/src/bin/verify-zero-copy.rs

+                .help("Load input data from the icu_testdata project."),
+        )
+        .arg(
+            Arg::with_name("KEYS")


The difference with ALL_KEYS over TEST_KEYS is that in line 220 we wouldn't ignore key errors.

robertbastian · 2022-03-10T09:37:05Z

provider/core/src/datagen/mod.rs

+/// A blanket-implemented trait that allows for all the datagen-relevant traits
+/// to be used in trait objects. The type parameter is the marker type needed for
+/// [`IterableDynProvider`].
+pub trait OmnibusDatagenProvider<M: DataMarker>:


That is a cool solution, can you incorporate #1684 ?

robertbastian · 2022-03-10T09:37:28Z

provider/core/src/dynutil.rs

@@ -189,6 +204,52 @@ macro_rules! impl_dyn_provider {
            $crate::serde::SerializeMarker
        );
    };
+    ($provider:ty, [ $($struct_m:ty),+, ], SERDE_SE, impl DataConverter) => {


What ResourceProvider stuff?

Manishearth requested review from sffc, nordzilla and a team as code owners March 5, 2022 03:16

This was referenced Mar 5, 2022

Write tooling to keep track of (and enforce) that our data structs do not allocate #1661

Closed

Export key registry from icu_datagen #1512

Closed

Manishearth changed the title ~~Add payload conversion and heap measurement code~~ Add payload conversion and heap measurement tool Mar 5, 2022

Manishearth removed request for a team and nordzilla March 5, 2022 05:10

Manishearth commented Mar 8, 2022

View reviewed changes

Manishearth mentioned this pull request Mar 8, 2022

Make time_zone/formats@1 fully zero-copy #1676

Merged

Manishearth force-pushed the verify-zero-copy branch from 78b3eac to 1883b45 Compare March 8, 2022 18:33

sffc requested changes Mar 9, 2022

View reviewed changes

provider/core/src/dynutil.rs Show resolved Hide resolved

provider/core/src/datagen/heap_measure.rs Outdated Show resolved Hide resolved

provider/core/src/datagen/data_conversion.rs Show resolved Hide resolved

tools/datagen/src/bin/datagen.rs.orig Outdated Show resolved Hide resolved

robertbastian requested changes Mar 9, 2022

View reviewed changes

sffc reviewed Mar 9, 2022

View reviewed changes

tools/datagen/src/bin/verify-zero-copy.rs Show resolved Hide resolved

sffc reviewed Mar 9, 2022

View reviewed changes

tools/datagen/src/bin/datagen.rs Show resolved Hide resolved

Manishearth added 11 commits March 9, 2022 08:25

Add payload conversion and heap measurement stuff

f6b281d

Fix ci

3a6f30b

fmt

db518b7

docs

3dd365c

Include in CI

3a0743f

add readme contents to datagen

fae0e11

clip

10bf9ea

features fix

4f1eee6

fmt

e9dda42

include experimental keys

2f07997

Add --keys

30c8b46

Manishearth added 6 commits March 9, 2022 08:26

Add --check

503caf8

rm time zone formats

c03303e

Add final_bytes_allocated but don't use it yet

26d0792

fixes

ba2c9f2

fixes

74c1b97

update to net

4c8b208

Manishearth force-pushed the verify-zero-copy branch from f94ec0d to 4c8b208 Compare March 9, 2022 16:27

hook in segmenter data

ff49018

Manishearth requested review from aethanyc and makotokato as code owners March 9, 2022 16:32

Manishearth added 2 commits March 9, 2022 08:33

fix datagen

932c7f6

fix

50ee281

sffc approved these changes Mar 9, 2022

View reviewed changes

Manishearth requested review from robertbastian and removed request for aethanyc and makotokato March 9, 2022 21:52

robertbastian requested changes Mar 10, 2022

View reviewed changes

Manishearth requested a review from robertbastian March 10, 2022 16:21

robertbastian approved these changes Mar 10, 2022

View reviewed changes

Manishearth merged commit 37118aa into unicode-org:main Mar 10, 2022

Manishearth deleted the verify-zero-copy branch March 10, 2022 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add payload conversion and heap measurement tool #1670

Add payload conversion and heap measurement tool #1670

Manishearth commented Mar 5, 2022 •

edited

Loading

Manishearth commented Mar 5, 2022

Manishearth commented Mar 8, 2022

Manishearth Mar 8, 2022

robertbastian Mar 9, 2022

Manishearth Mar 9, 2022

robertbastian Mar 10, 2022

Manishearth Mar 10, 2022

Manishearth Mar 10, 2022

jira-pull-request-webhook bot commented Mar 8, 2022

sffc left a comment

robertbastian Mar 9, 2022

Manishearth Mar 9, 2022

robertbastian Mar 9, 2022

Manishearth Mar 9, 2022

robertbastian Mar 9, 2022

Manishearth Mar 10, 2022

Manishearth Mar 10, 2022

sffc Mar 10, 2022

robertbastian Mar 10, 2022

Manishearth Mar 10, 2022

robertbastian Mar 9, 2022

Manishearth Mar 9, 2022

robertbastian Mar 10, 2022

Manishearth Mar 10, 2022 •

edited

Loading

robertbastian Mar 9, 2022

sffc left a comment

jira-pull-request-webhook bot commented Mar 9, 2022

Manishearth commented Mar 9, 2022

robertbastian Mar 10, 2022

robertbastian Mar 10, 2022

robertbastian Mar 10, 2022

Add payload conversion and heap measurement tool #1670

Add payload conversion and heap measurement tool #1670

Conversation

Manishearth commented Mar 5, 2022 • edited Loading

Manishearth commented Mar 5, 2022

Manishearth commented Mar 8, 2022

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 Mar 8, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth Mar 10, 2022 • 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

jira-pull-request-webhook bot commented Mar 9, 2022

Manishearth commented Mar 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Mar 5, 2022 •

edited

Loading

Manishearth Mar 10, 2022 •

edited

Loading