The default of tests.jvms should not be capped by alpar-t · Pull Request #35259 · elastic/elasticsearch

alpar-t · 2018-11-05T15:39:30Z

Lift the upper limit of the default.
We heuristically divide by two because it was found that increasing past
the number of physical cores does not yield benefits.

Lift the upper limit of the default. We heuristically divide by two because it was found that increasing past the number of physical cores does not yield benefits.

elasticmachine · 2018-11-05T15:39:33Z

Pinging @elastic/es-core-infra

jasontedor

I left a comment to trigger some discussion.

jasontedor · 2018-11-05T16:12:43Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

        project.tasks.withType(RandomizedTestingTask) {
            jvm "${project.runtimeJavaHome}/bin/java"
-            parallelism System.getProperty('tests.jvms', 'auto')
+            parallelism System.getProperty('tests.jvms',  String.valueOf(Runtime.getRuntime().availableProcessors() / 2))


I have some concerns with this. Not all machines have hyper-threading, and not all machines have hyper-threading enabled. Some folks are choosing to disable hyper-threading because of security concerns. Intel is even releasing high-end processors that do not have hyper-threading (e.g., the i7-9700K, a workstation-class processor). On some machines, this will be too conservative.

ES itself uses Runtime.getRuntime().availableProcessors() as the default for the number of processors. Is there any reason we shouldn't match it? Maybe because that'd be a bit aggressive for tests. Some of them are multi-threaded and they all benefit from multi-threaded GC.

Maybe we make a hasHyperthreading variable? It'd be OS dependant, but easy enough to check on linux and mac. I dunno about windows.

Dropping my 2 cents in, I feel like we should go with Runtime.getRuntime().availableProcessors(). I think the optimal value varies based on hardware, but I found on an i7-6850k (6 core, 12 threads) that -Dtests.jvms=12 was faster than -Dtests.jvms=6; I'll caveat saying that the last time I tested this was 6 months ago and I didn't test values in between so there could be an optimal value between number of physical cores and number of threads that a processor can handle. Given my feeling that this is going to be hardware dependent, I think we could take the simplest approach first and if needed consider adding detection for hyper threading later on?

CI has 8 cores 16 threads right now, and running with 8 is faster than with 16 for sure.
It might not be related to hardware alone, but also to the number of the tests, as if there are few tests per worker the cost of starting up those workers is not worth it.

One can easily add systemProp.tests.jvms=6 to ~/.gradle/gradle.properties to customize this for their own hardware if the default is not appropriate.

@jasontedor I'm not entirely convinced it will be too conservative. On the processor you mentioned it will make no difference as it has 8 cores, and we'll continue using 4 on that processor, so there will be no change, as we currently cap at 4.
We will use fewer test workers on CPUs with fewer than 8 cores that have no or disabled hyper-threading.

I don't have a strong preference on the value we set this to. We could set it to the number of processors, but would prefer not to do hyper-threading detection ( or anything OS specific for that matter ) until we are absolutely sure we need it.

Maybe adding some documentation in TESTING will be enough.

@jaymode In my experience, going above the number of physical cores has led to worse performance. I've seen this on an i7-5930K and an i7-6950X. 🤷‍♀️

@atorok By conservative, I mean that it sets the default value at one that is lower than what I think the optimal value is.

@jasontedor do you think that we should implement hyper-threading detection now ? I was trying to make the point that while the configuration will not be optimal for users with hyper threading disabled or absent, it will still be the same or close as we have Today as we cap to 4 threads right now. If you agree that this is the case we could start with this change to get some benefits in CI right away and then look at what else we can do to have a more generally useful default if this is common enough that we feel having systemProp.tests.jvms documented will not be sufficient.

…arallel

alpar-t · 2018-11-14T17:39:58Z

@jasontedor @nik9000 @rjernst ready for review with hyper threading detection on Linux

nik9000 · 2018-11-14T17:58:34Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

    static void applyCommonTestConfig(Project project) {
+        String defaultParallel = 'auto'
+        if (project.file("/proc/cpuinfo").exists()) {
+            Map<String, Integer> socketToCore = [:]


Maybe add a comment that we expect this branch for all linux distros. I'm not sure if they all have proper /proc filesystems but I think this is fairly reasonable. At worst they get auto instead.

nik9000 · 2018-11-14T17:58:59Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

        project.tasks.withType(RandomizedTestingTask) {
            jvm "${project.runtimeJavaHome}/bin/java"
-            parallelism System.getProperty('tests.jvms', 'auto')
+            parallelism System.getProperty('tests.jvms',  defaultParallel)


Extra space.

nik9000 · 2018-11-14T17:59:21Z

TESTING.asciidoc

-By default the tests run on up to 4 JVMs based on the number of cores. If you
-want to explicitly specify the number of JVMs you can do so on the command
+By default the tests run on multiple processes using half the available processors 
+as reported by the JVM. 


Could you update this to mention physical cores?

nik9000 · 2018-11-14T18:00:24Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

+                    if (name == "cpu cores") {
+                        assert currentID.isEmpty() == false
+                        socketToCore[currentID] = Integer.valueOf(value)
+                    }


Clear currentID here too I think.

nik9000 · 2018-11-14T18:03:13Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

+                    }
+                }
+            })
+            defaultParallel = socketToCore.values().sum().toString();


This gets the number of logical cores, I think. I thought we wanted the number of physical cores here. So socketToCore.size(). I think.

Socket is for when you might have multiple CPU socket, as in multiple CPU chips, thus the sum.

alpar-t · 2018-11-14T19:52:25Z

Thanks @nik9000 . Ready for another round.

alpar-t · 2018-11-15T07:02:57Z

@elasticmachine test thsi please

alpar-t · 2018-11-15T07:03:14Z

@elasticmachine test this please

…arallel

nik9000

LGTM. I'll get the macOS one once you merge this.

Default to the number of physical cores across all sockets instead.

In elastic#35259 we switched the default number of VMs to fork for unit tests to the number of physical CPU cores. But because we could only get an accurate count on machines with a normal `/proc` filesystem, macOS machine did not pick up the new default. Given that macOS is a huge portion of developer machines, we'd like to get the right default there. This does that. It also moves the default-finding process from happening once per testing task to happening once at startup. This seems like a good choice in general, but a very good choice for macOS because we have to run a command to list the count.

In #35259 we switched the default number of VMs to fork for unit tests to the number of physical CPU cores. But because we could only get an accurate count on machines with a normal `/proc` filesystem, macOS machine did not pick up the new default. Given that macOS is a huge portion of developer machines, we'd like to get the right default there. This does that. It also moves the default-finding process from happening once per testing task to happening once at startup. This seems like a good choice in general, but a very good choice for macOS because we have to run a command to list the count.

The default of auto is capped at 4 cores

184c245

Lift the upper limit of the default. We heuristically divide by two because it was found that increasing past the number of physical cores does not yield benefits.

alpar-t added >non-issue :Delivery/Build Build or test infrastructure v7.0.0 v6.6.0 labels Nov 5, 2018

alpar-t requested a review from nik9000 November 5, 2018 15:39

jasontedor reviewed Nov 5, 2018

View reviewed changes

alpar-t added 3 commits November 6, 2018 13:03

Update docs

b8865d7

Merge remote-tracking branch 'origin/master' into run-more-tests-in-p…

0181e72

…arallel

Add physical core detection on Linux

99dff10

nik9000 requested changes Nov 14, 2018

View reviewed changes

PR review

533f480

Merge remote-tracking branch 'origin/master' into run-more-tests-in-p…

0091e8f

…arallel

nik9000 approved these changes Nov 16, 2018

View reviewed changes

alpar-t merged commit fda59ff into elastic:master Nov 19, 2018

alpar-t deleted the run-more-tests-in-parallel branch November 19, 2018 12:01

alpar-t added a commit that referenced this pull request Nov 19, 2018

The default of tests.jvms should not be capped (#35259)

2f55b78

Default to the number of physical cores across all sockets instead.

nik9000 mentioned this pull request Nov 21, 2018

Build: Pick default test jvms for macOS #35789

Merged

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Conversation

alpar-t commented Nov 5, 2018

Uh oh!

elasticmachine commented Nov 5, 2018

Uh oh!

jasontedor 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t commented Nov 14, 2018

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!

alpar-t commented Nov 14, 2018

Uh oh!

alpar-t commented Nov 15, 2018

Uh oh!

alpar-t commented Nov 15, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants