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

Allow configuring driver threads based on the number of cores #22809

Merged
merged 1 commit into from
May 30, 2024

Conversation

arhimondr
Copy link
Member

@arhimondr arhimondr commented May 22, 2024

Description

Allow configuring task.max-worker-threads as <multiplier>C where C will be automatically resolved to the number of physical cores.

Based on trinodb/trino#20772

Motivation and Context

It is not uncommon for clusters to consist of different CPU generation machines with a slightly different core count. For example Skylake Intel processor usually come with a smaller number of slightly faster cores, while similar copper lake CPUs come with higher number of slightly slower cores. Configuring a cluster to fixed number of cores may either underutilize one type of machines or underutilize the other.

Test Plan

Unit test

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Improve configuring worker threads relative to core count by setting the ``task.max-worker-threads`` configuration property to ``<multiplier>C``. For example, setting the property to ``2C`` configures the worker thread pool to create up to twice as many threads as there are cores available on a machine. :pr:`22809`

@arhimondr arhimondr marked this pull request as ready for review May 22, 2024 20:02
@arhimondr arhimondr requested a review from a team as a code owner May 22, 2024 20:02
@arhimondr arhimondr requested a review from presto-oss May 22, 2024 20:02
@steveburnett
Copy link
Contributor

== RELEASE NOTES ==

General Changes
* Improve configuring worker threads relative to core count by setting the ``task.max-worker-threads`` configuration property to ``<multiplier>C``. For example, setting the property to ``2C`` configures the worker thread pool to create up to twice as many threads as there are cores available on a machine. :pr:`22809`


long threads;
if (value.endsWith(PER_CORE_SUFFIX)) {
long multiplier = parseLong(value.substring(0, value.length() - PER_CORE_SUFFIX.length()).trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there's any benefit in allowing floating point values? e.g. 1.25C, 1.5C, etc? It may introduce some additional complexity in that we'd need to round/floor non-integer results, but if you have a large machine, say 48 cores, you would be stuck with only having the option of using 48, 96, or even large multiples with no in-between. It seems like a pretty big jump and I think allowing a floating point value would allow some additional flexibility for those cases

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is. If IO takes on average less than 50% of scheduled time then it setting it to 2x the cores might be too much. Let me add support for fraction.

yingsu00
yingsu00 previously approved these changes May 22, 2024
Copy link
Contributor

@yingsu00 yingsu00 left a comment

Choose a reason for hiding this comment

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

@ZacBlanco I saw a similar comment from the original PR and I think he made a valid point. Also when we do perf testings, the number of threads are usually multiple of the number of cores, and this number is usually 1 or 2. 2 is mostly used on Intel CPUs which have hyper-threading support. I think this change is fine and the main use case is to allow 1C instead of only 2C. On a system like Presto where CPU was never saturated, maybe higher number would help, we can revisit this if needed in the future.

hantangwangd
hantangwangd previously approved these changes May 23, 2024
@ZacBlanco
Copy link
Contributor

@ZacBlanco I saw a similar comment from the original PR and I think he made a valid point. Also when we do perf testings, the number of threads are usually multiple of the number of cores, and this number is usually 1 or 2. 2 is mostly used on Intel CPUs which have hyper-threading support. I think this change is fine and the main use case is to allow 1C instead of only 2C. On a system like Presto where CPU was never saturated, maybe higher number would help, we can revisit this if needed in the future.

I agree that most systems you either have hyperthreading or not, which is why you might choose to have only 1 or 2C available, so it makes sense. However, given that Presto has many other configurations (exchange client/server threads, etc) I can imagine a case where you may not want to always allocate 100% or 200% of available physical cores in order to reserve some cores to perform IO.

As an example, on a 32-core machine you might want to allocate 0.9C for task execution, and leave the remaining cores available to use by exchange clients, configurable as 0.1C or something. This is one particular use case. I'm not sure what other threads pools the worker might use but if you want to preserve some core allocation for background tasks I think fractional values have a use case.

On Java side I traced this particular configuration to see if the max-worker-threads actually bounds the entire worker but it seems like the exchange threads are an entirely separate threadpool1. I can't speak for the same on the native worker. I haven't done any perf benchmarks to show the impact, but I think allowing the config to support a floating point could have a benefit for IO throughput when the rest of the worker is at full utilization.

Footnotes

  1. https://github.com/prestodb/presto/blob/7290ab8848485df21cf01b8e7c958bbc36bb0775/presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java#L788

@arhimondr arhimondr dismissed stale reviews from hantangwangd and yingsu00 via da9d164 May 24, 2024 17:24
@arhimondr arhimondr force-pushed the core-thread-count branch 2 times, most recently from da9d164 to 53bfe50 Compare May 24, 2024 17:33
@arhimondr
Copy link
Member Author

Thanks for the review everyone. Added support for fractional as I realized it may actually be useful for our deployment. Please take an another look.

@yingsu00
Copy link
Contributor

@ZacBlanco

@ZacBlanco I saw a similar comment from the original PR and I think he made a valid point. Also when we do perf testings, the number of threads are usually multiple of the number of cores, and this number is usually 1 or 2. 2 is mostly used on Intel CPUs which have hyper-threading support. I think this change is fine and the main use case is to allow 1C instead of only 2C. On a system like Presto where CPU was never saturated, maybe higher number would help, we can revisit this if needed in the future.

I agree that most systems you either have hyperthreading or not, which is why you might choose to have only 1 or 2C available, so it makes sense. However, given that Presto has many other configurations (exchange client/server threads, etc) I can imagine a case where you may not want to always allocate 100% or 200% of available physical cores in order to reserve some cores to perform IO.

As an example, on a 32-core machine you might want to allocate 0.9C for task execution, and leave the remaining cores available to use by exchange clients, configurable as 0.1C or something. This is one particular use case. I'm not sure what other threads pools the worker might use but if you want to preserve some core allocation for background tasks I think fractional values have a use case.

On Java side I traced this particular configuration to see if the max-worker-threads actually bounds the entire worker but it seems like the exchange threads are an entirely separate threadpool1. I can't speak for the same on the native worker. I haven't done any perf benchmarks to show the impact, but I think allowing the config to support a floating point could have a benefit for IO throughput when the rest of the worker is at full utilization.

Footnotes

Yeah these make sense, but Java Presto does not do async IO, and as you pointed out, exchange clients are a separate thread pool, so this config is only for the worker threads. But I agree that allowing fractions give us more flexibility.

ZacBlanco
ZacBlanco previously approved these changes May 28, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for asking my opinion! Made a suggestion how to address the issue, and a couple nits about style. Looks good.

presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved
@arhimondr
Copy link
Member Author

Thanks for taking a look @steveburnett, updated.

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull updated branch, new local docs build, everything looks good. Thanks!

@arhimondr arhimondr merged commit 3cb1e6f into prestodb:master May 30, 2024
57 checks passed
@arhimondr arhimondr deleted the core-thread-count branch May 30, 2024 15:45
@wanglinsong wanglinsong mentioned this pull request Jun 25, 2024
36 tasks
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.

6 participants