-
Notifications
You must be signed in to change notification settings - Fork 102
Deduplicate generic::bufread::Decoder impl of tokio/futures-io
#391
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
dfcae1f to
dd5602e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
dd5602e to
bf4e415
Compare
NobodyXu
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.
Some explanation on my design/API choice
| &mut self, | ||
| cx: &mut Context<'_>, | ||
| output: &mut PartialBuffer<&mut [u8]>, | ||
| reader: &mut dyn AsyncBufRead, |
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.
cc @robjtede I made this design choice since
- for async I/O inlining usually doesn't help with performance, as it is usually the I/O that is bottleneck and inlining doesn't eliminate it
- it would avoid monomophization
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 would also say that even for in-memory objects or AsyncBufRead with (de)compression it wouldn't matter, because the AsyncBufRead::poll_fill_buf returns a &[u8], AFAIK this is something inline cannot optimize out since it involves a buffer and our de/encoder impl often involves complex stuff.
The on,y case it could be bottleneck is when poll_fill_buf returns very small slice or the (de)compression only consumes a very small part of it, like literally a few bytes, but that would usually mean the user is using a very terrible BufReader implementation
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've think of an alternative design which does not need to pass the reader at all, it also removes the variable first within do_poll_read, making it truly stateless:
impl Decoder {
fn do_poll_read(
&mut self,
output: &mut PartialBuffer<&mut [u8]>,
decoder: &mut impl Decode,
input: &mut PartialBuffer<&[u8]>,
first: bool,
) -> ControlFlow<Result<NextAction>> {
let mut first = true;
loop {
match self.state {
Decode => {
// logic unchanged ...
if res? {
self.state = State::Flushing;
} else {
return ControlFlow::Continue(()); // Ask caller to call `reader.consume` and `reader.poll_fill_buf`
}
}
}
}
}
}Caller of this funciton will then do:
let mut input = PartialBuffer(&[][]);
let mut first = true;
loop {
match this.inner.do_poll_read(output, this.decoder, &mut input) {
Continue(_) => {
if !first {
this.reader.consume(input.written.len());
}
first = false;
input = PartialBuffer(ready!(this.reader.poll_fill_buf(cx))?);
},
Break(res) => break res,
}
}this design is much cleaner
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.
@robjtede I've updated the code with this new design, and use macro_rules! to further reduce duplicate code.
I quite like the current iteration, I think it'd make maintenance much, much simpler
| #[derive(Debug)] | ||
| pub struct Decoder { | ||
| state: State, | ||
| multiple_members: bool, | ||
| } |
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.
cc @robjtede I decided to keep the state as simple as possible, anything else can be passed by parameter
|
If there's no objection, I'll merge it this evening |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
===========================
===========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I will ping you again when releasing this change @robjtede , if you think of something you'd like me to change I can open a separate pr |
Ref #384