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

CurrentThread::turn() but only returning if *none* of the futures can proceed #310

Closed
sdroege opened this issue Apr 10, 2018 · 8 comments
Closed

Comments

@sdroege
Copy link
Contributor

sdroege commented Apr 10, 2018

Currently CurrentThread::turn() tries to run every future exactly once, if none can proceed it parks the thread and tries that again.

For my use-case (throttling the calls to turn/park to have things processed in batches of N ms to reduce wakeups) this is suboptimal, and what I would need is a function that runs all futures until none of them can proceed anymore. The problem here is that running future N can unblock future N-1, which was then already tried in this turn so it would only ever be tried again in the next turn.

Implementing this seems relatively straightforward in a naive way

    pub fn turn(&mut self, duration: Option<Duration>)
        -> Result<Turn, TurnError>
    {
        if !self.tick() {
            let res = match duration {
                Some(duration) => self.executor.park.park_timeout(duration),
                None => self.executor.park.park(),
            };

            if res.is_err() {
                return Err(TurnError { _p: () });
            }

            loop {
                if !self.tick() { break; }
            }
        }

        Ok(Turn(()))
    }
@MathieuDuponchelle
Copy link

This did fix an issue I was observing, and makes sense to me from a high level perspective

@sdroege
Copy link
Contributor Author

sdroege commented Apr 10, 2018

The suggestion above, while it works, would run forever and never return if there are e.g. two futures that always notify each other while being polled, or a future that notifies itself.

With the current implementation, any future that is notified while we're in turn would only ever be polled on the next call to turn.

EDIT: And actually we should also do such a loop around tick in the else-case of the other if, otherwise turn would behave different in both cases.

@sdroege
Copy link
Contributor Author

sdroege commented Apr 11, 2018

There's also the problem that calling turn forever (my changed version or the original) will potentially never park the thread and thus poll all fds (if CurrentThread uses the Reactor for parking the thread). This means that turn could process futures forever without ever noticing that some of the fds are ready too now, and never work on those futures.

The scheduler makes sure that futures are somewhat fairly treated, that is only futures that were ready up to the current tick are handled in the order in which they were notified. However by not polling if there are outstanding futures, we would break this and favour futures that are notified indepedent of IO.

@sdroege
Copy link
Contributor Author

sdroege commented Apr 11, 2018

In summary, I believe it would be required for scheduler fairness to always interleave polling/parking and the call to tick. And the turn API violates scheduler fairness because of that.

A "fair" turn would first always poll with a 0 timeout, then tick once. And only if there were no futures in the queue at that point, it would poll with the real timeout and follow with a single tick.

This would however not solve my use-case :) For implementing that I would in addition need a return value on turn to know if any futures were polled at all or if the queue was just empty all the time.

sdroege added a commit to sdroege/tokio that referenced this issue Apr 11, 2018
… first

This ensures that all fd-based futures are put into the queue for the
current tick, if the CurrentThread is parking via the Reactor.

Otherwise, if there are queued up futures already, only those would be
polled in the turn. These futures could then notify others/themselves to
have the queue still non-empty on the next turn. Which then potentially
allows the reactor to never be polled, and thus fd-based futures are
never queued up and polled.

Also return in the Turn return value whether any futures were polled at
all, which allows the caller to know if any work was done at all in this
turn and based on that adjust behaviour.

tokio-rs#310
@sdroege
Copy link
Contributor Author

sdroege commented Apr 11, 2018

Added some implementation for this here #313

sdroege added a commit to sdroege/gst-plugin-threadshare that referenced this issue Apr 12, 2018
… before waiting

Otherwise in e.g. a pipeline like
  ts-udpsrc ! ts-queue ! fakesink
the first turn would only get a packet and queue it up, then we would
wait due to throttling and only then we would forward the packet from
the queue (but not poll the socket again), wait again due to throttling
and only then poll and get the next packet.

See tokio-rs/tokio#310
@carllerche
Copy link
Member

Sorry for the delay. I was out last week. I'll be working on getting up to speed again.

@carllerche
Copy link
Member

I commented on the PR, I'll keep discussion there.

sdroege added a commit to sdroege/tokio that referenced this issue Apr 24, 2018
… first

This ensures that all fd-based futures are put into the queue for the
current tick, if the CurrentThread is parking via the Reactor.

Otherwise, if there are queued up futures already, only those would be
polled in the turn. These futures could then notify others/themselves to
have the queue still non-empty on the next turn. Which then potentially
allows the reactor to never be polled, and thus fd-based futures are
never queued up and polled.

Also return in the Turn return value whether any futures were polled at
all, which allows the caller to know if any work was done at all in this
turn and based on that adjust behaviour.

tokio-rs#310
@sdroege
Copy link
Contributor Author

sdroege commented Apr 25, 2018

This is done now

@sdroege sdroege closed this as completed Apr 25, 2018
gstreamer-github pushed a commit to sdroege/gst-plugin-rs that referenced this issue Nov 10, 2018
…eft to be run before waiting

Otherwise in e.g. a pipeline like
  ts-udpsrc ! ts-queue ! fakesink
the first turn would only get a packet and queue it up, then we would
wait due to throttling and only then we would forward the packet from
the queue (but not poll the socket again), wait again due to throttling
and only then poll and get the next packet.

See tokio-rs/tokio#310
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants