Use gRPC streams for linera storage service. #3346

usagi32 · 2025-02-16T06:03:21Z

addresses #1793

usagi32 · 2025-02-16T06:19:53Z

…nto linera-storage-service-grpc-streams

afck · 2025-02-19T10:35:57Z

linera-storage-service/proto/key_value_store.proto

@@ -53,7 +32,7 @@ message RequestContainsKeys {
 }

 message ReplyContainsKeys {
-  repeated bool tests = 1;
+  repeated bool tests = 1 [packed = true];


Isn't packed the default anyway?

Yes, its in proto3, it was not in proto2.

afck · 2025-02-19T10:40:09Z

linera-storage-service/src/client.rs

+            if value.is_none() {
+                return Ok(None);
+            }
+
+            reformed_value.append(&mut value.unwrap());


We can avoid the unwrap (and why the &mut?):

Suggested change

if value.is_none() {

return Ok(None);

}

reformed_value.append(&mut value.unwrap());

let Some(value) = value else {

return Ok(None);

};

reformed_value.append(value);

Also, should we log a warning if one of the values is None? That isn't expected behavior, I think?

Append takes an exclusive reference?
No, its fine. None is actually part of the message optional field. If key is not found None is send and we return from it.

We'd just like to avoid unwrap wherever it's easily possible. if let else works just as well here, without the unwrap.

Right, the append could probably be replaced with extend.

We'd just like to avoid unwrap wherever it's easily possible. if let else works just as well here, without the unwrap.

Actually, was talking about the need to log the None. It's fine and expected.

Right, the append could probably be replaced with extend.

That would be quite bad, extend will iterate move things on the stack for each element, append moves things in bulk in-place on the heap. It will be like O(1) vs O(n), where n is at least 6 orders of magnitude greater. Sometimes the compiler inlines and optimizes it away so both are same, but let's not rely on that.

afck · 2025-02-19T10:44:34Z

linera-storage-service/src/client.rs

+        let mut chunk_size = 0;
+        for full_key in full_keys {
+            if MAX_PAYLOAD_SIZE >= full_key.len() + chunk_size {
+                chunk_size += full_key.len();


Technically the chunk size probably also contains the encoded length of the full key, and then the encoded length of grouped_keys?

Not sure if MAX_PAYLOAD_SIZE leaves enough room for that or if we should add them here? (@MathieuDutSik)

(Also below.)

Actually, gRPC proto encodes a little more than even the length.
There were two extreme cases

keys are closer to MAX_KEY_SIZE, then there is a lot more than enough room in MAX_PAYLOAD_SIZE.

keys are small like 10 bytes, then there will be like hundreds of thousands of keys to test before the limit, even in synthetic tests we are not doing that.

Both are unrealistic cases, and there will performance and efficiency tradeoffs. Extra encodings are also variable for each element as non primitive types are not packed together. Its like tag + length + data where tag can be anything from 1 to 5 bytes depending on the field number and same for length.
Just a simple buffer of like 10-20 bytes will dominate the size of very small keys.

Let me know.

afck · 2025-02-19T10:47:27Z

linera-storage-service/src/client.rs

+            }
+        };
+
+        let _ = std::thread::spawn(spawner);


Is it really worth doing that in a separate thread? There's no heavy computation going on in there.

We have to get to the receiver at the end of functions body to take the advantages of streaming for switching the compute bound and IO bound operations. Preprocessing should be avoided as possible.
I just did that as there was no future to be awaited in the closure.
It just hit me that the receiver will likely be awaited by the tonic, we can just do this like a normal future.

afck · 2025-02-19T10:51:28Z

linera-storage-service/src/client.rs

+
+                        if 0 == num_messages {
+                            values.push(Some(value));
+                            value = Vec::new();


I'd rather declare value and num_messages at the beginning of the while loop's body.

Aren't they declared at beginning only?
Its just reassignment of the binding after moving the value.

I mean after the while, inside the outer loop's body: That would remove the need to reset it. I'd also find it a bit more readable, because it conceptually tracks the progress of the inner loop.

Actually, it needs to be persisted across the while loop. You may refer to the server module.

afck · 2025-02-19T10:58:23Z

linera-storage-service/src/client.rs

-            self.read_entries(message_index, num_chunks).await
+
+        let is_zero = |buf: &[u8]| {
+            let (prefix, aligned, suffix) = unsafe { buf.align_to::<u128>() };


Let's avoid unsafe.
Why not keep using repeated KeyValue?

Actually I was going to remove the unsafe, it was for me just during the testing as its just a very fast way to check if slice is all 0s. Code should do fine removing the condition using it.
repeated KeyValue will give unnecessary complexity to the code and will degrade the performance and efficiency.
Best way to approach it serialize the whole thing mimicking bcs, and de-serialize at the other end on-the-fly.

afck · 2025-02-19T11:01:07Z

linera-views/src/test_utils/mod.rs

        get_random_key_values2(30, 4, 10),
        get_random_key_values2(30, 4, 100),
+        //get_random_key_values2(10, 200, 3_999_700),


Does that value have a particular significance?
Anyway, let's either remove the line or the //.

Large key lengths or number of keys are exponentially slow in the tests. Likely due to the way run_reads is designed.
Just left this for the thoughts.

afck · 2025-02-19T11:04:21Z

linera-storage-service/Cargo.toml

@@ -19,7 +19,7 @@ test = ["linera-views/test"]

 [[bin]]
 name = "linera-storage-server"
-path = "src/server.rs"
+path = "src/main.rs"


It was quite handy for me during the testing. We get a server library function we can spawn any time to perform tests using #[tokio::test] without building everything.

…nto linera-storage-service-grpc-streams

Use gRPC streams for linera storage service.

9a901c2

usagi32 added 3 commits February 18, 2025 01:54

Merge branch 'main' of https://github.com/linera-io/linera-protocol i…

3758f46

…nto linera-storage-service-grpc-streams

corrections

83f7666

small change

3f48878

afck reviewed Feb 19, 2025

View reviewed changes

afck requested a review from MathieuDutSik February 19, 2025 11:08

usagi32 added 3 commits February 22, 2025 05:18

Merge branch 'main' of https://github.com/linera-io/linera-protocol i…

884cc5f

…nto linera-storage-service-grpc-streams

corrections

ff33da4

few remaining changes

b3f7dc3

usagi32 requested a review from afck February 22, 2025 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use gRPC streams for linera storage service. #3346

Use gRPC streams for linera storage service. #3346

usagi32 commented Feb 16, 2025

usagi32 commented Feb 16, 2025

afck Feb 19, 2025

usagi32 Feb 19, 2025

afck Feb 19, 2025

usagi32 Feb 19, 2025

afck Feb 20, 2025

usagi32 Feb 22, 2025

afck Feb 19, 2025

usagi32 Feb 19, 2025

afck Feb 19, 2025

usagi32 Feb 19, 2025

afck Feb 19, 2025

usagi32 Feb 19, 2025

afck Feb 20, 2025

usagi32 Feb 22, 2025

afck Feb 19, 2025

usagi32 Feb 19, 2025

afck Feb 19, 2025

usagi32 Feb 19, 2025

afck Feb 19, 2025

usagi32 Feb 19, 2025

Use gRPC streams for linera storage service. #3346

Are you sure you want to change the base?

Use gRPC streams for linera storage service. #3346

Conversation

usagi32 commented Feb 16, 2025

usagi32 commented Feb 16, 2025

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

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