refactor: switch from simple-dns to hickory-proto #107
Conversation
dignifiedquire
force-pushed
the
refactory-hickory
branch
from
e9bace3 to
92d6068
Compare
| /// Signed DNS packet | ||
| pub struct SignedPacket { | ||
| inner: Inner, | ||
| message: Message, |
There was a problem hiding this comment.
A SignedPacket is now a wrapper around a hickory Message, which is a Vec<Query> and a bunch of Vec<Record>. The previous SignedPacket was basically free to clone and would be backed by at most 1 heap allocation. The new one is not cheap to clone and has possibly a large number of heap allocations.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, the alternative is to reparse the structure everytime you request details, like the previous version did for some parts.
There was a problem hiding this comment.
It is not clear to me, what the usage patterns are in practice, and which ones should be optimized, so I went with the simplest and most typesafe one.
There was a problem hiding this comment.
I don't think this actually gets mutated, so this could also be wrapped in an Arc under the hood, to optimize the cloning path again.
There was a problem hiding this comment.
pushed a version that does the last variant
There was a problem hiding this comment.
Yes, the alternative is to reparse the structure everytime you request details, like the previous version did for some parts.
No, the current version has the parsed Packet already, no need to reparse, and that is memory efficient because simple-dns packet takes a reference of the bytes and read from them using offsets on demand.
I don't think this actually gets mutated, so this could also be wrapped in an Arc under the hood, to optimize the cloning path again.
Yes it is not mutated after its creation, still, simple-dns Packet + Bytes, is more efficient than an Arc and a bunch of vectors.
But I can tolerate memory inefficiency if simple-dns can't be made robust.
| } | ||
|
|
||
| /// Returns a slice of the serialized [SignedPacket] omitting the leading public_key, | ||
| /// to be sent as a request/response body to or from [relays](https://github.com/Nuhvi/pkarr/blob/main/design/relays.md). | ||
| pub fn to_relay_payload(&self) -> Bytes { | ||
| self.inner.borrow_owner().slice(32..) | ||
| pub fn to_relay_payload(&self) -> Vec<u8> { |
There was a problem hiding this comment.
Another example of allocation that isn't happening currently in SignedPacket, because currently the original packet is stored as is, which you can do even with hickory message, but then you would be doubling the amount of data, because simple-dns Packet doesn't copy the data, instead it just points to it.
Due to the segfaults we have seen with simple-dns, and to reduce the dependency load in
irohthis switches tohickory-protoRef n0-computer/iroh#2844
Open Questions
packetvsmessagenaming, hickory and the RFC call the structureMessage, but pkarr and simple-dns call themPacketSignedPacket? see refactor: switch from simple-dns to hickory-proto #107 (comment)simple-dnsbefore?