Configure thread pools size relative to the number of available cores by wendigo · Pull Request #16303 · trinodb/trino

wendigo · 2023-02-28T13:19:12Z

Description

Alternative to #16287

It makes it possible to configure threads pools using more sophisticated syntax:

[int] (exact number)
[float]C - multiplier of available cores (i.e. machine with 24 cores, 2C = 48 threads, 0.1C = 2 threads)

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

phd3

Added some comments. Overall I like this generic approach over specific change in #16287, but would also let other maintainers chime in w.r.t whether this should go in airlift (and the syntax)

phd3 · 2023-03-01T16:12:58Z

...-raptor-legacy/src/main/java/io/trino/plugin/raptor/legacy/storage/StorageManagerConfig.java

is this a separate bugfix as looks like previous impl would always cap it to 1? if so, i'd keep it in a separate commit/PR. I'm not sure about whether the previous value was intentional. cc @electrum

phd3 · 2023-03-01T16:13:41Z

...tor-legacy/src/test/java/io/trino/plugin/raptor/legacy/storage/TestStorageManagerConfig.java

same comment about max here

phd3 · 2023-03-01T16:18:03Z

core/trino-main/src/main/java/io/trino/execution/TaskManagerConfig.java

for many of these, documentation wuold need to be updated right?

phd3 · 2023-03-01T16:19:23Z

core/trino-spi/src/main/java/io/trino/spi/ThreadsCount.java

is this something that should belong in airlift like Duration/Datasize? @trinodb/maintainers

phd3 · 2023-03-01T16:36:17Z

core/trino-main/src/main/java/io/trino/execution/TaskManagerConfig.java

I think we should be able to use ThreadsCount parameter instead of string: https://github.com/airlift/airlift/blob/master/configuration/src/main/java/io/airlift/configuration/ConfigurationFactory.java#L666

is there a reason to do otherwise?

core/trino-spi/src/main/java/io/trino/spi/ThreadsCount.java

phd3 · 2023-03-01T17:00:10Z

core/trino-spi/src/test/java/io/trino/spi/TestThreadsCount.java

nit: use assertThatThrownBy

wendigo · 2023-03-01T19:08:13Z

@phd3 I agree that this could be a part of the airlift and applied there as well

findepi · 2023-03-01T20:55:50Z

I am concerned about additional complexity this introduced. Is it outweighed with value added?
for scripting systems like maven it's quite practical to set -T 1C without thinking much about cores... (Actually, even for maven it's not enough. I use # threads being 1C - 1 to maintain MacOS responsiveness while running a build)
For Trino, i think people know where they deploy and if they need to tune, they can do it without issue.

(this doesn't mean we shouldn't auto-tune. We should auto-tune out-of-the-box, but i understand this is now what this PR is doing)

electrum · 2023-03-02T06:02:13Z

I like this in general. There are probably various configs we could update to have better defaults.

I don’t like the “C2” syntax since it’s easy to mix up with “2C”. I also question if this is needed. What’s the use case? Do we ever require power of two? If we do, we could make rounding automatic for those?

wendigo · 2023-03-02T08:17:18Z

@electrum there are cases when power of two size is required and actually validated. What would be a better syntax?

core/trino-spi/src/main/java/io/trino/spi/ThreadsCount.java

sopel39 · 2023-03-02T09:03:13Z

core/trino-spi/src/main/java/io/trino/spi/ThreadsCount.java

It's confusing as it suggest it's 2 number of cores rather than pow of two. See how we deal with it in io.trino.execution.TaskManagerConfig#taskConcurrency.

I think user should just provide number that is power of two. These config properties shouldn't be touched by user too much anyway if they don't know what they are doing.

core/trino-main/src/main/java/io/trino/execution/TaskManagerConfig.java

phd3 · 2023-03-07T19:13:33Z

@wendigo so I read this is as we generally agree that this can move into airlift.

Based on some comments above - looks like we still need to discuss on the syntax simplification especially around C2. I like the suggestion in #16303 (comment). @findepi do you feel strongly about the overhead if somehow the C2 scenario is simplified?

wendigo · 2023-03-29T11:16:31Z

Extracted to airlift/units#31

wendigo · 2023-07-17T13:24:10Z

@phd3 @Praveen2112 @sopel39 this type is now coming from airlift/units

New type was introduced in the airlift/units 1.10

findepi · 2023-07-21T11:31:23Z

client/trino-jdbc/src/test/java/io/airlift/units/ThreadCount.java

+// newer units version in JDBC due to different JDK target.
+// It is temporary solution until client and JDBC are moved to JDK 11+.
+// This class is added to test classes, so it won't be a part of the jdbc driver.
+public class ThreadCount


Why would we need this copy?
plase remove

Jdk 8 target and newer units is on 11

i know. but trino-jdbc should avoid having classes in io.airlift

It’s the matter of test scope

So this class resides in trino-jdbc tests, it is unused there, and must remain API-compatible with the newly added ThreadCount class in units. Does this sounds like we're doing something wrong here?

Yeah. Keeping JDBC on JDK 8 which prohibits upgrading dependencies like airlift units

If i understand correctly, trino-jdbc ships units 1.7 for its own use and it downgrades units version for test scope resulting in NoClassDefFoundError when we try to start trino server.

unless maven allows us to package units 1.7 but still use a newer version for test classpath, i see the following options:

stop using airlift units in trino-jdbc (perhaps copying & repackaging necessary classed if this is indeed warranted)

move jdbc tests that require trino server to a separate module, where we will use modern units.

Moving tests seems like a better option

findepi · 2023-09-21T12:44:34Z

@wendigo i'd suggest to split this PR and configure eg task.max-worker-threads first and then proceed with other configs.
things like legacy raptor's backup config will want a different reviewers audience than the core driver processing configuration.

findepi · 2023-09-21T19:07:09Z

I tried using ThreadCount internally and realized it's not really a value object. It's just a parser String -> int.
I think i would replace the class with a parser:

public class ThreadCountParser
{
    private static final String PER_CORE_SUFFIX = "C";
    private static final Supplier<Integer> AVAILABLE_PROCESSORS = memoize(MachineInfo::getAvailablePhysicalProcessorCount);
    public static final ThreadCountParser DEFAULT = new ThreadCountParser(AVAILABLE_PROCESSORS);

    private final Supplier<Integer> coreCount;

    @VisibleForTesting
    ThreadCountParser(Supplier<Integer> coreCount);

    public int parse(String value);
}

this would result in simpler code, simpler concepts

added bonus?

the parser would belong to Trino codebase (or airlift), not units
and thus there would be no testing issue for trino-jdbc

cc @wendigo @losipiuk

findepi · 2023-09-23T07:23:49Z

core/trino-main/src/main/java/io/trino/execution/TaskManagerConfig.java

    private DataSize maxIndexMemoryUsage = DataSize.of(64, Unit.MEGABYTE);
    private boolean shareIndexLoading;
-    private int maxWorkerThreads = Runtime.getRuntime().availableProcessors() * 2;
+    private ThreadCount maxWorkerThreads = ThreadCount.valueOf("2C");


This is two separate changes

the main one per PR title: allowing to configure n in n * number of cores formula

how the "number of cores" is determined

I think the changes should go as separate things. The (1) is obvious, but the (2) is tricky and needs to be thoroughly code commented

findepi · 2023-09-23T07:25:11Z

client/trino-jdbc/src/test/java/io/airlift/units/ThreadCount.java

+            int availableProcessorCount = Runtime.getRuntime().availableProcessors();
+            int totalPhysicalProcessorCount = availableProcessorCount;
+            if ("amd64".equals(osArch) || "x86_64".equals(osArch)) {
+                OptionalInt procInfo = tryReadFromProcCpuinfo();


How is it better than Runtime.getRuntime().availableProcessors() and why take min of the two instead of just using "the better one"?
This should be documented.

findepi · 2023-09-23T07:25:51Z

client/trino-jdbc/src/test/java/io/airlift/units/ThreadCount.java

+                }
+            }
+
+            // cap available processor count to container cpu quota (if there is any).


Why container quota is inspected for certain architectures only?

Worker threads defaults to 2*#cores. This commit allows configuration to be #cores based too. Extracted from trinodb#16303 Co-authored-by: Mateusz "Serafin" Gajewski <mateusz.gajewski@gmail.com>

Worker threads defaults to 2*#cores. This commit allows configuration to be #cores based too. Extracted from #16303 Co-authored-by: Mateusz "Serafin" Gajewski <mateusz.gajewski@gmail.com>

cla-bot bot added the cla-signed label Feb 28, 2023

wendigo requested review from gaurav8297, hashhar, kokosing and phd3 February 28, 2023 13:19

wendigo force-pushed the serafin/add-threads-count branch from 8eeb7aa to fd7c5ed Compare February 28, 2023 13:31

github-actions bot added hive Hive connector tests:hive labels Feb 28, 2023

wendigo force-pushed the serafin/add-threads-count branch from fd7c5ed to 9a92ef8 Compare March 1, 2023 14:18

wendigo changed the title ~~Adds ThreadsCount record to simplify configuration~~ Configure thread pools size relative to the number of available cores Mar 1, 2023

phd3 reviewed Mar 1, 2023

View reviewed changes

phd3 requested a review from a team March 1, 2023 17:14

sopel39 requested changes Mar 2, 2023

View reviewed changes

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

sopel39 mentioned this pull request Mar 29, 2023

Add ThreadCount type airlift/units#31

Merged

wendigo force-pushed the serafin/add-threads-count branch from 9a92ef8 to de3fc07 Compare July 17, 2023 13:16

wendigo requested review from Praveen2112, phd3 and sopel39 July 17, 2023 13:23

Migrate threads configuration to use ThreadCount

33d39b3

New type was introduced in the airlift/units 1.10

wendigo force-pushed the serafin/add-threads-count branch from de3fc07 to 33d39b3 Compare July 17, 2023 19:51

github-actions bot added the jdbc Relates to Trino JDBC driver label Jul 17, 2023

findepi reviewed Jul 21, 2023

View reviewed changes

findepi reviewed Sep 23, 2023

View reviewed changes

wendigo closed this Nov 23, 2023

wendigo deleted the serafin/add-threads-count branch November 23, 2023 19:14

findepi mentioned this pull request Feb 20, 2024

Configure worker threads relative to core count #20772

Merged

Conversation

wendigo commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

phd3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wendigo commented Mar 1, 2023

Uh oh!

findepi commented Mar 1, 2023

Uh oh!

electrum commented Mar 2, 2023

Uh oh!

wendigo commented Mar 2, 2023

Uh oh!

Uh oh!

sopel39 Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phd3 commented Mar 7, 2023

Uh oh!

wendigo commented Mar 29, 2023

Uh oh!

wendigo commented Jul 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Sep 21, 2023

Uh oh!

findepi commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

wendigo commented Feb 28, 2023 •

edited

Loading

sopel39 Mar 2, 2023 •

edited

Loading

findepi commented Sep 21, 2023 •

edited

Loading