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

fix: Excessive thread use #3

Merged
merged 6 commits into from
Jul 19, 2024
Merged

Conversation

cwhite-rippling
Copy link
Contributor

Step 1:

  • Instead of spawning a thread, use tokio to sleep until tokio thinks we should wake up.
  • Then when we wake up, check if the correct amount of time has passed.

Step 2:

  • yes: Poll:Ready
  • no: Repeat Step 1

Enjoys (most of) the performance of tokio without any of the code :). State free, initialization free!!

Also when taking a second look at this code I realized I can express this in a much simpler manner. Originally I just patched the thread portion of the future implementation, but by using tokio I don't actually need to implement a future at all. I turned ~50 lines of code into <10.

pub async fn sleep(duration: Duration) {
    let deadline = SuspendUnawareInstant::now() + duration;
    let mut now = SuspendUnawareInstant::now();
    while now < deadline {
        tokio::time::sleep(deadline - now).await;

        now = SuspendUnawareInstant::now();
    }
}

@cwhite-rippling cwhite-rippling requested a review from brhoades July 19, 2024 17:06
@brhoades
Copy link
Collaborator

Let's mention the bug fixed in the PR title (potentially excessive thread use) and describe it in the description.

Copy link
Collaborator

@brhoades brhoades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's add a regression test.

Copy link
Collaborator

@brhoades brhoades Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good.

Before we merge, can we add a regression test which would have blown up with the prior impl? I expect spawning and polling ~5k 500ms sleeps wouldn't have went well before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great callout! Also just ran some tests and for 100k 500ms timeouts it took 15.57s using threads and 0.83s with tasks. Amazing speedup.

@cwhite-rippling cwhite-rippling changed the title Remove std::thread usage fix: Excessive thread use Jul 19, 2024
@cwhite-rippling cwhite-rippling merged commit fd5824c into main Jul 19, 2024
@cwhite-rippling cwhite-rippling deleted the calder/improve-future-impl branch July 24, 2024 18:23
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

Successfully merging this pull request may close these issues.

2 participants