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 ThreadCount type #31

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Mar 29, 2023

No description provided.

@wendigo
Copy link
Contributor Author

wendigo commented Mar 29, 2023

cc @martint @electrum @hashhar @phd3

@wendigo wendigo force-pushed the serafin/add-threads-count-unit branch from 7668ea8 to 0da66e4 Compare March 29, 2023 13:07
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

We definitely do not want to add OSHI as a dependency here. Can we do this in pure Java, such as by reading /proc/cpuinfo?

@wendigo
Copy link
Contributor Author

wendigo commented Mar 29, 2023

@electrum can I make it optional with the default being a /proc/cpuinfo based?

@electrum
Copy link
Member

Optional dependencies are a pain. How do you test that the fallback works? You need a different test suite that doesn’t have the optional dependency on the classpath.

The question is if we can get this from Java without needing a native dependency. If no, then we can evaluate options. If yes, then why make it optional?

@wendigo
Copy link
Contributor Author

wendigo commented Mar 31, 2023

@electrum Sure. I'll implement it without deps. Thanks for guidance!

@wendigo wendigo force-pushed the serafin/add-threads-count-unit branch 2 times, most recently from 48da75b to 14c5638 Compare April 9, 2023 09:33
@wendigo
Copy link
Contributor Author

wendigo commented Apr 9, 2023

@electrum ptal

@wendigo wendigo force-pushed the serafin/add-threads-count-unit branch 2 times, most recently from 8e9df4f to a7ff0ff Compare April 11, 2023 09:11
@wendigo
Copy link
Contributor Author

wendigo commented Apr 11, 2023

Tested in docker on arm64:

public static void main(String[] args)
    {
        System.out.println("Exact value of " + ThreadsCount.valueOf(args[0]));
        System.out.println("Next 2th value of " + ThreadsCount.valueOf(args[0]).nextPowerOf2());
    }
ARG JAVA_VERSION=17
FROM openjdk:${JAVA_VERSION}-jdk

COPY target/units-1.9-SNAPSHOT.jar units.jar
COPY *.jar .

ENTRYPOINT ["java", "-cp", "units.jar:slf4j-api-2.0.6.jar", "io.airlift.units.ThreadsCount"]
CMD [""]
❯ docker run --platform=linux/amd64 --cpus=3.5 threads-count 0.5C
Exact value of 2 threads
Next 2th value of 2 threads
❯ docker run --platform=linux/amd64 --cpus=3.5 threads-count 1C
Exact value of 4 threads
Next 2th value of 4 threads
❯ docker run --platform=linux/amd64 --cpus=3.5 threads-count 1.2C
Exact value of 5 threads
Next 2th value of 8 threads
❯ docker run --platform=linux/amd64 --cpus=3.5 threads-count 3C
Exact value of 12 threads
Next 2th value of 16 threads
❯ docker run --platform=linux/amd64 --cpus=1.5 threads-count 1C
Exact value of 2 threads
Next 2th value of 2 threads
❯ docker run --platform=linux/amd64 --cpus=1.5 threads-count 2C
Exact value of 4 threads
Next 2th value of 4 threads
❯ docker run --platform=linux/amd64 --cpus=8 threads-count 2C
Exact value of 16 threads
Next 2th value of 16 threads
❯ docker run --platform=linux/amd64 --cpus=8 threads-count 1.4C
Exact value of 11 threads
Next 2th value of 16 threads

@wendigo wendigo force-pushed the serafin/add-threads-count-unit branch from a7ff0ff to 9b40e65 Compare April 11, 2023 09:29
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Looking good

src/main/java/io/airlift/units/PowerOfTwoValidator.java Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
src/main/java/io/airlift/units/ThreadsCount.java Outdated Show resolved Hide resolved
src/main/java/io/airlift/units/ThreadsCount.java Outdated Show resolved Hide resolved
src/main/java/io/airlift/units/ThreadsCount.java Outdated Show resolved Hide resolved
src/main/java/io/airlift/units/ThreadsCount.java Outdated Show resolved Hide resolved
src/test/java/io/airlift/units/TestThreadsCount.java Outdated Show resolved Hide resolved
src/test/java/io/airlift/units/TestThreadsCount.java Outdated Show resolved Hide resolved
@wendigo wendigo force-pushed the serafin/add-threads-count-unit branch 2 times, most recently from 293491a to b68eea2 Compare April 12, 2023 09:01
@wendigo
Copy link
Contributor Author

wendigo commented Apr 12, 2023

@electrum all of your comments are addressed now

@wendigo wendigo force-pushed the serafin/add-threads-count-unit branch from b68eea2 to 0645461 Compare April 12, 2023 09:13
@wendigo
Copy link
Contributor Author

wendigo commented Apr 12, 2023

I've fixed the compilation error on JDK 11 (the last working airlift version is 219, 220 was switched to JDK 17)

@wendigo wendigo requested review from losipiuk and dain April 19, 2023 08:28
@wendigo
Copy link
Contributor Author

wendigo commented Apr 19, 2023

@sopel39 ptal

Copy link

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM - but I @sopel39 PTAL too.

@wendigo wendigo force-pushed the serafin/add-threads-count-unit branch 2 times, most recently from 7d0e6d4 to 4c572f7 Compare April 19, 2023 08:59
@wendigo wendigo force-pushed the serafin/add-threads-count-unit branch from 4c572f7 to 0ac0845 Compare May 4, 2023 11:04
@wendigo
Copy link
Contributor Author

wendigo commented May 8, 2023

@dain @electrum please review. I've dropped powerOfTwo on ThreadCount

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Let's drop the @PowerOfTwo validator. If we're going to do the clamping in the caller, then it doesn't make sense to have this validator. Also, it wouldn't work with ThreadCount since that isn't an integer.

src/main/java/io/airlift/units/ThreadCount.java Outdated Show resolved Hide resolved
src/test/java/io/airlift/units/TestThreadsCount.java Outdated Show resolved Hide resolved
src/test/java/io/airlift/units/TestThreadsCount.java Outdated Show resolved Hide resolved
src/test/java/io/airlift/units/TestThreadsCount.java Outdated Show resolved Hide resolved
@electrum electrum changed the title Add ThreadsCount type Add ThreadCount type May 8, 2023
pom.xml Outdated Show resolved Hide resolved
relative to the number of physical CPUs.

On ARM64 and non-linux systems this is resolved using java's Runtime.
For AMD64 on Linux /proc/cpuinfo is parsed to detect number of physical CPUs.

Both implementations are container-aware and will return the number of
requested CPUs if specified for the container.
@wendigo wendigo force-pushed the serafin/add-threads-count-unit branch from 0ac0845 to 5442f37 Compare July 4, 2023 13:58
@wendigo
Copy link
Contributor Author

wendigo commented Jul 4, 2023

@electrum PTAL. Rebased and addressed all comments. I've added @MinThreadCount and @MaxThreadCount validators

@electrum electrum merged commit 1a6a60e into airlift:master Jul 5, 2023
1 check passed
@wendigo wendigo deleted the serafin/add-threads-count-unit branch July 5, 2023 20:10
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.

5 participants