[🐛 Bug]: significant performance and memory usage - regressions when switching from Selenium 3 to 4

[christophs78]

What happened?

PrimeFaces comes with a suite of integrationstest: https://github.com/primefaces/primefaces/tree/master/primefaces-integration-tests

We currently use Selenium 3.141.59. When i run them on my dev-machine (8C/16T, 32 GB RAM) it looks like this:

With primefaces/primefaces#8145 we try to upgrade to Selenium 4.1.0. There it looks like this:

Additional my notebook get´s unresponsive.

We also run this integrationtests on Github actions. They take about twice the time with Selenium 4.1.0. (And we run into some follow-up problems probably because of worse performance due to resource starvation.)

There already has been a (to some degree) similar issue: #9536

How can we reproduce the issue?

See PrimeFaces-Github-links above.

Relevant log output

Selenium 4.1.0:
https://github.com/primefaces/primefaces/actions/runs/1539682360

Compare this with a run on 3.141.59:
https://github.com/primefaces/primefaces/runs/4413396941?check_suite_focus=true

Operating System

Windows 10, Ubuntu 20.04.3

Selenium version

Java 4.1.0

What are the browser(s) and version(s) where you see this issue?

Chrome 96.0.4664.45, Firefox 94.0.2

What are the browser driver(s) and version(s) where you see this issue?

ChromeDriver 96.0.4664.45, GeckoDriver 0.30.0

Are you using Selenium Grid?

No

[github-actions]

@christophs78, thank you for creating this issue. We will troubleshoot it as soon as we can.

---

Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

[christophs78]

Maybe this issue is somehow related to parallelism / concurrency.
We use junit.jupiter.execution.parallel.enabled=true to create one thread / browser instance per hardware-thread. This worked very well with Selenium 3.x. (--> on my dev-machine 16 threads)
Is it possible Selenium 4.1.0 has some issues in this area?

[cocorossello]

I think this is related to #9359

A workaround is also provided in the issue

[christophs78]

I think this is related to #9359

A workaround is also provided in the issue

Played around with this for one and a half hour. Yes it´s the same issue, but the workaround does not work.
I also checked out https://github.com/titusfortner/bug9359 and tried it on my machine. I reduced his settings from 12 to 2. It does not matter which values are returned by our JUnit5-CustomStrategy-impl.

Ah. It depends on the Java-version. The work-around does not work for Java 8, but it works for Java 11.
Update: It´s also (at least partial) broken for Java 17. (reproduceable with https://github.com/titusfortner/bug9359)

IMO #9359 needs to be reopened.

[titusfortner]

Well, looks like everyone now has to use Java 11.

I'm dismayed to be able to duplicate your findings.

I'll close this one and re-open that one. :-/

[pujagani]

@christophs78 Thank you for your findings and effort in solving this! I am sorry the earlier fix was not the perfect fix for this problem.
junit-team/junit5#1858 (comment) Mentions that the potential problem lies due to

selenium/java/src/org/openqa/selenium/remote/service/DriverService.java

Line 67 in 28070d2

private final ExecutorService executorService = Executors.newFixedThreadPool(2, r -> {

That change was made to support Junit 5, which now based on your findings works only for Java 11 unfortunately. Apologies that it does not work for all versions.
It was made since

selenium/java/src/org/openqa/selenium/remote/service/DriverService.java

Line 204 in 28070d2

CompletableFuture<StartOrDie> serverStarted = CompletableFuture.supplyAsync(() -> {

by default uses a ForkJoinPool, and that seemed to be the problem so we switched it to use a fixed thread pool.
Based on your findings, what do you think is primarily causing the problem? Any suggestions on how we can tackle it?

Thank you!

[christophs78]

@pujagani Thank´s for your reply. To be honest i currently have no good idea how to fix this within Selenium.
My personal feeling (being a senior java-dev, but not having extensive experience with async on java) is nested thread-pools (ForkJoinPool used buy JUnit 5 and ThreadPoolExecutor used by Selenium 4) is not the best idea. Maybe even OpenJDK is not perfect in this constellation.
I´d liked to play around a bit with Selenium - source and trying ideas, but IntelliJ-Bazel-plugin does not support IntelliJ 2021.3 yet. Maybe i can come back on this in a few weeks.
So I´ll try to do some sandbox-test´s with a sandbox-project where i try to reproduce the ThreadPoolExecutor nested within a ForkJoinPool and come back afterwards.
(Maybe you already saw junit-team/junit5#2805 as an idea how to tackle this issue from the other side. I´ll also look what the JUnit - team thinks about this.)

[titusfortner]

@christophs78 thanks again for your help with tracking this down.

Fwiw, JetBrains Toolbox makes it really simple to downgrade a minor version to something that supports Bazel. Also, if you can join us on the Selenium chat (IRC or Slack)— https://www.selenium.dev/support/#ChatRoom
Devs hang out in #selenium-tlc chat room, and we'd love to collaborate with you as best we can.

[christophs78]

Thanks for offering your collaboration. I´ll come back as soon as i have good questions to ask.

My efforts to reproduce this issue with a sandbox plain-java-project have not been successful yet. At least i understand some previous conversation regarding ForkJoinPool and implementing our own ParallelExecutionConfiguration, ParallelExecutionConfigurationStrategy makes sense.

So i tested https://github.com/titusfortner/bug9359 with each Java-version starting with 13. It work´s up to Java 16 and is broken with Java 17. (I´ll keep Java 8 aside for now. With Java 17 it is broken for Hotspot and J9.)
Next stop is to look what was modified in OpenJDK regarding concurrency between 16 and 17.

[christophs78]

Changes within java.util.concurrent for Java 17: https://github.com/openjdk/jdk/commits/jdk-17-ga/src/java.base/share/classes/java/util/concurrent
Java 16 was released on 19 Jan 2021. 8 commits between Java 16 GA and Java 17 GA.

openjdk/jdk@5b7b18c#diff-85448900741ca0f6d70b61cb62d38bbbd49b015f78c4efa67ea8cdc3ae15e9f4 and openjdk/jdk@5cfa8c9#diff-e398beb49cd8d3e6c2f3a8ca8eee97172c57d7f88f3ccd8a3c704632cab32f5f did some sgnificant changes to ForkJoinPool.

Any thought´s about this?

Maybe it´s a regression in Java 17. Would help to have a plain Java reproducer for this. If we are able to reproduce this with plain Java we may reach out to the OpenJDK-team.

[christophs78]

Side-note regarding Java 8: ForkJoinPool does not have a constructor to define things like maxPoolSize. This enhanced constructor was introduced with Java 9. - - > We propably can't fix this issue for Java 8.

[christophs78]

Java 17 - issue - reproducer: https://github.com/christophs78/ForkJoinPoolReproducer
Any idea how to approach the OpenJDK-team with this? https://bugs.openjdk.java.net/secure/Dashboard.jspa is readonly
(https://dev.karakun.com/rico/2021/02/22/openjdk-bug-tracker.html -> https://bugreport.java.com/bugreport/)

[cocorossello]

Hi, I'm not really sure if it does solve the selenium problem, but it does solve the reproducer test in JDK11 and 17...
christophs78/ForkJoinPoolReproducer#1

Taken from https://stackoverflow.com/questions/69768534/why-does-java-17-throw-a-rejectedexecutionexception-when-adding-tasks-to-a-forkj

[titusfortner]

Since we're discussing it here, I'll leave it open, and close the other one. :)

[titusfortner]

Do I have this right?

Java 8 isn't going to work; need to stick with JUnit 4 or Selenium 3.
Java 9 - 16 work with the approach that was outlined earlier.
Java 17 broke something, but fix needs to be made in the Java code and we just have to figure out how to get them to do that.

All in all this isn't as bad as I feared. The Java 8 thing sucks, but I think there are more 9-16 folks than 17 folks still.

[christophs78]

Hi, I'm not really sure if it does solve the selenium problem, but it does solve the reproducer test in JDK11 and 17... christophs78/ForkJoinPoolReproducer#1

Taken from https://stackoverflow.com/questions/69768534/why-does-java-17-throw-a-rejectedexecutionexception-when-adding-tasks-to-a-forkj

Excellent point!

I´m able to confirm this fixes the issue.

Did following within org.junit.platform.engine.support.hierarchical.ForkJoinPoolHierarchicalTestExecutorService:

private ForkJoinPool createForkJoinPool(ParallelExecutionConfiguration configuration) {
	ForkJoinWorkerThreadFactory threadFactory = new WorkerThreadFactory();
	return Try.call(() -> {
		// Try to use constructor available in Java >= 9
		Constructor<ForkJoinPool> constructor = ForkJoinPool.class.getDeclaredConstructor(Integer.TYPE,
			ForkJoinWorkerThreadFactory.class, UncaughtExceptionHandler.class, Boolean.TYPE, Integer.TYPE,
			Integer.TYPE, Integer.TYPE, Predicate.class, Long.TYPE, TimeUnit.class);
		Predicate<? super ForkJoinPool> predicateSaturate = pool -> true;
		return constructor.newInstance(configuration.getParallelism(), threadFactory, null, false,
			configuration.getCorePoolSize(), configuration.getMaxPoolSize(), configuration.getMinimumRunnable(),
			predicateSaturate, configuration.getKeepAliveSeconds(), TimeUnit.SECONDS);
	}).orElseTry(() -> {
		// Fallback for Java 8
		return new ForkJoinPool(configuration.getParallelism(), threadFactory, null, false);
	}).getOrThrow(cause -> new JUnitException("Failed to create ForkJoinPool", cause));
}

So i'll try to do a proper JUnit 5 PR.

[christophs78]

There´s already junit-team/junit5#2787 and junit-team/junit5#2792 covering this. -> We just have to wait for JUnit 5.9.

[pujagani]

Thank you @christophs78 for digging into this so well. Appreciate the efforts :)

[tkumar18]

Hello @christophs78 Do we have any workaround for Java 17 or need to wait for Junit 5.9?

[christophs78]

Hello @christophs78 Do we have any workaround for Java 17 or need to wait for Junit 5.9?

No good workaround. Disabling ParallelExecution, using Java 11 instead of 17....

[tkumar18]

@christophs78 I noticed for Java 17, if we keep getMinimumRunnable as 0 in the Custom Strategy it works fine and there is no issue. Can you please check once?

@OverRide
public ParallelExecutionConfiguration createConfiguration(ConfigurationParameters arg0) {
return this;
}

@Override
public int getCorePoolSize() {
    return 5;
}

@Override
public int getKeepAliveSeconds() {
    return 30;
}

@Override
public int getMaxPoolSize() {
    return 5;
}

@Override
public int getMinimumRunnable() {
    return 0;
}

@Override
public int getParallelism() {
    return 5;
}

[christophs78]

@tkumar18 : Excellent point! I´m able to confirm this helps.

[tkumar18]

@christophs78 Thanks for the confirmation. Will still need to wait for 5.9 though.

[ramaboddupally]

@tkumar18 @christophs78 can you please confirm if this issue resolved, what is the work around for now.

[christophs78]

https://github.com/primefaces/primefaces/blob/master/primefaces-integration-tests/src/test/java/org/primefaces/integrationtests/JUnit5Selenium4Strategy.java
works for Java 11 and 17

[titusfortner]

@christophs78 what will change with JUnit 5.9?

And thanks again for your work on this.

[christophs78]

Since @tkumar18 figured out returning 0 for getMinimumRunnable() makes Java 17 happy, there are IMO no more major reasons to wait for JUnit 5.9.
JUnit 5.9 (as far as we know know) will still require to provide a project-specific implementation of ParallelExecutionConfiguration and ParallelExecutionConfigurationStrategy. JUnit 5.9 will allow to set saturate to false via ParallelExecutionConfiguration. This is an alternative to minimumRunnable = 0.

[titusfortner]

Excellent. I'll close this issue then if we're not expecting any additional functionality. Thanks everyone for your assistance getting this sorted.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.