Implement and consolidate VarZeroVec mutation operations #1127

kupiakos · 2021-09-30T18:11:55Z

This consolidates all bit-shifting mutation operations into a single
function, since there's significant shared logic.

This consolidates all bit-shifting mutation operations into a single function, since there's significant shared logic. Fixes unicode-org#1080.

jira-pull-request-webhook · 2021-09-30T18:20:06Z

Notice: the branch changed across the force-push!

utils/zerovec/src/varzerovec/owned.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Manishearth

Overall looks really good! I mostly would like to see some more comments around the unsafe stuff before merging, but otherwise this is close to ready.

Note on our process: We try to push new commits each time so that it's easy to review, and everything gets squashed at the end when we merge, so just make commits as you make progress instead of amending or squashing.

Manishearth · 2021-09-30T19:58:00Z

utils/zerovec/src/varzerovec/owned.rs

@@ -90,6 +97,51 @@ impl<T: AsVarULE> VarZeroVecOwned<T> {
        self.get_components().get(idx)
    }

+    /// Get the position of a specific element in the data segment.


nit: document the safety invariant here in a ## Safety section

(ditto on the other functions)

in particular, there are probably valid and invalid states for entire_slice to be in here, because e.g. this relies on entire_slice being the appropriate length

I think for most of these functions entire_slice has to be entirely valid, except for index_data() where only the indices array needs to be in the right position (but may have incorrect indices) and set_len() where nothing needs to be valid

Done, not sure how strictly worded the safety section needs to be here

I think we can include something about how entire_slice() needs to be well-formed. Because most of the time we're operating on transitional states of entire_slice(), these methods can't be called then.

For example, in shift_indices() it's actually valid to call that on a buffer with a malformed index segment, but it must have the correct length set (and that's when we call it).

I'm not sure how much better I can do here without precisely explaining all invariants of the slice where the helper functions are used.

utils/zerovec/src/varzerovec/owned.rs

Manishearth · 2021-09-30T20:29:49Z

utils/zerovec/src/varzerovec/owned.rs

+        } else {
+            let idx_offset = 4 + idx * 4;
+            let idx_slice = &self.entire_slice[idx_offset..idx_offset + 4];
+            u32::from_unaligned(&PlainOldULE::<4>::from_byte_slice_unchecked(idx_slice)[0]) as usize


nit: can we use index_data() in this branch?

I can, though it requires making both this and element_range_unchecked &mut self.

Alternatively, I made index_data and index_data_mut.

Ah, never mind then. Making two functions seems okay but the current code is also okay, mostly wanted to reduce duplication if possible

utils/zerovec/src/varzerovec/owned.rs

Manishearth · 2021-09-30T21:39:00Z

utils/zerovec/src/varzerovec/owned.rs

+            if shift_type == ShiftType::Remove {
+                // Move the data before the element back by 4 to remove the index.
+                debug_assert!(prev_element_p.start >= index_range.end);
+                ptr::copy(


thought: might be worth having a shift_bytes(Range<*const u8>, *mut u8) helper given that all of the uses of ptr::copy do the same kind of thing here (ptr::copy(x, y, z.offset_from(x)) ==> shift_bytes(x..z, y))

Manishearth · 2021-09-30T21:41:27Z

utils/zerovec/src/varzerovec/owned.rs

+                slice_range.end.offset_from(prev_element_p.end) as usize,
+            );
+
+            match shift_type {


thought: The actual variable this gets set to is further off, I wonder if this would help?

Suggested change

match shift_type {

/* let first_affected_index = */ match shift_type {

I instead moved the users of the variable into the block. I think it ends up cleaner.

utils/zerovec/src/varzerovec/owned.rs

Manishearth · 2021-09-30T21:49:18Z

utils/zerovec/src/varzerovec/owned.rs

+    // Invalidate and resize the data at an index, optionally inserting or removing the index.
+    // Also updates affected indices and the length.
+    // Returns a slice to the new element data - it doesn't contain uninitialized data but its value is indeterminate.
+    unsafe fn shift(&mut self, index: usize, new_size: u32, shift_type: ShiftType) -> &mut [u8] {


praise: this is a really good refactoring

Thank you! It was a fun puzzle :)

Adds a new test, clears the slice on last element removal, and adds an invariant testing function.

kupiakos · 2021-10-01T07:49:05Z

utils/zerovec/src/varzerovec/owned.rs

+    ///
+    /// Note: an index is valid if it doesn't point to data past the end of the slice and is
+    /// less than or equal to all future indices. The length of the index segment is not part of each index.
+    fn verify_integrity(&self) -> bool {


Debating if this is too overboard or if it should just depend on SliceComponents::parse_byte_slice

Manishearth

one small bit of improvements on the safety doc comments and we should be good to go!

Manishearth · 2021-10-01T07:41:52Z

utils/zerovec/src/varzerovec/owned.rs

@@ -90,6 +97,51 @@ impl<T: AsVarULE> VarZeroVecOwned<T> {
        self.get_components().get(idx)
    }

+    /// Get the position of a specific element in the data segment.


I think we can include something about how entire_slice() needs to be well-formed. Because most of the time we're operating on transitional states of entire_slice(), these methods can't be called then.

For example, in shift_indices() it's actually valid to call that on a buffer with a malformed index segment, but it must have the correct length set (and that's when we call it).

utils/zerovec/src/varzerovec/owned.rs

Manishearth · 2021-10-02T07:04:46Z

Ah, make sure to test with cargo test --all-features, it's failing a map test.

Manishearth · 2021-10-02T07:04:58Z

I feel like it was passing CI earlier, though.

Manishearth · 2021-10-02T07:16:56Z

Ah, a simpler repro would be constructing a VarZeroVec by pushing in strings that are three or fewer characters in length: We call self.index_data_mut() while it's invalid to call since it expects the entire_slice array to actually be the right size, which it isn't yet.

Manishearth · 2021-10-02T07:26:54Z

Pushed up a fix

kupiakos · 2021-10-02T07:39:09Z

We call self.index_data_mut() while it's invalid to call since it expects the entire_slice array to actually be the right size, which it isn't yet.

Ahh good catch, thanks for fixing

Manishearth · 2021-10-02T07:48:34Z

Thanks for working on this!

…#1127) * Implement and consolidate VarZeroVec mutation operations This consolidates all bit-shifting mutation operations into a single function, since there's significant shared logic. Fixes unicode-org#1080. Co-authored-by: Manish Goregaokar <manishsmail@gmail.com>

kupiakos requested review from Manishearth and sffc as code owners September 30, 2021 18:11

Implement and consolidate VarZeroVec mutation operations

c8739ca

This consolidates all bit-shifting mutation operations into a single function, since there's significant shared logic. Fixes unicode-org#1080.

kupiakos force-pushed the main branch from 9dbc7b1 to c8739ca Compare September 30, 2021 18:20

Manishearth previously approved these changes Sep 30, 2021

View reviewed changes

Improve the implementation of mutating varzerovec

43031e3

Adds a new test, clears the slice on last element removal, and adds an invariant testing function.

kupiakos dismissed Manishearth’s stale review via 43031e3 October 1, 2021 07:36

kupiakos commented Oct 1, 2021

View reviewed changes

Make verify_integrity slightly more correct

64504a8

kupiakos dismissed Manishearth’s stale review via 64504a8 October 1, 2021 07:53

Manishearth previously approved these changes Oct 1, 2021

View reviewed changes

kupiakos added 2 commits October 1, 2021 23:14

Add a little more about well-formed slices

7d2d47f

Also update shift_indices() invariant

65e67f6

Manishearth previously approved these changes Oct 2, 2021

View reviewed changes

Fix up index_data_mut()

290d0b5

Manishearth dismissed their stale review via 290d0b5 October 2, 2021 07:26

Manishearth previously approved these changes Oct 2, 2021

View reviewed changes

fix clippy

8ca8ffe

Manishearth dismissed their stale review via 8ca8ffe October 2, 2021 07:30

Manishearth approved these changes Oct 2, 2021

View reviewed changes

Manishearth merged commit e6245c2 into unicode-org:main Oct 2, 2021

Manishearth mentioned this pull request Oct 3, 2021

Remove AsVarULE #1126

Merged

sffc removed their request for review October 18, 2021 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement and consolidate VarZeroVec mutation operations #1127

Implement and consolidate VarZeroVec mutation operations #1127

kupiakos commented Sep 30, 2021

jira-pull-request-webhook bot commented Sep 30, 2021

Manishearth left a comment

Manishearth Sep 30, 2021

Manishearth Sep 30, 2021

Manishearth Sep 30, 2021

kupiakos Oct 1, 2021

Manishearth Oct 1, 2021 •

edited

Loading

kupiakos Oct 2, 2021

Manishearth Sep 30, 2021

kupiakos Oct 1, 2021

kupiakos Oct 1, 2021

Manishearth Oct 1, 2021

Manishearth Sep 30, 2021

kupiakos Oct 1, 2021

Manishearth Sep 30, 2021

kupiakos Oct 1, 2021 •

edited

Loading

Manishearth Sep 30, 2021

kupiakos Oct 1, 2021

kupiakos Oct 1, 2021

Manishearth left a comment

Manishearth Oct 1, 2021 •

edited

Loading

Manishearth commented Oct 2, 2021

Manishearth commented Oct 2, 2021

Manishearth commented Oct 2, 2021

Manishearth commented Oct 2, 2021

kupiakos commented Oct 2, 2021

Manishearth commented Oct 2, 2021

	match shift_type {
	/* let first_affected_index = */ match shift_type {

Implement and consolidate VarZeroVec mutation operations #1127

Implement and consolidate VarZeroVec mutation operations #1127

Conversation

kupiakos commented Sep 30, 2021

jira-pull-request-webhook bot commented Sep 30, 2021

Manishearth 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

Manishearth Oct 1, 2021 • edited Loading

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

kupiakos Oct 1, 2021 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Manishearth Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

Manishearth commented Oct 2, 2021

Manishearth commented Oct 2, 2021

Manishearth commented Oct 2, 2021

Manishearth commented Oct 2, 2021

kupiakos commented Oct 2, 2021

Manishearth commented Oct 2, 2021

Manishearth Oct 1, 2021 •

edited

Loading

kupiakos Oct 1, 2021 •

edited

Loading

Manishearth Oct 1, 2021 •

edited

Loading