-
Notifications
You must be signed in to change notification settings - Fork 170
[runtime/p2p] Use Buf / BufMut in networking APIs
#2558
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
Conversation
Deploying monorepo with
|
| Latest commit: |
429c414
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0f4be186.monorepo-eu0.pages.dev |
| Branch Preview URL: | https://cl-vectored-network.monorepo-eu0.pages.dev |
3ceff94 to
c376e34
Compare
3106904 to
d29b10d
Compare
233a1d2 to
8f6dfef
Compare
8f6dfef to
b7faedd
Compare
Buf / BufMut in networking APIs
b7faedd to
957c3ae
Compare
| buf.put_slice(&message); | ||
| self.inner.send(buf.freeze(), priority).await | ||
| self.inner | ||
| .send(subchannel.encode().chain(message), priority) |
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.
🚀
stream/src/utils/codec.rs
Outdated
| len.write(&mut prefixed_buf); | ||
| prefixed_buf.extend_from_slice(buf); | ||
| sink.send(prefixed_buf).await.map_err(Error::SendFailed) | ||
| let data = len.encode().freeze().chain(buf); |
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.
🚀
(this one will actually get vectored)
de1b2d6 to
ba0fb8a
Compare
| &mut self, | ||
| buf: impl Into<StableBuf> + Send, | ||
| ) -> impl Future<Output = Result<StableBuf, Error>> + Send; | ||
| fn recv(&mut self, buf: impl BufMut + Send) -> impl Future<Output = Result<(), Error>> + Send; |
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.
Should we add a trait alias for BufMut + Send
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.
I don't think so; It's only used 7 times in the whole codebase & is a bit more clear to have inline.
ba0fb8a to
e7f0fd7
Compare
410dcfe to
14c4508
Compare
patrick-ogrady
left a comment
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.
Left my changes here: #2706
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.
Maybe we could have:
Encode::encode_mut() -> BytesMutEncode::encode() -> Bytes { self.encode_mut().freeze() }
This would remove all of the encode().freeze() noise and make sure we actually freeze as much as we can (I think we are missing a lot of places?). We probably wouldn't use encode_mut() anywhere right now (but not sure about this, should be trivial anyway).
2b71082 to
8af9de0
Compare
8af9de0 to
429c414
Compare
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #2558 +/- ##
==========================================
- Coverage 92.82% 92.81% -0.01%
==========================================
Files 361 361
Lines 106797 106800 +3
==========================================
- Hits 99134 99129 -5
- Misses 7663 7671 +8
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Overview
Alters the core networking APIs to use
Buf/BufMutrather thanStableBuf. This unlocks passing non-contiguous buffers + vectorized read/write operations.closes #784