feat: remove chunking from rpc #6345
Conversation
ghpbot-tari-project
added
P-acks_required
Test Results (CI) 3 files 120 suites 35m 58s ⏱️ Results for commit cc3258d. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files + 2 11 suites +11 46m 9s ⏱️ + 46m 9s For more details on these failures, see this check. Results for commit cc3258d. ± Comparison against base commit 1d6e0d8. ♻️ This comment has been updated with latest results. |
| proto::rpc::RpcResponse { | ||
| request_id, | ||
| status: message.status.as_u32(), | ||
| flags: message.flags.bits().into(), | ||
| payload: message.payload.to_vec(), | ||
| } |
There was a problem hiding this comment.
I didn't notice the message.to_proto() function before. That would be better to use than this
| proto::rpc::RpcResponse { | |
| request_id, | |
| status: message.status.as_u32(), | |
| flags: message.flags.bits().into(), | |
| payload: message.payload.to_vec(), | |
| } | |
| message.to_proto() |
comms/core/src/protocol/rpc/mod.rs
Outdated
| @@ -33,30 +33,12 @@ mod test; | |||
| pub const RPC_MAX_FRAME_SIZE: usize = 3 * 1024 * 1024; // 3 MiB | |||
| /// Maximum number of chunks into which a message can be broken up. | |||
| const RPC_CHUNKING_MAX_CHUNKS: usize = 16; // 16 x 256 Kib = 4 MiB max combined message size | |||
hansieodendaal
force-pushed
the
ho_remove_chunks
branch
from
a509416 to
4128be0
Compare
ghpbot-tari-project
removed
the
P-reviews_required
|
Not sure what to do with the failing |
The problem was that the "payload too large" message wasn't being constructed properly and, without chunking, the max frame size should be >= max response size. I pushed fixes for these |
ghpbot-tari-project
added
the
P-reviews_required
ghpbot-tari-project
removed
the
P-reviews_required
Removed chunking from rpc as this does not improve latency or throughput.
hansieodendaal
force-pushed
the
ho_remove_chunks
branch
from
5d9e060 to
5b6b78c
Compare
ghpbot-tari-project
added
the
P-reviews_required
Oops, did not see that. Working on it now. |
No problem :) readded the fix |
ghpbot-tari-project
removed
the
P-reviews_required
Description --- - remove chunking from RPC protocol Motivation and Context --- Chunking does not add any benefit especially when not using tor. Porting over changes from L1: tari-project/tari#6345 How Has This Been Tested? --- Manually What process can a PR reviewer use to test or verify this change? --- Should work as before Breaking Changes --- - [ ] None - [ ] Requires data directory to be deleted - [x] Other - Please specify BREAKING CHANGE: Any responses which previously triggered chunking (>= 256kb) would not be readable by clients using this PR
Description
Removed chunking from RPC as this does not improve latency or throughput and adds code complexity.
Motivation and Context
See above.
How Has This Been Tested?
System-level tests were conducted syncing a fresh archival base node in a controlled environment over Gigabit wired LAN where only one other base node was connected. The blockchain had many full blocks and many UTXOs, as a result of a 24-hour stress test.
Header sync
No difference could be seen with header sync, as expected, due to the small message sizes.
Block sync
Blocks throughput increased during full block sync for both coins-split and normal transactions due to reduced latencies (data transfer time); up to 8% increased block throughput was measured when blocks were filled with normal transactions.
What process can a PR reviewer use to test or verify this change?
Review code changes and test data, or perform a similar comparative test.
Breaking Changes