feat(kad): use `Bytes` for `Record::value` #4753

joshuef · 2023-10-28T12:30:07Z

Description

This should help avoid potentially costly clones over Record.

Notes & open questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

joshuef · 2023-10-28T12:30:57Z

Similar in scope to #4751 This should make end user handling of Records much lighter

examples/distributed-key-value-store/src/main.rs

protocols/kad/CHANGELOG.md

protocols/kad/src/behaviour/test.rs

thomaseizinger

Looks good, one more suggestion :)

protocols/kad/src/behaviour/test.rs

joshuef · 2023-10-30T08:58:05Z

Updated 🙇

protocols/kad/CHANGELOG.md

thomaseizinger · 2023-11-02T00:19:33Z

protocols/kad/src/protocol.rs

@@ -314,7 +314,7 @@ pub enum KadResponseMsg {
        /// The key of the record.
        key: record::Key,
        /// Value of the record.
-        value: Vec<u8>,
+        value: Bytes,


I just explored this a bit more and KadResponseMsg is actually never cloned so we can probably just remove the trait and this here.

Ah sorry I think I posted this to the wrong thread here.

thomaseizinger · 2023-11-02T00:21:03Z

protocols/kad/src/protocol.rs

@@ -422,7 +422,7 @@ fn resp_msg_to_proto(kad_msg: KadResponseMsg) -> proto::Message {
            key: key.to_vec(),
            record: Some(proto::Record {
                key: key.to_vec(),
-                value,
+                value: value.to_vec(),


Same as with the gossipsub branch, I am a bit doubtful that this actually yields improvements because we are copying here again.

Hmmm, the knock on is that we're reading from the Record which is and can be cloned in kad and user space. So we'll end up allocating to just have vecs in here ( if the Record contains Bytes for the value which I think it should...).

That comes in

HandlerIn::PutRecordRes { key: record.key, value: record.value, // <- we'd have to convert _from_ Bytes here, request_id, },```

Sorry the above was for another comment response.

We are still having to convert to vec here, but that should become the only clone, vs a variety of cloneing that happens in the kad layers at the moment.

So this should still yield reduce mem.

mergify · 2023-11-02T15:45:36Z

This pull request has merge conflicts. Could you please resolve them @joshuef? 🙏

joshuef · 2023-11-03T10:06:19Z

Small update/rebase there.

mergify · 2023-11-05T07:47:31Z

This pull request has merge conflicts. Could you please resolve them @joshuef? 🙏

joshuef · 2023-11-08T09:14:33Z

What's the proper way w/r/t version bumping here? Should I be bumping in the cargo.toml already? Or just noting the unreleased nature in the changelog?

mergify · 2023-11-10T02:38:34Z

This pull request has merge conflicts. Could you please resolve them @joshuef? 🙏

joshuef · 2023-11-13T11:12:10Z

@thomaseizinger Are there any blockers to landing this at the moment? Are we looking at the protobuf Cow work as a prereq? (I'm a bit stalled there, not sure on a decent approach... I've commented on the issue for that)

thomaseizinger · 2023-11-14T02:10:28Z

@thomaseizinger Are there any blockers to landing this at the moment? Are we looking at the protobuf Cow work as a prereq? (I'm a bit stalled there, not sure on a decent approach... I've commented on the issue for that)

Two things:

It is a breaking change because Record and all of its fields are public.
I am not convinced we are reducing the number of allocations. Doesn't this PR as is just delay them, assuming we ignore user-space? I'd prefer a more holistic solution that we can properly benchmark to be honest.

As a first step, we should probably prepare for making all fields of Record private and provide accessor / conversion functions. That will give us freedom down the line to make changes to the internals.

I understand that you probably don't want to wait for that to land as we've just cut a breaking release :)
If you don't mind, I would appreciate if you could explore within your fork on what changes you need to properly cut down on allocations within libp2p-kad. I'd assume that migrating the protobuf's to use borrowed data is definitely part of that solution.

joshuef · 2023-11-14T09:16:21Z

That's fine, I can explore a bit more easily now that we're atop 0.53 in our repo anyway. I can understand wanting to know more concretely what's afoot and the benefits we'd get here. Do you have a recommended test setup/bench for allocations at the moment? (You can something in another thread, but I don't recall where that was).

Also makes sense w/r/t Record APIs 👍

Doesn't this PR as is just delay them

I don't think so? It converts every kad-space clone of Record into something much cheaper (I can see four points where we record.clone() in kad, eg.). So that's allocs avoided there, even if we still need to realloc at the moment for protobuf.

Plus any user space gains when handling Records?

I'll try and form a minimal change from our codebase (with the libp2p branch being the only diff) and get some numbers / repro steps if that's helpful 👍

thomaseizinger · 2023-11-21T00:17:25Z

I'll try and form a minimal change from our codebase (with the libp2p branch being the only diff) and get some numbers / repro steps if that's helpful 👍

That would be great!

This should help avoid potentially costly clones

joshuef force-pushed the KadRecordBytes branch from dcc2036 to 6074c1e Compare October 28, 2023 12:30

joshuef force-pushed the KadRecordBytes branch from 6074c1e to caffe74 Compare October 28, 2023 12:45

joshuef changed the title ~~fix(gossip): convert kad record.value to Bytes.~~ feat(kad): convert kad record.value to Bytes. Oct 28, 2023