add versionize support for more data structures #37

ZizhengBian · 2022-04-18T07:48:56Z

add versionize support for VecDeque/HashMap/HashSet.

Signed-off-by: Zizheng Bian [email protected]

Reason for This PR

Added support for some of the commonly used data structures: VecDeque, HashMap, and HashSet

Description of Changes

Add versionize support for new types in primitives.rs.
Add Error arms for new types in lib.rs.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any user-facing changes are mentioned in CHANGELOG.md.

ZizhengBian · 2022-04-18T07:55:38Z

Hi @sandreim @berciuliviu @jiangliu This is the continue work of #25

src/primitives.rs

studychao · 2022-11-25T08:28:54Z

Hi @roypat @luminitavoicu @aghecenco Could you please take a look at this PR :)

roypat

Hi,
Sorry for the late response, we've all been pretty busy with a release the last few weeks. This look good to me, and I think @andreeaflorescu's concern regarding VecDequeue/Vec compatibility is addressed by Vec and VecDequeue having the same serialized format (e.g. the serializer "forgets" about the ring buffer structure of VecDequeue and just lays out the queue sequentially - which I think is what should happen, anyway).

Just a few instances where you forgot to replace T with $GenericParam when converting the Vec impl into a macro, can you fix those and rebase?

src/primitives.rs

ZizhengBian · 2022-12-06T03:13:02Z

Hi, Sorry for the late response, we've all been pretty busy with a release the last few weeks. This look good to me, and I think @andreeaflorescu's concern regarding VecDequeue/Vec compatibility is addressed by Vec and VecDequeue having the same serialized format (e.g. the serializer "forgets" about the ring buffer structure of VecDequeue and just lays out the queue sequentially - which I think is what should happen, anyway).

Just a few instances where you forgot to replace T with $GenericParam when converting the Vec impl into a macro, can you fix those and rebase?

Thanks, I fixed these problems.

But I think the CI envirnoment may broken @roypat :

roypat · 2022-12-07T11:58:23Z

oh my, thanks for letting me know, I'll have a look later today!

roypat · 2022-12-07T16:28:05Z

fixed the CI, it seems that after rebasing some of your tests started failing (when you wrote this PR, the project was using rust 1.54, we've since updated to having the CI run in 1.64, and some Display impls in the standard library changed along the way)

ZizhengBian · 2022-12-11T15:47:03Z

Thank you, and I have fixed the error code in test cases, but the CI still failed due to timeout, how can I trigger CI's retry logic? @roypat

roypat · 2022-12-12T09:17:55Z

Ugh, I think that might be related to rust-vmm/community#137

I increased the timeouts (I think? The tests all passed in under 5 minutes this time), so it seems the only thing left to address is the coverage. I had a look at the report and it seems the only thing uncovered are .map_err lines, so in my book just adjusting the target value here is enough

add versionize support for VecDeque/HashMap/HashSet. Signed-off-by: Zizheng Bian <[email protected]>

ZizhengBian · 2022-12-13T02:42:23Z

Hi @andreeaflorescu @luminitavoicu @aghecenco Could you please look at this PR :)

ZizhengBian · 2022-12-20T09:41:19Z

Hi @roypat , could you help me to find others who have permission to merge review this pr?

ShadowCurse · 2022-12-20T11:01:55Z

src/primitives.rs

+            fn serialize<W: std::io::Write>(
+                &self,
+                mut writer: &mut W,
+                version_map: &VersionMap,
+                app_version: u16,
+            ) -> VersionizeResult<()> {
+                if self.len() > MAX_VEC_SIZE / std::mem::size_of::<$GenericParam>() {
+                    return Err(VersionizeError::VecLength(self.len()));
+                }
+
+                // Serialize in the same fashion as bincode:
+                // Write len.
+                bincode::serialize_into(&mut writer, &self.len())
+                    .map_err(|ref err| VersionizeError::Serialize(format!("{:?}", err)))?;


Does writer needs to be marked as mut?
I see it is used for bincode::serialize_into(&mut writer, ..., but shouldn't just writer also work, because it is already a &mut W?

JonathanWoollett-Light · 2022-12-20T10:13:15Z

src/primitives.rs

+                bincode::serialize_into(&mut writer, &self.len())
+                    .map_err(|ref err| VersionizeError::Serialize(format!("{:?}", err)))?;


Suggested change

bincode::serialize_into(&mut writer, &self.len())

.map_err(|ref err| VersionizeError::Serialize(format!("{:?}", err)))?;

bincode::serialize_into(&mut writer, &self.len())

.map_err(|err| VersionizeError::Serialize(format!("{err:?}")))?;

JonathanWoollett-Light · 2022-12-20T10:14:43Z

src/primitives.rs

+                let len: usize = bincode::deserialize_from(&mut reader)
+                    .map_err(|ref err| VersionizeError::Deserialize(format!("{:?}", err)))?;


Suggested change

let len: usize = bincode::deserialize_from(&mut reader)

.map_err(|ref err| VersionizeError::Deserialize(format!("{:?}", err)))?;

let len: usize = bincode::deserialize_from(&mut reader)

.map_err(|err| VersionizeError::Deserialize(format!("{err:?}")))?;

JonathanWoollett-Light · 2022-12-20T10:19:12Z

src/primitives.rs

+                let mut v = Vec::with_capacity(len);
+
+                for _ in 0..len {
+                    let element: $GenericParam =
+                        $GenericParam::deserialize(reader, version_map, app_version).map_err(
+                            |ref err| VersionizeError::Deserialize(format!("{:?}", err)),
+                        )?;
+                    v.push(element);
+                }
+                Ok(v.into())


Suggested change

let mut v = Vec::with_capacity(len);

for _ in 0..len {

let element: $GenericParam =

$GenericParam::deserialize(reader, version_map, app_version).map_err(

|ref err| VersionizeError::Deserialize(format!("{:?}", err)),

)?;

v.push(element);

}

Ok(v.into())

let vec = (0..len).map(|_|

$GenericParam::deserialize(reader, version_map, app_version).map_err(

|err| VersionizeError::Deserialize(format!("{err:?}")),

)

).collect::<Result<Vec<_>,_>>()?;

vec.map($VecType::from)

JonathanWoollett-Light · 2022-12-20T10:32:22Z

src/primitives.rs

@@ -281,9 +288,75 @@ where
    }
 }

-impl<T> Versionize for Vec<T>
+macro_rules! impl_versionize_vec_like_type {
+    ($VecType:ident <$GenericParam:ident>) => {


Given the invocations are with Vec<T> and VecDeque<T> we can remove the generic parameter from the macro just using T within it.

Suggested change

($VecType:ident <$GenericParam:ident>) => {

($VecType:ident) => {

and changing the invocations to:

impl_versionize_vec_like_type!(Vec); impl_versionize_vec_like_type!(VecDeque);

JonathanWoollett-Light · 2022-12-20T10:34:54Z

src/primitives.rs

+impl<T: Default + FamStruct + Versionize> Versionize for FamStructWrapper<T>
 where
-    T: Versionize,
+    <T as FamStruct>::Entry: Versionize,
+    T: std::fmt::Debug,


We should define traits on T either entirely within impl<T: ...> or within where T: ...,.

Suggested change

impl<T: Default + FamStruct + Versionize> Versionize for FamStructWrapper<T>

where

T: Versionize,

<T as FamStruct>::Entry: Versionize,

T: std::fmt::Debug,

impl<T> Versionize for FamStructWrapper<T>

where

<T as FamStruct>::Entry: Versionize,

T: std::fmt::Debug + Default + FamStruct + Versionize,

JonathanWoollett-Light · 2022-12-20T11:42:05Z

src/primitives.rs

+        assert_eq!(
+            format!("{}", err),
+            "HashMap of length exceeded 20971536 > 20971520 bytes"
+        );


Suggested change

assert_eq!(

format!("{}", err),

"HashMap of length exceeded 20971536 > 20971520 bytes"

);

assert_eq!(

err.to_string(),

"HashMap of length exceeded 20971536 > 20971520 bytes"

)

JonathanWoollett-Light · 2022-12-20T11:43:42Z

src/primitives.rs

+        let mut store = HashSet::new();
+        store.insert("test 1".to_owned());
+        store.insert("test 2".to_owned());
+        store.insert("test 3".to_owned());


Suggested change

let mut store = HashSet::new();

store.insert("test 1".to_owned());

store.insert("test 2".to_owned());

store.insert("test 3".to_owned());

let store = HashSet::from([

String::from("test 1"),

String::from("test 2"),

String::from("test 3")

]);

JonathanWoollett-Light · 2022-12-20T11:44:12Z

src/primitives.rs

+        let mut hash_set: HashSet<u32> = HashSet::new();
+        hash_set.insert(1);
+        hash_set.insert(2);
+        hash_set.insert(3);


Suggested change

let mut hash_set: HashSet<u32> = HashSet::new();

hash_set.insert(1);

hash_set.insert(2);

hash_set.insert(3);

let hash_set = HashSet::from([1,2,3]);

JonathanWoollett-Light · 2022-12-20T11:46:52Z

src/primitives.rs

+        assert_eq!(
+            format!("{}", err),
+            "HashSet of length exceeded 10485768 > 10485760 bytes"
+        );


Suggested change

assert_eq!(

format!("{}", err),

"HashSet of length exceeded 10485768 > 10485760 bytes"

);

assert_eq!(

err.to_string(),

"HashSet of length exceeded 10485768 > 10485760 bytes"

);

JonathanWoollett-Light · 2022-12-20T11:49:06Z

src/primitives.rs

+        let mut err = HashSet::with_capacity(MAX_HASH_SET_LEN / 8 + 1);
+        for i in 0..(MAX_HASH_SET_LEN / 8 + 1) {
+            err.insert(i);
+        }


Suggested change

let mut err = HashSet::with_capacity(MAX_HASH_SET_LEN / 8 + 1);

for i in 0..(MAX_HASH_SET_LEN / 8 + 1) {

err.insert(i);

}

let err = (0..(MAX_HASH_SET_LEN / 8 + 1)).collect::<HashSet<_>>();

ZizhengBian force-pushed the jason/add-new-primitives branch 2 times, most recently from b884fa8 to 1802fcb Compare April 18, 2022 07:49

imaginezz mentioned this pull request Apr 18, 2022

add versionize for VecDequeue/HashMap/HashSet. #25

Closed

6 tasks

ZizhengBian force-pushed the jason/add-new-primitives branch 2 times, most recently from 503521d to 398dbd1 Compare April 18, 2022 11:49

raduiliescu requested a review from a team May 3, 2022 09:14

andreeaflorescu reviewed Nov 16, 2022

View reviewed changes

src/primitives.rs Show resolved Hide resolved

ZizhengBian force-pushed the jason/add-new-primitives branch 2 times, most recently from 7064e6b to 1275b64 Compare November 16, 2022 15:24

roypat reviewed Dec 5, 2022

View reviewed changes

src/primitives.rs Outdated Show resolved Hide resolved

src/primitives.rs Outdated Show resolved Hide resolved

src/primitives.rs Outdated Show resolved Hide resolved

src/primitives.rs Outdated Show resolved Hide resolved

ZizhengBian force-pushed the jason/add-new-primitives branch 2 times, most recently from ce133f5 to 8c514c6 Compare December 6, 2022 03:07

ZizhengBian requested review from roypat and andreeaflorescu and removed request for roypat and andreeaflorescu December 7, 2022 11:39

ZizhengBian force-pushed the jason/add-new-primitives branch from 8c514c6 to 53a2069 Compare December 9, 2022 13:20

add versionize support for more data structures

29bfa55

add versionize support for VecDeque/HashMap/HashSet. Signed-off-by: Zizheng Bian <[email protected]>

ZizhengBian force-pushed the jason/add-new-primitives branch from 53a2069 to 29bfa55 Compare December 12, 2022 13:32

roypat approved these changes Dec 12, 2022

View reviewed changes

ShadowCurse reviewed Dec 20, 2022

View reviewed changes

JonathanWoollett-Light reviewed Dec 20, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add versionize support for more data structures #37

add versionize support for more data structures #37

ZizhengBian commented Apr 18, 2022 •

edited

Loading

ZizhengBian commented Apr 18, 2022 •

edited

Loading

studychao commented Nov 25, 2022

roypat left a comment

ZizhengBian commented Dec 6, 2022 •

edited

Loading

roypat commented Dec 7, 2022

roypat commented Dec 7, 2022

ZizhengBian commented Dec 11, 2022

roypat commented Dec 12, 2022

ZizhengBian commented Dec 13, 2022

ZizhengBian commented Dec 20, 2022 •

edited

Loading

ShadowCurse Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

JonathanWoollett-Light Dec 20, 2022

		bincode::serialize_into(&mut writer, &self.len())
		.map_err(\|ref err\| VersionizeError::Serialize(format!("{:?}", err)))?;

		let len: usize = bincode::deserialize_from(&mut reader)
		.map_err(\|ref err\| VersionizeError::Deserialize(format!("{:?}", err)))?;

	($VecType:ident <$GenericParam:ident>) => {
	($VecType:ident) => {

add versionize support for more data structures #37

Are you sure you want to change the base?

add versionize support for more data structures #37

Conversation

ZizhengBian commented Apr 18, 2022 • edited Loading

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

ZizhengBian commented Apr 18, 2022 • edited Loading

studychao commented Nov 25, 2022

roypat left a comment

Choose a reason for hiding this comment

ZizhengBian commented Dec 6, 2022 • edited Loading

roypat commented Dec 7, 2022

roypat commented Dec 7, 2022

ZizhengBian commented Dec 11, 2022

roypat commented Dec 12, 2022

ZizhengBian commented Dec 13, 2022

ZizhengBian commented Dec 20, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZizhengBian commented Apr 18, 2022 •

edited

Loading

ZizhengBian commented Apr 18, 2022 •

edited

Loading

ZizhengBian commented Dec 6, 2022 •

edited

Loading

ZizhengBian commented Dec 20, 2022 •

edited

Loading