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

Improved thread pool code #1589

Closed
wants to merge 10 commits into from
Closed

Conversation

jjb
Copy link
Contributor

@jjb jjb commented May 25, 2018

These are various small and large changes to the ThreadPool code.

My main goal here is to work toward code that is easier to reason about and easier to change.

More specific goals that this code may facilitate:

  • understand which objects need to be threadsafe together -- we can possibly have more than one mutex, each wrapping a smaller amount of code
  • related to this: the @todo array would probably be better implemented as a QueueQueues come with built in concurrency-facilitating behavior (they have an internal mutex so no outside mutext is needed and #pop sleeps when the queue is empty and returns nil when the Queue has been shut down), which I believe will make the code nicer and possibly more performant. See an example of how this can be used here: patrickxb/stathat@b47dd80
  • try to move thread coordination and thread work into different places. currently the main thread loop both checks to see if it's supposed to reap itself and also checks for more available work. i think it will be possible and beneficial to move the reaping code to a higher level, and only have the thread cease when it sees "shutdown"

There are several Thread methods that ThreadPoolWorker simply passes down to the Thread that it manages. This could be a smell... but I think it can lead to further improvements in which coordination code goes where. Perhaps we can keep iterating so that a shutdown command does several of the things needed for managing an individual thread, without need for fine-grained thread coordination at the top ThreadPool level.

related: #1577, #1586, #1588

It's a work in progress. I'd love to get some feedback to direct my continued work. I would recommend looking at the commits one by one. "hide whitespace changes" will help with some of them.

image

I'm guessing that you will like some of the initial aesthetic changes. If so, I can make another PR with just those so we can get those into master and simplify this PR.

jjb added 10 commits May 25, 2018 15:22
* makes it clear that `@threads` is set of threads in the pool
* differentiates from the higher-level puma concept of
  workers (OS-level processes)
use elsif and else instead of break for flow control,
then break after logic is done
Start moving work done by a ThreadPool's worker threads
into a standalone class.
@jjb jjb force-pushed the improved-thread-pool-code branch from fd379ac to 78f0e53 Compare May 25, 2018 22:12
@jjb
Copy link
Contributor Author

jjb commented Oct 24, 2018

@schneems ever get a chance to take a look at this?

@evanphx
Copy link
Member

evanphx commented Feb 20, 2019

I'm going to say no to this change. The usage of moving to a big set of Hash's for configuration coupled with moving the most important part of the file to a different file doesn't help with understanding, imho.

@evanphx evanphx closed this Feb 20, 2019
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