Switch PluralRules Data to ZeroVec #1240

zbraniecki · 2021-10-28T22:31:08Z

^^^^^^^^^^^

zbraniecki · 2021-10-28T23:38:12Z

Fixes #615.

jira-pull-request-webhook · 2021-10-29T00:18:23Z

Notice: the branch changed across the force-push!

components/datetime/src/datetime.rs is different
components/datetime/src/format/datetime.rs is different
components/datetime/src/zoned_datetime.rs is different
components/plurals/examples/elevator_floors.rs is no longer changed in the branch
components/plurals/examples/unread_emails.rs is no longer changed in the branch
components/plurals/src/lib.rs is different
components/plurals/src/rules/runtime/ast.rs is different
components/plurals/tests/categories.rs is now changed in the branch
components/plurals/tests/fixtures/categories.json is now changed in the branch
components/plurals/tests/fixtures/mod.rs is now changed in the branch
components/plurals/tests/plurals.rs is different
provider/testdata/data/testdata.postcard is different
utils/zerovec/src/zerovec/mod.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

zbraniecki · 2021-10-29T01:42:06Z

@Manishearth @sffc - can you skim through for sanity check before I start polishing?

Manishearth

Looks like you're on the right track so far!

components/plurals/src/lib.rs

components/plurals/src/rules/runtime/ast.rs

Manishearth · 2021-10-29T22:05:44Z

components/plurals/src/rules/runtime/ast.rs

+#[derive(Clone, Debug, PartialEq)]
+pub struct Relation<'data> {
+    pub(crate) and_or: AndOr,
+    pub(crate) polarity: Polarity,


thought (also a note to self, really): if these are refactored into an AndOrPolarityOperand struct on the non-ULE side and we just implement ULE on that (instead of doing the andor_polarity_operand field) this becomes something much more amenable to the custom derive that I hope to have

jira-pull-request-webhook · 2021-11-04T22:50:57Z

Notice: the branch changed across the force-push!

Cargo.lock is different
components/plurals/Cargo.toml is different
components/plurals/src/lib.rs is different
components/plurals/src/rules/runtime/ast.rs is different
provider/cldr/src/transform/plurals.rs is different
provider/testdata/data/testdata.postcard is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

zbraniecki · 2021-11-04T22:53:50Z

I applied Manish feedback and tested using exponent for modulo and cutting RangeOrValueULE to be PlainOldULE<4> - neither really impacts performance (the former deteriorates perf, the latter gives ~1-2% win), so I'd prefer not to overoptimize it right now.

@sffc @Manishearth - can I get second round of feedback? After that, I'd like to polish the code and submit for full review.

sffc

A few comments to start; more coming soon

components/plurals/src/lib.rs

components/plurals/src/rules/mod.rs

components/plurals/src/rules/runtime/ast.rs

sffc

Next round of feedback. I finished runtime/ast.rs which seems to be one of the bigger files.

components/plurals/src/rules/runtime/ast.rs

sffc · 2021-11-05T03:29:06Z

components/plurals/src/rules/runtime/ast.rs

+    type Error = &'static str;
+
+    #[inline]
+    unsafe fn from_byte_slice_unchecked(bytes: &[u8]) -> &Self {


Request: please verify this impl with @Manishearth

Hehehe I wrote this code but yes I should have a closer look, will mention when I've reviewed

components/plurals/src/rules/runtime/ast.rs

sffc

Good! Nothing big, just a few more small things.

components/plurals/src/rules/runtime/ast.rs

components/plurals/src/rules/runtime/resolver.rs

components/plurals/tests/fixtures/categories.json

utils/zerovec/src/zerovec/mod.rs

jira-pull-request-webhook · 2021-11-08T21:30:28Z

Notice: the branch changed across the force-push!

components/plurals/src/rules/runtime/ast.rs is different
utils/zerovec/src/ule/pair.rs is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

zbraniecki · 2021-11-08T23:09:41Z

I applied the next round of feedback and I think I'm ready to clean up the code, document fix tests and put for full review.

But before I do, I noticed something concerning:

The performance is good - plurals/pluralrules/overview goes from 27us on my machine to 24us for an 11% win! Yay!

But the memory is much less affected.

unread_emails on main:

[memory] > dhat: Total:     15,573 bytes in 12 blocks
[memory] > dhat: At t-gmax: 15,311 bytes in 5 blocks

unread_emails on pr-zv:

[memory] > dhat: Total:     15,078 bytes in 4 blocks
[memory] > dhat: At t-gmax: 15,070 bytes in 3 blocks

elevator_flooors on main:

[memory] > dhat: Total:     16,601 bytes in 28 blocks
[memory] > dhat: At t-gmax: 15,732 bytes in 15 blocks

elevator_flooors on pr-zv:

[memory] > dhat: Total:     15,077 bytes in 4 blocks
[memory] > dhat: At t-gmax: 15,069 bytes in 3 blocks

@sffc @Manishearth - is that expected? Is it VZV cost, or should we investigate further?

Manishearth · 2021-11-08T23:15:18Z

Note that those examples use a static data provider so they're both loading all of the data as part of the binary, which is probably where most of that number comes from.

Manishearth · 2021-11-08T23:21:09Z

It would be worth diffing the amount the memory usage increases between the start of main() and the construction of the PluralRules object

jira-pull-request-webhook · 2021-11-15T16:25:13Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

…f it.

zbraniecki force-pushed the pr-zv branch from a287670 to 56fb64c Compare October 29, 2021 00:18

Manishearth reviewed Oct 29, 2021

View reviewed changes

zbraniecki force-pushed the pr-zv branch from 56fb64c to 619dd22 Compare November 4, 2021 22:50

zbraniecki requested a review from sffc November 4, 2021 22:52

sffc reviewed Nov 4, 2021

View reviewed changes

components/plurals/src/lib.rs Show resolved Hide resolved

components/plurals/src/rules/mod.rs Show resolved Hide resolved

components/plurals/src/rules/runtime/ast.rs Outdated Show resolved Hide resolved

sffc requested changes Nov 5, 2021

View reviewed changes

zbraniecki changed the title ~~Zibi's PR ZV PR~~ ZB PR ZV PR Nov 5, 2021

Manishearth reviewed Nov 5, 2021

View reviewed changes

components/plurals/src/rules/runtime/ast.rs Show resolved Hide resolved

sffc requested changes Nov 6, 2021

View reviewed changes

zbraniecki force-pushed the pr-zv branch from 59a14f0 to bbebe71 Compare November 8, 2021 21:30

zbraniecki marked this pull request as ready for review November 10, 2021 00:38

zbraniecki requested review from filmil, gregtatum, nordzilla and a team as code owners November 10, 2021 00:38

zbraniecki requested review from sffc and Manishearth and removed request for filmil, gregtatum and nordzilla November 10, 2021 00:39

zbraniecki force-pushed the pr-zv branch from 2537d69 to 2b36626 Compare November 10, 2021 00:43

zbraniecki added 22 commits November 15, 2021 10:24

Plug it all together

978c6e3

Fix tests

5fd0dbb

Apply feedback

bad24b0

Add RelationULE doc

562602b

Apply feedback

1f5c9c3

Add inlines and docs

85bef5f

Uncomment tests

2aca97a

Apply reviewers feedback

02eba1c

Fix tests and apply feedback

55f1f30

Revise postcard file

c6925e8

Fix provider benchmarks

af0a5bf

Fix more tests

53bdb26

Add rountrip test

ac11dae

Fix another test

4344425

Fix more tests

d3ac05c

Update postcard

e64d626

Fix readme

aa05c79

Regenerate diplomat ffi

3059542

Fix diplomat example

8e64819

Fix test-cpp

4d8696f

Update safety comment

365f8bd

Apply feedback

460b5b6

zbraniecki force-pushed the pr-zv branch from a84776a to 460b5b6 Compare November 15, 2021 16:25

No need to guard local type used for debug test. DCE will take care o…

8db9c46

…f it.

sffc approved these changes Nov 15, 2021

View reviewed changes

Manishearth approved these changes Nov 15, 2021

View reviewed changes

zbraniecki merged commit e5da8ba into unicode-org:main Nov 15, 2021

zbraniecki mentioned this pull request Nov 15, 2021

PluralRuleStringsV1 should be parsed at data load time #615

Closed

sffc mentioned this pull request Nov 22, 2021

Favor SmallVec over Box in pluralrules #285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch PluralRules Data to ZeroVec #1240

Switch PluralRules Data to ZeroVec #1240

zbraniecki commented Oct 28, 2021

zbraniecki commented Oct 28, 2021

jira-pull-request-webhook bot commented Oct 29, 2021

zbraniecki commented Oct 29, 2021

Manishearth left a comment

Manishearth Oct 29, 2021

jira-pull-request-webhook bot commented Nov 4, 2021

zbraniecki commented Nov 4, 2021

sffc left a comment

sffc left a comment

sffc Nov 5, 2021

Manishearth Nov 5, 2021

sffc left a comment

jira-pull-request-webhook bot commented Nov 8, 2021

zbraniecki commented Nov 8, 2021

Manishearth commented Nov 8, 2021

Manishearth commented Nov 8, 2021

jira-pull-request-webhook bot commented Nov 15, 2021

Switch PluralRules Data to ZeroVec #1240

Switch PluralRules Data to ZeroVec #1240

Conversation

zbraniecki commented Oct 28, 2021

zbraniecki commented Oct 28, 2021

jira-pull-request-webhook bot commented Oct 29, 2021

zbraniecki commented Oct 29, 2021

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth Oct 29, 2021

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Nov 4, 2021

zbraniecki commented Nov 4, 2021

sffc left a comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

sffc Nov 5, 2021

Choose a reason for hiding this comment

Manishearth Nov 5, 2021

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Nov 8, 2021

zbraniecki commented Nov 8, 2021

Manishearth commented Nov 8, 2021

Manishearth commented Nov 8, 2021

jira-pull-request-webhook bot commented Nov 15, 2021