-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
perf: improve ipc poll logic #4037
Conversation
Codecov Report
... and 9 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
changes lgtm, got a couple of questions / requests for clarification
let mut budget = 5; | ||
|
||
// ensure we still have enough budget for another iteration | ||
'outer: loop { | ||
budget -= 1; | ||
if budget == 0 { | ||
// make sure we're woken up again | ||
cx.waker().wake_by_ref(); | ||
return Poll::Pending | ||
} |
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.
not quite clear to me - why do we need to return early? is it because the task is blocking and you want to yield back control to the scheduler?
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.
yes, this ensures this loops doesn't run for too long, since this is polled concurrently with something that also receives responses (from subscriptions)
'inner: loop { | ||
let mut drained = false; | ||
// drain all calls that are ready and put them in the output item queue | ||
if !this.pending_calls.is_empty() { |
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 this be bounded?
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.
what exactly?
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.
pending_calls futures
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.
these are already effectively bounded by the rate limit params that restrict how many calls can be active
Poll::Ready(res) => { | ||
let item = match res { | ||
Ok(Some(resp)) => resp, | ||
Ok(None) => continue 'inner, |
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.
under which circumstances this can return Ok(None)
?
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.
for subscriptions responses which are routed differently
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.
lgtm
improve poll logic of ipc connection
this makes it slightly more complicated but now priorities output over input, since output is the real bottleneck if input is high.
ref #4028