Remove some serializations in `MapView`. #3036

MathieuDutSik · 2024-12-16T20:35:44Z

Motivation

The performance of the MapView has recently emerged as a subject of concern. While the use of async Iterators is under discussion, it is clear that there are other issues.

Proposal

The following was done:

The enum type ValueOrBytes was introduced to represent the fact that in some cases we have the values and in some other cases its serialization.
The function for_each_key_value_while no longer serializes the values; it just creates a ValueOrBytes::Value.
The use of iterator() is replaced by a into_iterator_owned() since this avoids a .to_vec() in the construction of the ValueOrBytes::Bytes. Note that we cannot have a Bytes(&a [u8])since indeed the scope off` is larger than the one in which the key values are created.
Some Clone traits have to be added. But this should be viewed as a gain since before we were serializing and then deserializing, i.e. a very expensive clone.
The recently introduced functions index_values were needlessly complex. They are replaced by simpler code.

Possible criticism:

The functions for_each_key_value and for_each_key_value_while are kept even if their use is complex. This is not an issue as those functions are not used outside of the linera-views code. They could be made private.
Lack of benchmark. But I consider the elimination of serialization/deserialization sufficient as a win so as to not need additional justifications.

Test Plan

The CI should do the job.

Release Plan

The PR could be deployed on TestNet / DevNet as desired.

Links

None.

…() operation.

deuszx

Changes seem rather arbitrary - I don't see any explanation why we're doing that.

MathieuDutSik · 2024-12-17T09:14:36Z

Changes seem rather arbitrary - I don't see any explanation why we're doing that.

Serializing data and deserializing is a very inefficient method. Replacing by Clone is much better in terms of speed.

afck · 2024-12-17T09:45:56Z

I'm not sure about this: It considerably complicates the public API, and in almost all cases it's the values we want, not the bytes.
The only exception seems to be when we compute the hash. So maybe this optimization should be done only in a private function that can then be used in the implementation of for_each_key_value_while and hash?

afck · 2024-12-17T11:03:58Z

linera-views/src/views/map_view.rs

@@ -1279,7 +1367,7 @@ where
    {
        let prefix = Vec::new();
        self.map
-            .for_each_key_value(
+            .for_each_key_value_or_bytes(


Most of these can be just for_each_key_value, so we don't need the to_value call below.

Yes, thank you.

afck · 2024-12-17T12:01:07Z

linera-views/src/views/map_view.rs

@@ -776,7 +776,7 @@ where
    /// assert_eq!(count, 1);
    /// # })
    /// ```
-    pub(crate) async fn for_each_key_value_or_bytes<'a, F>(
+    pub async fn for_each_key_value_or_bytes<'a, F>(


So you want to make it public after all? I'd keep this and the ValueOrBytes type private.

Ok, changed.

ma2bd · 2024-12-17T12:07:44Z

linera-views/src/views/map_view.rs

        mut f: F,
        prefix: Vec<u8>,
    ) -> Result<(), ViewError>
    where
-        F: FnMut(&[u8], &[u8]) -> Result<bool, ViewError> + Send,
+        F: FnMut(&[u8], ValueOrBytes<'a, V>) -> Result<bool, ViewError> + Send,


This function is public API. Why would a user want to see the bytes?

Yes, wrong move. I wanted the test functions to be running, but that was a wrong idea.

ma2bd · 2024-12-17T12:18:12Z

I personally don't like the added code complexity with no performance gains to show for.
Are you trying to accelerate count? maybe there are better ways

ma2bd

back to your queue for the questions

ma2bd · 2024-12-17T13:49:30Z

linera-views/src/views/map_view.rs

            |index, value| {
                count += 1;
                hasher.update_with_bytes(index)?;
-                hasher.update_with_bytes(value)?;
+                let bytes = value.to_bytes()?;
+                hasher.update_with_bytes(&bytes)?;


Ok now I understand the motivation

ma2bd · 2024-12-17T13:52:25Z

linera-views/src/views/map_view.rs

+    T: Clone + DeserializeOwned,
+{
+    /// Convert to a value.
+    pub fn to_value(self) -> Result<T, ViewError> {


We could also use to_cow to avoid a clone

ma2bd · 2024-12-17T13:54:59Z

linera-views/src/views/map_view.rs

@@ -838,11 +920,12 @@ where
        let mut hasher = sha3::Sha3_256::default();
        let mut count = 0;


We should write 0u32 here. Otherwise the hashing scheme is not well defined

Oh yes, and this impacts other containers, and it makes the change TestNet/DevNet breaking.

linera-views/src/views/map_view.rs

ma2bd · 2024-12-17T13:58:32Z

linera-views/src/views/map_view.rs

@@ -67,6 +67,40 @@ pub struct ByteMapView<C, V> {
    updates: BTreeMap<Vec<u8>, Update<V>>,
 }

+/// Whether we have a value or its serialization.
+pub enum ValueOrBytes<'a, T> {


Still not private

MathieuDutSik added 5 commits December 16, 2024 20:13

Introduce the SerOrNot construction and simplify some code.

5b06e26

Some simplification.

93702f7

A better way to convert that simplifies the code.

bcd4e83

Switch to using the into_iterator_owned in order to avoid the .to_vec…

86d2308

…() operation.

Formatting correction.

fd913f5

MathieuDutSik marked this pull request as ready for review December 17, 2024 04:33

MathieuDutSik requested review from afck, deuszx and bart-linera December 17, 2024 04:34

deuszx reviewed Dec 17, 2024

View reviewed changes

deuszx approved these changes Dec 17, 2024

View reviewed changes

Introduce other function for the F(&[u8], V).

7ce5695

afck reviewed Dec 17, 2024

View reviewed changes

Further simplifications from Andreas Fackler.

e2026f4

afck approved these changes Dec 17, 2024

View reviewed changes

Some corrections in order to have the unit tests passing.

4cec035

afck reviewed Dec 17, 2024

View reviewed changes

ma2bd reviewed Dec 17, 2024

View reviewed changes

ma2bd requested changes Dec 17, 2024

View reviewed changes

afck self-requested a review December 17, 2024 12:19

MathieuDutSik added 2 commits December 17, 2024 13:26

Put back the functions "for_each_key_value_or_bytes" as private.

048baa3

Remove the "pub(crate)".

eb69fc6

MathieuDutSik merged commit f201d21 into linera-io:main Dec 17, 2024
23 checks passed

ma2bd reviewed Dec 17, 2024

View reviewed changes

linera-views/src/views/map_view.rs Show resolved Hide resolved

ma2bd reviewed Dec 17, 2024

View reviewed changes

MathieuDutSik mentioned this pull request Dec 17, 2024

Corrections to MapView after the introduction of ValueOrBytes. #3046

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some serializations in `MapView`. #3036

Remove some serializations in `MapView`. #3036

MathieuDutSik commented Dec 16, 2024 •

edited

Loading

deuszx left a comment

MathieuDutSik commented Dec 17, 2024

afck commented Dec 17, 2024

afck Dec 17, 2024

MathieuDutSik Dec 17, 2024

afck Dec 17, 2024

ma2bd Dec 17, 2024

MathieuDutSik Dec 17, 2024

ma2bd Dec 17, 2024 •

edited

Loading

MathieuDutSik Dec 17, 2024

ma2bd commented Dec 17, 2024

ma2bd left a comment

ma2bd Dec 17, 2024

MathieuDutSik Dec 17, 2024

ma2bd Dec 17, 2024

ma2bd Dec 17, 2024 •

edited

Loading

MathieuDutSik Dec 17, 2024

ma2bd Dec 17, 2024

		@@ -838,11 +920,12 @@ where
		let mut hasher = sha3::Sha3_256::default();
		let mut count = 0;

Remove some serializations in MapView. #3036

Remove some serializations in MapView. #3036

Conversation

MathieuDutSik commented Dec 16, 2024 • edited Loading

Motivation

Proposal

Test Plan

Release Plan

Links

deuszx left a comment

Choose a reason for hiding this comment

MathieuDutSik commented Dec 17, 2024

afck commented Dec 17, 2024

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

ma2bd Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2bd commented Dec 17, 2024

ma2bd 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

ma2bd Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove some serializations in `MapView`. #3036

Remove some serializations in `MapView`. #3036

MathieuDutSik commented Dec 16, 2024 •

edited

Loading

ma2bd Dec 17, 2024 •

edited

Loading

ma2bd Dec 17, 2024 •

edited

Loading