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

Add thread-per-driver executor #18118

Merged
merged 7 commits into from
Aug 15, 2023
Merged

Conversation

martint
Copy link
Member

@martint martint commented Jul 4, 2023

Implements a new split executor that uses a dedicated thread per split. Concurrency is controlled by parking and unparking threads after each execution quantum while ensuring each thread gets its fair share of compute resources. Replaces the multi-level feedback queue with a simpler implementation inspired on Linux's CFS scheduling algorithm.

Some benefits of this approach:

  • Using a dedicated thread per split makes it possible to use Virtual Threads in the upcoming JDK 21 release and beyond
  • Potentially avoids ping-ponging across CPU cores since drivers are no longer unmounted and remounted on arbitrary threads.
  • It makes it possible to use thread-confined arenas for better memory management (with the upcoming Foreign Function & Memory API in the JDK)

Release notes

(x) This is not user-visible or docs only and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Jul 4, 2023
@wendigo wendigo self-requested a review July 4, 2023 04:34
@martint martint force-pushed the thread-per-split branch 17 times, most recently from 347e1a8 to 5796352 Compare July 10, 2023 19:39
@martint martint mentioned this pull request Jul 10, 2023
19 tasks
Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

The first 5 commits look good

@martint martint force-pushed the thread-per-split branch 9 times, most recently from 230bb8f to 5eca4ff Compare July 13, 2023 02:10
@martint martint force-pushed the thread-per-split branch 3 times, most recently from 55fc1ef to 831170a Compare July 13, 2023 21:51
@martint martint marked this pull request as ready for review July 13, 2023 21:53
Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

Looks good. Given this is behind a flag, I have no concerns about merging this as is.

The test mixed class-level state with method-level
state and wasn't consistent with everything that was
going on in that class.
These objects don't need lifecycle managed
throughout the test, so they can be created
where they are needed
@martint martint force-pushed the thread-per-split branch 6 times, most recently from 701e910 to 438f3d0 Compare August 14, 2023 21:55
@martint martint changed the title Add thread-per-split executor Add thread-per-driver executor Aug 14, 2023
In preparation for adding a ThreadPerSplitTaskExecutor
If there's an exception while the driver is being created,
the error is not being propagated to the state machine.
This can cause tasks to not be destroyed and blocked
drivers to not be canceled.
* Uses a fair queue based on Completely Fair Scheduler
* Runs each driver in a separate thread
@martint martint merged commit 5a0e489 into trinodb:master Aug 15, 2023
@martint martint deleted the thread-per-split branch August 15, 2023 05:04
@github-actions github-actions bot added this to the 424 milestone Aug 15, 2023
@JRemitz
Copy link
Member

JRemitz commented Aug 1, 2024

@martint - could the missing JMX metrics possibly be related to this change?

#22905

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants