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

Virtual threads #13619

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Virtual threads #13619

wants to merge 2 commits into from

Conversation

gortiz
Copy link
Contributor

@gortiz gortiz commented Jul 16, 2024

This PR adds the ability to run multi-stage stages in different executors. Two executors are provided:

  • A cached thread executor, which mimics the current behavior
  • A virtual thread executor, which uses virtual threads, introduced in Java 21.

Changes in the code are pretty simple. The main problem here is that we need to:

  • Provide code that only compiles and is seen in Java 21.
  • Still support Java 21 11.

Changes in the pipeline are not yet complete and that is why I open this PR as a draft

@hpvd
Copy link

hpvd commented Jul 16, 2024

+1
would be really interesting to see the Performance Impact of this.
E.g. for mariadb its pretty huge:
https://mariadb.com/resources/blog/benchmark-jdbc-connectors-and-java-21-virtual-threads/

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 20.00000% with 24 lines in your changes missing coverage. Please review.

Project coverage is 61.97%. Comparing base (59551e4) to head (c656647).
Report is 764 commits behind head on master.

Files Patch % Lines
...pache/pinot/spi/executor/ExecutorServiceUtils.java 0.00% 22 Missing ⚠️
...ache/pinot/segment/spi/memory/PinotDataBuffer.java 66.66% 0 Missing and 1 partial ⚠️
...he/pinot/segment/spi/memory/unsafe/MmapMemory.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #13619      +/-   ##
============================================
+ Coverage     61.75%   61.97%   +0.22%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2555     +119     
  Lines        133233   140733    +7500     
  Branches      20636    21868    +1232     
============================================
+ Hits          82274    87224    +4950     
- Misses        44911    46874    +1963     
- Partials       6048     6635     +587     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration <0.01% <0.00%> (-0.01%) ⬇️
integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration2 0.00% <0.00%> (ø)
java-11 61.92% <16.66%> (+0.21%) ⬆️
java-21 61.82% <16.66%> (+0.20%) ⬆️
java-22 61.80% <16.66%> (?)
skip-bytebuffers-false 61.96% <20.00%> (+0.21%) ⬆️
skip-bytebuffers-true 61.80% <16.66%> (+34.07%) ⬆️
temurin 61.97% <20.00%> (+0.22%) ⬆️
unittests 61.97% <20.00%> (+0.22%) ⬆️
unittests1 46.48% <20.00%> (-0.41%) ⬇️
unittests2 27.70% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gortiz
Copy link
Contributor Author

gortiz commented Jul 16, 2024

would be really interesting to see the Performance Impact of this.

I don't expect an actual performance gain. In fact we tried virtual threads last summer and they were a bit less efficient than normal threads. That is expected and acceptable because in ideal situations these threads will not do any IO but mostly wait for other threads and heavy computation. The later is the worst case for virtual threads.

But given the current multi-stage topology, the easiest way to ensure tasks not get blocked is to use cached thread pools, which means in case of large QPS we end up using too many threads and even worse, in constrained environments (ie CPU limits un k8s/docker) we can consume all the CPU quota with multi-stage threads, starving the rest of the system.

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

3 participants