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.

Sign up for GitHub

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

Jump to bottom

Remove the zero index from VarZeroVec #5601

Merged
merged 9 commits into from
Oct 1, 2024
Merged

Remove the zero index from VarZeroVec #5601

merged 9 commits into from
Oct 1, 2024

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Sep 25, 2024 •
edited
Loading

Fixes #5516

Also removes METADATA_WIDTH, we've set it to 0 and aren't using it. It's an unnecessary untested complication on our unsafe code.

@Manishearth Manishearth force-pushed the remove-first-index-vzv branch 4 times, most recently from f06ff0e to 43aec09 Compare September 27, 2024 00:04
@Manishearth Manishearth marked this pull request as ready for review September 27, 2024 00:05
@Manishearth Manishearth requested a review from sffc as a code owner September 27, 2024 00:05
@Manishearth
Copy link
Member Author

I still need to run all the datagens and fix non-zerovec tests, but this works within zv.

@Manishearth Manishearth requested a review from a team as a code owner September 27, 2024 00:16
@Manishearth Manishearth force-pushed the remove-first-index-vzv branch from 789202f to 220b65f Compare September 27, 2024 00:18
@sffc
Copy link
Member

sffc commented Sep 27, 2024

Also removes METADATA_WIDTH, we've set it to 0 and aren't using it. It's an unnecessary untested complication on our unsafe code.

It was there to enable us to use FlexZeroVec as the index store. But, making VZV cheaper overall means that we can lean toward designs with more, shorter VZVs with Index8, instead of fewer, longer ones with Index16. We also now have ZeroTrie which covers some of these types of cases. I'm okay removing METADATA_WIDTH so long as we keep the door open to potentially re-adding a more compact index structure in the future.

@Manishearth
Copy link
Member Author

It was there to enable us to use FlexZeroVec as the index store.

Yeah, that would require far more changes to the unsafe code of VZV than just METADATA_WIDTH, METADATA_WIDTH didn't particularly set the stage for anythig there code-wise.

It would be interesting encapsulating more and more of the format into the trait to allow for fancier custom formats.

@Manishearth Manishearth mentioned this pull request Sep 27, 2024
Closed
@Manishearth Manishearth force-pushed the remove-first-index-vzv branch from 220b65f to 46c16c4 Compare September 27, 2024 00:53
@Manishearth Manishearth force-pushed the remove-first-index-vzv branch 2 times, most recently from d7b6c62 to 1f66a65 Compare September 27, 2024 01:21
sffc
sffc reviewed Sep 27, 2024
View reviewed 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.

To components.rs

utils/zerovec/src/varzerovec/components.rs Outdated Show resolved Hide resolved
utils/zerovec/src/varzerovec/components.rs
// Safety: mid is < end <= len, so in-range
let cmp = predicate(self.get_unchecked(mid));

match cmp {
Copy link
Member

Choose a reason for hiding this comment

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

Question: why did you expand the binary_search impl instead of using the std one?

Copy link
Member Author

Choose a reason for hiding this comment

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

The hacky impl was written assuming a slice to search on. Now I could still use a byte slice that I have cut out appropriately, but the indices slice that we currently use is no longer long enough for this to work, and it was going to get more complicated, so I decided to dispense with the weird hack. I can bring it back and make it work with the weird hack if desired.

@Manishearth Manishearth mentioned this pull request Sep 27, 2024
Merged
Manishearth added a commit that referenced this pull request Sep 30, 2024
@Manishearth
Remove Blob schema v1, v2, add v3 (#5608)
c925c81 
Fixes #5603

v3 isn't yet necessary but will become necessary with
#5601. This can be landed
independently of #5601 but
these PRs will bitrot each other. Slight preference to landing this
first.

(At the moment, V2 == V3, but #5601 breaks the format)

This makes V1 and V2 data still *detectable* by ICU4X, but they will
throw a helpful error when attempting to load them.

From a user's POV, there is now only the "blob" format (which covers
both V003 and V003Bigger, picked at datagen time based on data size.


<!--
Thank you for your pull request to ICU4X!

Reminder: try to use [Conventional
Comments](https://conventionalcomments.org/) to make comments clearer.

Please see
https://github.com/unicode-org/icu4x/blob/main/CONTRIBUTING.md for
general
information on contributing to ICU4X.
-->
@Manishearth Manishearth force-pushed the remove-first-index-vzv branch 3 times, most recently from f00a1d8 to 74c0048 Compare September 30, 2024 18:46
@sffc sffc self-requested a review September 30, 2024 23:34
@Manishearth Manishearth force-pushed the remove-first-index-vzv branch 2 times, most recently from bc43449 to 94f05ab Compare September 30, 2024 23:38
sffc
sffc previously approved these changes Oct 1, 2024
View reviewed changes
tutorials/cpp/fixeddecimal.cpp Outdated Show resolved Hide resolved
utils/zerovec/src/varzerovec/components.rs
Comment on lines +352 to +353
let indices_bytes = slice.get_unchecked(..F::Index::SIZE * (len_minus_one as usize));
let things = slice.get_unchecked(F::Index::SIZE * (len_minus_one as usize)..);
Copy link
Member

Choose a reason for hiding this comment

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

Thought: you should use split_at_unchecked here, but I think it isn't MSRV yet

utils/zerovec/src/varzerovec/components.rs Show resolved Hide resolved
@Manishearth Manishearth force-pushed the remove-first-index-vzv branch from 94f05ab to 1cc0814 Compare October 1, 2024 08:24
@Manishearth Manishearth merged commit 0006d90 into unicode-org:main Oct 1, 2024
28 checks passed
@Manishearth Manishearth deleted the remove-first-index-vzv branch October 1, 2024 08:41
@Manishearth Manishearth mentioned this pull request Oct 2, 2024
Merged
Manishearth added a commit that referenced this pull request Oct 2, 2024
@Manishearth
Fix test of PackedPattern repr (#5636)
11ff589 
Caused by clash of #5620 and
#5601


<!--
Thank you for your pull request to ICU4X!

Reminder: try to use [Conventional
Comments](https://conventionalcomments.org/) to make comments clearer.

Please see
https://github.com/unicode-org/icu4x/blob/main/CONTRIBUTING.md for
general
information on contributing to ICU4X.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertbastian robertbastian robertbastian approved these changes

@sffc sffc sffc left review comments

@zbraniecki zbraniecki Awaiting requested review from zbraniecki zbraniecki is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the initial 0 length from VarZeroVec's representation
3 participants
@Manishearth @sffc @robertbastian