Skip to content
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

Update to futures 0.2 #1448

Closed
seanmonstar opened this issue Feb 21, 2018 · 11 comments · Fixed by #1470
Closed

Update to futures 0.2 #1448

seanmonstar opened this issue Feb 21, 2018 · 11 comments · Fixed by #1470
Labels
E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work.
Milestone

Comments

@seanmonstar
Copy link
Member

It's not released yet, but this is just tracking that it needs to happen and is part of 0.12.

@seanmonstar seanmonstar added the E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work. label Feb 21, 2018
@seanmonstar seanmonstar added this to the 0.12 milestone Feb 21, 2018
@cramertj
Copy link

I'm super excited for this and would be interested in helping out! futures-0.2 is nearly code-complete, so we could probably get started soon. Do you have a branch you'd like PRs submitted to?

@seanmonstar
Copy link
Member Author

There's a 0.12.x branch now on the repo where I've been trying to add new breaking changes to.

@srijs
Copy link
Contributor

srijs commented Mar 17, 2018

Now that tokio 0.1 has landed in the 0.12.x branch, we should be able to attempt this.

Is anyone working on this yet? Otherwise I'd have some time tomorrow to take a stab!

@srijs
Copy link
Contributor

srijs commented Mar 18, 2018

Good news: So far I've got everything but the tests to compile. I'm using the unstable-futures feature that recently landed in tokio, together with two PRs I've opened to use futures 0.2.0-alpha in dependency crates:

I'll try to find some time tonight to fix up the tests, and then this should be ready for review (fingers crossed).

@srijs
Copy link
Contributor

srijs commented Mar 19, 2018

Tests compile now, however running into issues with iovec. Will investigate and (hopefully) fix tomorrow!

@seanmonstar
Copy link
Member Author

@srijs just as a heads up, I've merged 0.12.x into master, will be deleting the 0.12.x branch so there's no confusion of the difference between master and 0.12.x.

@aturon
Copy link
Contributor

aturon commented Mar 19, 2018

@srijs awesome work!

@srijs
Copy link
Contributor

srijs commented Mar 20, 2018

Okay, I think I figured out the issue with iovec, and could use some help finding the right way to go.

Previously (iirc) WriteBufAuto was used to detect whether the underlying AsyncWrite object supported/used vectored i/o, and was adjusting the buffering strategy accordingly. This worked because the method on AsyncWrite was write_buf, which actually accepted a Buf object. So the detection is more or less straight-forward: If the underlying object calls Buf::bytes, it likely does not use vectored io, but it's likely that it does when it called Buf::bytes_vec instead.

With the new interface in futures, the equivalent to write_buf is now poll_vectored_write which accepts a mutable slice of IoVec. The issue with that is that the control is inverted: Instead of the write object making the choice of whether to use vectored i/o, the caller decides whether to use it, with the write object using the default impl of poll_vectored_write instead if it doesn't support vectored io (the default impl will fallback to using poll_write on the first iovec that was passed to it). This makes detection a lot harder in my understanding.

From what I can see, we have multiple options now:

  1. Drop the detection logic, always attempt to use vectored I/O (the AsyncWrite impl will fall back if not supported)
  2. Drop the detection logic, never use vectored I/O (sounds like a bad idea?)
  3. Petition for the AsyncWrite trait in futures 0.2 to use Buf instead of requiring a slice of IoVec (or possibly add a second method poll_write_buf?)
  4. Another magic way how we could detect this?

Conceivably we could also combine the approaches, e.g. go with Option 1 first and pursue Option 3 longer term.

@srijs
Copy link
Contributor

srijs commented Mar 20, 2018

I've got a sketch for a possible (imo slightly hacky) detection algorithm based on poll_vectored_write.

For context, my logic to write a Buf into an underlying AsyncWrite object looks something like this:

if !buf.has_remaining() {
    return Ok(Async::Ready(0));
}
let n = {
    static PLACEHOLDER: &[u8] = &[0];
    let mut bufs = [From::from(PLACEHOLDER); 64];
    let i = Buf::bytes_vec(&buf, &mut bufs[..]);
    try_ready!(io.poll_vectored_write(cx, &bufs[..i]))
};
buf.advance(n);
return Ok(Async::Ready(n));

We could exploit our knowledge of what the default impl for poll_vectored_write does in the following way:

If i > 1 (more than one io vec available) and n == bufs[0].len(), then we know with some certainty that the default impl was used, since only the first of many buffers was written. Switch our strategy to Strategy::Flatten. A false positive could be produced in a situation where the number of bytes that the underlying I/O can accept coincides with the length of the first buffer.

If i > 1 (more than one io vec available) and n > bufs[0].len(), then we know that a non-default impl was used, since more than just the first buffer was written, meaning there is a good chance that vectored I/O is supported. Switch our strategy to Strategy::Queue.

If neither of these conditions apply, we can't be certain either way, and should keep using the current strategy (which is Strategy::Auto by default).

The big risk here is that we depend on the behaviour of default impl for poll_vectored_write in futures, which I assume might change at any point.

@srijs
Copy link
Contributor

srijs commented Mar 20, 2018

@seanmonstar thanks for the heads-up, I'm rebasing now...

@aturon thanks, let's make futures 0.2 a success! :)

@srijs
Copy link
Contributor

srijs commented Mar 20, 2018

The PR can now be found here: #1470

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants