feat(code): limit the number of values in a sync response based on upper limit

[insumity]

Closes: #1164

This PR is the extension of #1171 that I do not have access to update anymore. Look at all the commits after Re-request range from any peer, not necessarily the same one to see the changes.

Testing
To see that indeed max_response_size is working, we ran the response_size_limit_exceeded test against the main branch with the following params:

    test.build()
        .run_with_params(
            Duration::from_secs(60),
            TestParams {
                enable_value_sync: true,
                // Values are around ~900 bytes, so we canNOT have 2 values in one batch.
                rpc_max_size: ByteSize::b(1000),
                batch_size: 2,
                parallel_requests: 1,
                ..Default::default()
            },
        )
        .await

in which case the test fails because it tries to send 2 values at once something that cannot be done because rpc_max_size is set to 1000 bytes.

---

PR author checklist

For all contributors

• Reference a GitHub issue
• Ensure the PR title follows the conventional commits spec
• Add a release note in RELEASE_NOTES.md if the change warrants it
• Add an entry in BREAKING_CHANGES.md if the change warrants it

For external contributors

• Maintainers of Malachite are allowed to push changes to the branch

[hvanz]

Amazing work, thanks @insumity!

As noted by @jmalicevic there will a performance hit for the extra encoding. Have you seen any performance decrease in your tests? Maybe a small benchmark test would be useful. As a solution, the encoded value could be cached somewhere but this will take some refactoring (for another PR). I'm not worried about the extra communication (GetResponseSize) between actors; this is just a couple of function calls.

[hvanz]

I understand the original request is not in the wire (that is, its response was received) but it's still in state.pending_requests. At least for the received range that is being processed. Is this correct? Then, after the new request issued here, there will be max_parallel_requests + 1 active requests in state.pending_requests. I'm not sure if this is a problem, but two requests will need to be removed from state.pending_requests before a new one can be issued.

[jmalicevic]

This is a good point, although I think in practice the behavior stays the same.
For example you have state.pending_requests = {req1[1-10],req2[11-20]} . You split 1-10 because the range is too big and get : state.pending_requests = {req1[1-3],req2[11-20], req3[4-10]}
The problem is that when req2 is done, you cannot issue another request because you are not done processing req1 and req3 . If we compare this to how it was before batching , we are making things slightly worse by not allowing req4 until we process everything from the original req1.

However, in practice this delay is probably not very big compared to the initial use case. In any case, I would not block this PR on this optimization so that people can integrate with the API changes and we can then optimize this under the hood.

WDYT @hvanz @insumity ?

[insumity]

Thanks for this @jmalicevic. Yes, what you describe is my view as well.

[hvanz]

Yes, I think we can live that. I'm thinking the extreme scenario where blocks are so big that they fill a whole response message. In the example, req1[1-10] would be split in 10 requests. I think this is not something that we should try to fix here. That would either be a problem in the configuration, or the whole setup is not well thought out. Maybe we could add a warning when state.pending_requests.len() > state.max_parallel_requests, to see in the logs when this case is happening very often.

[insumity]

Maybe we could add a warning when state.pending_requests.len() > state.max_parallel_requests, to see in the logs when this case is happening very often.

Added some logging here based on your comment.

[insumity]

Thanks @hvanz for your comments!

As noted by @jmalicevic there will a performance hit for the extra encoding. Have you seen any performance decrease in your tests? Maybe a small benchmark test would be useful. As a solution, the encoded value could be cached somewhere but this will take some refactoring (for another PR). I'm not worried about the extra communication (GetResponseSize) between actors; this is just a couple of function calls.

I did not see any performance decrease in the tests but that's because the block_size is pretty small (i.e., ~900 bytes per value). I have a PR on the informalsystems repo where I can tune block_size in tests and noticed the following rough numbers:

• for a single value of 1 MB of random data, the ProtobufCodec encoding takes around ~50 µs;
• for a value of 10 MB, encoding takes ~700 µs;
• for a value of 25 MB, encoding takes ~1.5 ms;
• for a value of 50 MB, encoding takes ~3 ms.

In any case, I changed the code so we can retrieve the encoded length without actually having to encode the data. So, now, computing the encoded length takes a few tens of microseconds irrespectively of the value size. Note that this only holds for the ProtobufCodec and not for the JsonCodec.
Update: I performed some slightly more serious benchmarking and the results can be found here. To encode and then get the length of a 100MB value we need ~5.5 ms but to get immediately the length of the encoded value using encoded_len we just need 85 ns. Also verified that encoded(sth).len() == encoded_len(sth).
Let me know if you want me to add the benchmark code in this PR.

Please do let me know what you think!

[hvanz]

Thanks for the benchmarks! They look pretty good. 10Mb is already quite big for a value and would probably reach the typical size limit of a response, so I think we don't need to worry for 700 µs.

[insumity]

Thanks for the benchmarks! They look pretty good. 10Mb is already quite big for a value and would probably reach the typical size limit of a response, so I think we don't need to worry for 700 µs.

@hvanz So, we're okay with encoding the data and then getting the length as was initial done? If that's the case, I can remove this commit.

[hvanz]

So, we're okay with encoding the data and then getting the length as was initial done? If that's the case, I can remove this commit.

@romac WDYT? Do we need this optimization? Encoding a value of 1Mb takes about 50 µs and for 10 MB it takes ~700 µs.

[jmalicevic]

@romac @hvanz , are we ok to merge this? Should we unblock in our fork if you don't have capacity?

[jmalicevic]

@romac @hvanz we have merged this in our fork and also added a follow up PR fixing a few other issues we found in the sync : informalsystems#15

The PR is long, happy to upstream it if you deem its useful.

[romac]

@romac @hvanz , are we ok to merge this? Should we unblock in our fork if you don't have capacity?

Apologies, I completely missed your question…

@romac @hvanz we have merged this in our fork and also added a follow up PR fixing a few other issues we found in the sync: informalsystems#15

Glad to hear you took the initiative of merging it in your fork and my apologies again for not getting to this sooner.

Unfortunately, we will likely not be able to merge this before a week or so.

The PR is long, happy to upstream it if you deem its useful.

That would be awesome if it's not too much to ask, otherwise I am happy to upstream it myself from your fork. We can also wait until this PR is merged though, to avoid stacking them.

[jmalicevic]

No worries, I understand :) Yes lets merge this PR and then we will upstream the other one and happy to do a sync review if you find it useful as its quite big.