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

[joshuef]

Description

This should help avoid potentially costly clones over Record.

Notes & open questions

Change checklist

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

[joshuef]

Similar in scope to https://github.com/libp2p/rust-libp2p/pull/4751 This should make end user handling of Records much lighter

[joshuef]

Updated 🙇

[mergify[bot]]

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

[joshuef]

Small update/rebase there.

[mergify[bot]]

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

[joshuef]

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[bot]]

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

[joshuef]

@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]

@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:

1. It is a breaking change because Record and all of its fields are public.
2. 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]

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 :+1:

---

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]

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!