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.

The standard size is 4M for gRPC. But the MAX_PAYLOAD_SIZE if 4000000 which leaves room for additional small stuff to be put.

It should be fine for the most part. Although, it can be broken if one tries intentionally, but I'm sure never in any real-life, practical scenario.
I'll do some few tweaks if I ever feel otherwise.

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.

No, you should remove the // and add a comment to the run_reads function complexity. Indeed it is slow.
A test does not need to be super fast, but it should be testing useful functionality.

The code should not be cryptic and if there is an underlying reason that takes more than 5 minutes to understand then it should be written clearly.

Yeah, will do. I just wanted this to be known by others, for if they ever feel storage service tests are progressing slow.

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

MathieuDutSik

It is a nice progress but a little cryptic.

MathieuDutSik · 2025-02-24T16:51:47Z

linera-storage-service/src/server.rs

+    // Another way is to cast &self to raw pointers and use unsafe rust
+    // that also will completely be ok as tasks are either scoped, joining
+    // themselves at their functions end or else even then the raw pointers


We do not want to use unsafe code.

Yes, that's why chose the safe approach. This comment was just to let the others know.

MathieuDutSik · 2025-02-24T17:16:14Z

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),


No, you should remove the // and add a comment to the run_reads function complexity. Indeed it is slow.
A test does not need to be super fast, but it should be testing useful functionality.

The code should not be cryptic and if there is an underlying reason that takes more than 5 minutes to understand then it should be written clearly.

MathieuDutSik · 2025-02-24T17:22:10Z

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();


The standard size is 4M for gRPC. But the MAX_PAYLOAD_SIZE if 4000000 which leaves room for additional small stuff to be put.

MathieuDutSik · 2025-02-24T17:25:17Z

linera-storage-service/src/server.rs

+            let mut message = VecDeque::from(message);
+            while !message.is_empty() || state.get(2).is_some_and(|len| 0 == *len) {
+                match state.len() {
+                    0 => {


Given that the code 0, 1, 2 and 3 are used for coding some meaning we cannot use some enum.
But it would be preferable to put some comments.

Yes, it would be nice.

MathieuDutSik · 2025-02-24T17:34:33Z

linera-storage-service/src/server.rs

+                                let _ = state.pop();
+                                let _ = state.pop();
+                                let _ = state.pop();


It is a little unfortunate to have the 3 pops. But the alternativer with split_off or drain is not much simpler.

My purpose was to leave better readability and understanding of what's going, to help others visualize. Since the size of the state vector here is 3, vec.clear could have been used, but it would have left others finding themselves a little wondering and with some assumptions with manual state tracking. It's better now.

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

MathieuDutSik reviewed Feb 24, 2025

View reviewed changes

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

MathieuDutSik Feb 24, 2025

usagi32 Feb 24, 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

MathieuDutSik Feb 24, 2025

usagi32 Feb 24, 2025

afck Feb 19, 2025

usagi32 Feb 19, 2025

MathieuDutSik left a comment

MathieuDutSik Feb 24, 2025

usagi32 Feb 24, 2025

MathieuDutSik Feb 24, 2025

MathieuDutSik Feb 24, 2025

MathieuDutSik Feb 24, 2025 •

edited

Loading

usagi32 Feb 24, 2025

MathieuDutSik Feb 24, 2025

usagi32 Feb 24, 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

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

MathieuDutSik 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

MathieuDutSik Feb 24, 2025 • 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

MathieuDutSik Feb 24, 2025 •

edited

Loading