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

Join waves #89

Merged
merged 7 commits into from
Oct 31, 2017
Merged

Join waves #89

merged 7 commits into from
Oct 31, 2017

Conversation

dns2utf8
Copy link
Member

@dns2utf8 dns2utf8 commented Sep 9, 2017

When I talked about the threadpool at the Zürichsee meetup somebody came up with a problem when multiple threads join on a pool.

The scenario is joining threads should not be stuck once their wave
of joins has completed. So once one thread joining on a pool has
succeded other threads joining on the same pool must get out even if
the thread is used for other jobs while the first group is finishing
their join

@frewsxcv
Copy link
Collaborator

I read this through a couple times, but I'm a little confused what the issue is without these changes. Is the problem when multiple threads try to join the pool at the same time? Is there a minimal reproducing test that demonstrates what the problem is? I appreciate the pull request, just trying to better understand it :)

@dns2utf8
Copy link
Member Author

dns2utf8 commented Sep 24, 2017

I am sorry it took me so long to respond, RustFest is around the corner.
So the problem I am trying to solve came up at my talk (see video at 42:12): When a bunch of threads, let us call them submitter for now, join on a pool and immediately after it execute a new job like this:

/// more than one thread does this
pool.join();
pool.execute(move || { /* second wave of jobs */ });

Before this PR I noticed different behavior with N submitter on my laptop:

  • N == 1: works as expected
  • N < 6: works in 80% of runs, depending on system load
  • N > 8: the first 5-7 submitter threads insert the second job after the first ones are finished. The others wait for the first part of the second wave to finish, then they insert another 5-7 jobs. This goes on until all jobs are done.

After this patch the submitter threads have the generation locally and will exit the join once the event in time they are waiting for has passed. Even if the pool is already working on new jobs by the time they wake up.

Do you think the docs are clear enough? I can also squash the commits a bit if you like.

Copy link
Collaborator

@frewsxcv frewsxcv left a comment

Choose a reason for hiding this comment

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

wow, great job tracking this down @dns2utf8! i think your solution makes sense here, unblocking all threads waiting to be joined at once.

on a side note, i've been pretty busy lately (hence me taking a long time to review this). if you'd like, since you're doing most of the work for this crate nowadays, if you're comfortable with your changes, you can merge PRs without my approval. time permitting, i'm still happy to do reviews if you still want to ping me on PRs

i just gave you publishing rights for this crate if you want to try merging and publishing. let me know if you have any thoughts or need any help!

@dns2utf8
Copy link
Member Author

hey @frewsxcv

Thank you! I am honored 😄
I am trying to release the PR as 1.7.1.

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 30, 2017

🔒 Permission denied

Existing reviewers: click here to make dns2utf8 a reviewer

@dns2utf8
Copy link
Member Author

bors try

@bors
Copy link
Contributor

bors bot commented Oct 30, 2017

🔒 Permission denied

Existing reviewers: click here to make dns2utf8 a reviewer

@dns2utf8
Copy link
Member Author

@frewsxcv I was able to release 1.7.1 to crates.io and I made another PR #90 for the release.
I do not have permission to merge in this repo, but I think that is ok and bors' job now.

Would you mind adding me to the reviewers on bors?

@frewsxcv
Copy link
Collaborator

just added you, want to try again?

@dns2utf8
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Oct 31, 2017
89: Join waves r=dns2utf8 a=dns2utf8

When I talked about the threadpool at the Zürichsee meetup somebody came up with a problem when multiple threads join on a pool.

The scenario is joining threads should not be stuck once their wave
of joins has completed. So once one thread joining on a pool has
succeded other threads joining on the same pool must get out even if
the thread is used for other jobs while the first group is finishing
their join
@bors
Copy link
Contributor

bors bot commented Oct 31, 2017

@bors bors bot merged commit 3844c87 into rust-threadpool:master Oct 31, 2017
@dns2utf8 dns2utf8 deleted the join_waves branch October 31, 2017 16:29
@dns2utf8
Copy link
Member Author

Cool it works. Thank you 😄

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.

None yet

2 participants