RFR: JDK-8223322: Improve concurrency in jpackage instances
Remove all non final static variables in jpackage java code (using InheritableThreadLocal for Logger and Argument instances) and remove sychronization in JPackageToolProvider. ------------- Commit messages: - JDK-8223322: Improve concurrency in jpackage instances - JDK-8223322: Improve concurrency in jpackage instances Changes: https://git.openjdk.java.net/jdk/pull/1829/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1829&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8223322 Stats: 214 lines in 8 files changed: 134 ins; 52 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/1829.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1829/head:pull/1829 PR: https://git.openjdk.java.net/jdk/pull/1829
On Thu, 17 Dec 2020 20:46:50 GMT, Andy Herrick <herrick@openjdk.org> wrote:
Remove all non final static variables in jpackage java code (using InheritableThreadLocal for Logger and Argument instances) and remove sychronization in JPackageToolProvider.
Changes looks fine, but are we sure that external 3rd party tools used by jpackage will work concurrently without any issues? ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
On Thu, 17 Dec 2020 20:46:50 GMT, Andy Herrick <herrick@openjdk.org> wrote:
Remove all non final static variables in jpackage java code (using InheritableThreadLocal for Logger and Argument instances) and remove sychronization in JPackageToolProvider.
Changes requested by asemenyuk (Committer). test/jdk/tools/jpackage/share/ConcurrentTest.java line 53:
51: JPackageCommand.helloAppImage("com.other/com.other.Hello") 52: .useToolProvider(true) 53: .setPackageType(PackageType.getDefault())
`.removeArgumentWithValue("--type")` can be used to make jpackage create default native package. This will eliminate need to introduce `packageType.getDefault()` method. However, I'd rather replace JPackageCommand with PackageTest in this test scenario. Something like this: final PackageTest cmd1 = new PackageTest() .configureHelloApp() .addInitializer(cmd -> { cmd.setArgumentValue("--name", "ConcurrentOtherInstaller"); }); `cmd1.run(PackageTest.Action.CREATE);` would result in a package creation but will no unpack or install will be attempted. PackageTest is better as it will run tests on created package making sure it is valid. ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
On Thu, 17 Dec 2020 23:24:02 GMT, Alexey Semenyuk <asemenyuk@openjdk.org> wrote:
Remove all non final static variables in jpackage java code (using InheritableThreadLocal for Logger and Argument instances) and remove sychronization in JPackageToolProvider.
Changes requested by asemenyuk (Committer).
I'd propose to update the test to run more that two concurrent instance of jpackage command.
test/jdk/tools/jpackage/share/ConcurrentTest.java line 68:
66: 67: cmd1.useToolProvider(false); 68: cmd1.useToolProvider(false);
Shouldn't this be `cmd2`?
I'd parametrize the test function with `useToolProvider` parameter. This will help to avoid copy/paste of `TKit.assertTrue(times...)` ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
On Thu, 17 Dec 2020 23:26:44 GMT, Alexey Semenyuk <asemenyuk@openjdk.org> wrote:
Changes requested by asemenyuk (Committer).
I'd propose to update the test to run more that two concurrent instance of jpackage command.
Changes looks fine, but are we sure that external 3rd party tools used by jpackage will work concurrently without any issues?
I don't think we can be sure of thread-safe behavior of the third party tools, the best thing we can do is thoroughly test (as requested below, run a lot more instances simultaneously). Main thing we are implementing and testing is that jpackage code itself is thread safe. ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
On Thu, 17 Dec 2020 23:26:44 GMT, Alexey Semenyuk <asemenyuk@openjdk.org> wrote:
I'd propose to update the test to run more that two concurrent instance of jpackage command.
yes - I'm planning for a lot more in next revision, your suggestion to use only PackageTest.Action.CREATE will go a long way to making that possible, but I have had problems with test infrastructure fixing the workDir based on test class and method, and then getting IOExceptions as various PackageTest's try to write the same content to the same work dir. I need to create infrastructure to allow each PackageTest to operate on separate workDir. ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
On Fri, 18 Dec 2020 14:59:06 GMT, Andy Herrick <herrick@openjdk.org> wrote:
I'd propose to update the test to run more that two concurrent instance of jpackage command.
I'd propose to update the test to run more that two concurrent instance of jpackage command.
yes - I'm planning for a lot more in next revision, your suggestion to use only PackageTest.Action.CREATE will go a long way to making that possible, but I have had problems with test infrastructure fixing the workDir based on test class and method, and then getting IOExceptions as various PackageTest's try to write the same content to the same work dir. I need to create infrastructure to allow each PackageTest to operate on separate workDir.
Reverting this PR to DRAFT while the above suggestions are implemented. ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
On Fri, 18 Dec 2020 15:00:13 GMT, Andy Herrick <herrick@openjdk.org> wrote:
I'd propose to update the test to run more that two concurrent instance of jpackage command.
yes - I'm planning for a lot more in next revision, your suggestion to use only PackageTest.Action.CREATE will go a long way to making that possible, but I have had problems with test infrastructure fixing the workDir based on test class and method, and then getting IOExceptions as various PackageTest's try to write the same content to the same work dir. I need to create infrastructure to allow each PackageTest to operate on separate workDir.
Reverting this PR to DRAFT while the above suggestions are implemented.
.removeArgumentWithValue("--type") can be used to make jpackage create default native package. This will eliminate need to introduce packageType.getDefault() method.
The above will cause an exception from JPackageCommand.execute() > ... > JPackageCommand.verifyIsOfType(), but perhaps that problem will go away if only JPackageTest.run(PackageTest.Action.CREATE) is used. ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
On Fri, 18 Dec 2020 14:59:06 GMT, Andy Herrick <herrick@openjdk.org> wrote:
I have had problems with test infrastructure fixing the workDir based on test class and method, and then getting IOExceptions as various PackageTest's try to write the same content to the same work dir
I don't think we need changes to test infrastructure to run PackageTest instances concurrently. This is how to create input for jpackage: final Path inputDir = TKit.workDir().resolve("input"); HelloApp.createBundle(JavaAppDesc.parse("hellp.jar:Hello"), inputDir); This is the function to initialize PackegTest instance that will use hello.jar initialized above to build a package and write output in unique directory inside of test work directory: private PackageTest initTest(Path inputDir, bool useToolProvider) { // Just in case if TKit.createTempDirectory is not reliable enough to be executed concurrently final Path outputDir; synchronized (this) { outputDir = TKit.createTempDirectory("output"); } return new PackageTest() .addInitializer(cmd -> { cmd.useToolProvider(useToolProvider); cmd.setArgumentValue("--input", inoutDir); cmd.setArgumentValue("--main-class", "Hello"); cmd.setArgumentValue("--dest", outputDir); }); } Glue it all together: @Test @Parameter("true") @Parameter("false") public void testIt(bool useToolProvider) { final Path inputDir = TKit.workDir().resolve("input"); HelloApp.createBundle(JavaAppDesc.parse("hellp.jar:Hello"), inputDir); final int TEST_COUNT = 5; List<Runnable> tasks = new ArrayList<>(); for (int i = 0; i != TEST_COUNT; i++) { tests.push(ThrowingRunnable.toRunnable(() -> initTest(inputDir, useToolProvider).run(PackageTest.Action.CREATE))); } ExecutorService exec = Executors.newCachedThreadPool(); tasks.stream().forEach(exec::execute); exec.shutdown(); } ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
On Thu, 17 Dec 2020 20:46:50 GMT, Andy Herrick <herrick@openjdk.org> wrote:
Remove all non final static variables in jpackage java code (using InheritableThreadLocal for Logger and Argument instances) and remove sychronization in JPackageToolProvider.
test/jdk/tools/jpackage/share/ConcurrentTest.java line 68:
66: 67: cmd1.useToolProvider(false); 68: cmd1.useToolProvider(false);
Shouldn't this be `cmd2`? ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
Remove all non final static variables in jpackage java code (using InheritableThreadLocal for Logger and Argument instances) and remove sychronization in JPackageToolProvider.
Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8223322: Improve concurrency in jpackage instances ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/1829/files - new: https://git.openjdk.java.net/jdk/pull/1829/files/dfa7c507..2baef323 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1829&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1829&range=00-01 Stats: 103 lines in 4 files changed: 33 ins; 43 del; 27 mod Patch: https://git.openjdk.java.net/jdk/pull/1829.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1829/head:pull/1829 PR: https://git.openjdk.java.net/jdk/pull/1829
On Thu, 17 Dec 2020 20:46:50 GMT, Andy Herrick <herrick@openjdk.org> wrote:
Remove all non final static variables in jpackage java code (using InheritableThreadLocal for Logger and Argument instances) and remove sychronization in JPackageToolProvider.
This pull request has been closed without being integrated. ------------- PR: https://git.openjdk.java.net/jdk/pull/1829
participants (3)
-
Alexander Matveev
-
Alexey Semenyuk
-
Andy Herrick