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

Fixed size ThreadPool scheduler #20808

Merged
merged 69 commits into from
Jun 7, 2024
Merged

Fixed size ThreadPool scheduler #20808

merged 69 commits into from
Jun 7, 2024

Conversation

maierlars
Copy link
Collaborator

@maierlars maierlars commented Apr 3, 2024

Scope & Purpose

This PR implements a new ThreadPoolScheduler with a fixed number of threads, and a number of different thread pools. The ThreadPoolScheduler creates a separate thread pool for each priority lane.

We have the following thread pools:

  • SimpleThreadPool - this is a very simple thread pool with a std::queue, mutex + condition variable
  • LockFreeThreadPool - this one is similar to the SimpleThreadPool but replaces the std::queue with a boost::lockfree::queue
  • WorkStealingThreadPool - this is a first version of a work stealing pool where instead of a globally shared queue, each thread has a local queue

This PR also implements a number of synthetic benchmarks as tests (by default these are disabled so that they do not run durng regular builds). These benchmarks showed, that the WorkStealingThreadPool scales significantly better than the other two thread pools, and also much better then the current SupervisedScheduler.

@maierlars maierlars self-assigned this Apr 3, 2024
@cla-bot cla-bot bot added the cla-signed label Apr 3, 2024
@maierlars maierlars force-pushed the feature/thread-pool-scheduler branch from f9a3e4e to 35d0162 Compare April 3, 2024 13:15
@maierlars maierlars force-pushed the feature/thread-pool-scheduler branch from 35d0162 to d2994c5 Compare April 4, 2024 10:23
@maierlars maierlars marked this pull request as ready for review April 6, 2024 09:09
@maierlars maierlars marked this pull request as draft April 8, 2024 12:12
Copy link
Member

@goedderz goedderz left a comment

Choose a reason for hiding this comment

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

Soo much simpler 😍

arangod/Scheduler/SimpleThreadPool.cpp Outdated Show resolved Hide resolved
arangod/Scheduler/SimpleThreadPool.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jsteemann jsteemann left a comment

Choose a reason for hiding this comment

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

LGTM


// queue fill grad tracking is currently not implemented for this scheduler
double ThreadPoolScheduler::approximateQueueFillGrade() const { return 0; }
double ThreadPoolScheduler::unavailabilityQueueFillGrade() const { return 0; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the queue fill grade may be checked by a load balancer periodically to determine if the instance is still in good shape.
Consider this calling code, which determines the instance's availability for further requests, from a load balancer's perspective:

        // if the scheduler's queue is more than x% full, render
        // the server unavailable
        double unavailabilityFillGrade =
            scheduler->unavailabilityQueueFillGrade();
        if (unavailabilityFillGrade > 0.0) {
          double fillGrade = scheduler->approximateQueueFillGrade();
          if (fillGrade >= unavailabilityFillGrade) {
            // oops, queue is relatively full
            available = false;
          }   
        }   

While this code works if the unavailability fill grade is 0, it will lead to more requests being sent to the instance now even if the queue is relatively full.
If this is an intentional change, fine. If not, is it much work to implement the fill grade methods in the new scheduler, or is there a reason why we shouldn't do this?

Copy link
Member

Choose a reason for hiding this comment

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

These are functions defined in the Scheduler interface, but currently only make sense for the SupervisedScheduler (leaky abstraction). We will have to discuss if/how to add this to the new scheduler.

Copy link
Contributor

@jsteemann jsteemann left a comment

Choose a reason for hiding this comment

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

LGTM

@mpoeter mpoeter merged commit 70b2503 into devel Jun 7, 2024
6 checks passed
@mpoeter mpoeter deleted the feature/thread-pool-scheduler branch June 7, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants