-
Notifications
You must be signed in to change notification settings - Fork 205
perf: Use a global tokio runtime #1614
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1614 +/- ##
============================================
+ Coverage 56.12% 58.55% +2.42%
- Complexity 976 1063 +87
============================================
Files 119 125 +6
Lines 11743 12582 +839
Branches 2251 2374 +123
============================================
+ Hits 6591 7367 +776
- Misses 4012 4020 +8
- Partials 1140 1195 +55 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@Kontinuation @wForget could you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Now we are using environment variables to configure tokio runtime, the proper way of setting up these environment variables on Spark executors will be --conf spark.executorEnv.COMET_BLOCKING_THREADS=N
in a non-local setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, LGTM
@comphead @parthchandra could I get a committer review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thanks for the reviews @Kontinuation @wForget @parthchandra |
Which issue does this PR close?
Closes #1590
Helps with #1523
Rationale for this change
If I configure Spark executor with 8GB + 1GB offHeap and using default settings for tokio threads, q4 hangs in main branch, but completes with these changes.
What changes are included in this PR?
Based on an executor configured for 8 concurrent tasks and 1 core per task, the defaults for the overall process now change as follows:
How are these changes tested?
Manual testing. I do not see any change in overall TPC-H performance, but I can now get q4 to complete with less memory than before.
Note that we cannot close #1523 until apache/datafusion#15323 is resolved.