-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
multiaddr: Replace Bytes
with Arc<Vec<u8>>
.
#1370
Conversation
5854314
to
0ce6317
Compare
What about |
Just curious: What's wrong with Bytes? I forgotten it but vaguely recall it attempting to do some things nicely. |
I think that resizing complicates this approach. It would probably require quite a bit of effort to reach the same level of efficiency as with
Compared to previous versions, in v0.5 the design of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit: I'm extremely paranoid when it comes to the Write
trait.
Writing to a &mut Vec<u8>
automatically extends the vector, while writing to a &mut [u8]
will return an error if there isn't enough space.
Because of that, I'd prefer to make explicit that we're writing to a &mut Vec<u8>
.
Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>
Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>
Let's wait for #1328 before merging! |
Do I understand correctly that we have the following 4 options that only differ in performance?
If so, before introducing this change set, shouldn't it include benchmarks? Am I missing something? |
This second option would not provide much benefit over a
The last option has not been implemented. I welcome all benchmarks but the change is not really about improving the performance over what we have now but motivated by the desire to remove a now outdated dependency (bytes v0.4). In #1353 I left |
CC: #1367, #1351