Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove FlexZeroVec #5604

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Remove FlexZeroVec #5604

merged 2 commits into from
Oct 2, 2024

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Sep 27, 2024

We don't really use it anymore I think, except potentially for some usize ZHMs, which we can instead use Index8 VZV backing stores for if we desire.

@Manishearth Manishearth requested a review from a team as a code owner September 27, 2024 01:26
@Manishearth Manishearth force-pushed the flexzero-rm branch 2 times, most recently from 913eb00 to 483d741 Compare September 27, 2024 01:27
@Manishearth
Copy link
Member Author

Manishearth commented Sep 27, 2024

@sffc one lingering use case is in the ZeroTrie benches:

let actual = actual.map(|x| <zerovec::vecs::FlexZeroSlice as zerovec::maps::ZeroVecLike<usize>>::zvl_get_as_t(x, |y| *y));

This is the usize case. I'm happy with getting rid of it for now.

I don't really think it makes sense for usize to special-case as a flex container anyway.

@Manishearth
Copy link
Member Author

The other breakage is in BlobSchemaV1, which is probably going to go away in #5603

pub markers: ZeroMap2dBorrowed<'data, DataMarkerPathHash, Index32U8, usize>,

@Manishearth Manishearth changed the title Remove FlexZeroVec Remove FlexZeroVec and BlobSchemaV001 Sep 27, 2024
@Manishearth Manishearth changed the title Remove FlexZeroVec and BlobSchemaV001 Remove FlexZeroVec Sep 27, 2024
@Manishearth
Copy link
Member Author

This can't be landed until we fix #5603

@Manishearth Manishearth force-pushed the flexzero-rm branch 3 times, most recently from dc3669f to 1265158 Compare September 30, 2024 18:12
@Manishearth
Copy link
Member Author

The bulk of the FZV usage is in the ZeroTrie benches 🙃

@Manishearth Manishearth force-pushed the flexzero-rm branch 3 times, most recently from e3b2cdc to 81f3f21 Compare October 1, 2024 18:36
sffc
sffc previously approved these changes Oct 2, 2024
@@ -57,11 +57,11 @@ fn get_basic_bench(c: &mut Criterion) {

#[cfg(feature = "bench")]
g.bench_function("ZeroMap/usize", |b| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
g.bench_function("ZeroMap/usize", |b| {
g.bench_function("ZeroMap/u32", |b| {

@@ -171,11 +155,11 @@ fn get_subtags_bench_helper<M: criterion::measurement::Measurement>(

#[cfg(feature = "bench")]
g.bench_function("ZeroMap/usize", |b| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
g.bench_function("ZeroMap/usize", |b| {
g.bench_function("ZeroMap/u32", |b| {

sffc
sffc previously approved these changes Oct 2, 2024
@sffc
Copy link
Member

sffc commented Oct 2, 2024

What changed across the force-push? I see a bunch of changes pulled in from main.

@Manishearth
Copy link
Member Author

@sffc rebase over main and fix the zerotrie builder test numbers broken by the VZV changes

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the two diffs you applied earlier got undone

@Manishearth
Copy link
Member Author

Oops, fixed. Forgot I applied those patches late at night

@Manishearth Manishearth merged commit 951ca99 into unicode-org:main Oct 2, 2024
28 checks passed
@Manishearth Manishearth deleted the flexzero-rm branch October 2, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants