JDK 16 RFR of 8250580: Address reliance on default constructors in java.rmi
Hello,
Another bug in the quest to remove use of default constructors in the
JDK's public API, this time in the java.rmi module:
webrev: http://cr.openjdk.java.nhet/~darcy/8250580.0/
CSR: https://bugs.openjdk.java.net/browse/JDK-8250581
Patch below; thanks,
-Joe
---
old/src/java.rmi/share/classes/java/rmi/server/RMIClassLoaderSpi.java
2020-07-24 19:42:16.353847343 -0700
+++
new/src/java.rmi/share/classes/java/rmi/server/RMIClassLoaderSpi.java
2020-07-24 19:42:15.645847343 -0700
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2000, 2006, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2000, 2020, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -62,6 +62,11 @@
public abstract class RMIClassLoaderSpi {
/**
+ * Constructor for subclasses to call.
+ */
+ public RMIClassLoaderSpi() {}
+
+ /**
* Provides the implementation for
* {@link RMIClassLoader#loadClass(URL,String)},
* {@link RMIClassLoader#loadClass(String,String)}, and
JDK 16 RFR of JDK-8250578: Address reliance on default constructors in javax.sql
Hello,
Another module, another use of a default constructor. Please review the
fix and CSR for:
JDK-8250578: Address reliance on default constructors in javax.sql
CSR: https://bugs.openjdk.java.net/browse/JDK-8250579
http://cr.openjdk.java.net/~darcy/8250578.0/
Patch below.
Thanks,
-Joe
---
old/src/java.sql.rowset/share/classes/javax/sql/rowset/RowSetMetaDataImpl.java
2020-07-24 19:16:43.633847343 -0700
+++
new/src/java.sql.rowset/share/classes/javax/sql/rowset/RowSetMetaDataImpl.java
2020-07-24 19:16:42.941847343 -0700
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2003, 2014, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -52,6 +52,10 @@
* @since 1.5
*/
public class RowSetMetaDataImpl implements RowSetMetaData, Serializable {
+ /**
+ * Constructs a {@code RowSetMetaDataImpl} object.
+ */
+ public RowSetMetaDataImpl() {}
/**
* The number of columns in the RowSet object that
created
Re: RFR: JDK-8248239: jpackage adds some arguments twice in case it is re-executed by JLI
Looks good. Thanks, Alexander On 7/24/20 12:39 PM, Andy Herrick wrote: looks good /Andy On 7/21/2020 2:27 PM, Alexey Semenyuk wrote: Hi Aleksei, Looks good! - Alexey On 7/21/2020 11:42 AM, Aleksei Voitylov wrote: Hi, This is the updated fix which checks if LD_LIBRARY_PATH has been changed during jpackage executions: http://cr.openjdk.java.net/~avoitylov/webrev.8248239.03/ The fix stores hash from LD_LIBRARY_PATH in _JPACKAGE_LAUNCHER env variable. Until C++14 becomes mandatory for OpenJDK, a custom hash algorithm is used because standard C++ hash requires -std=c++11 or -std=gnu++11 compiler options. All test/jdk/tools/jpackage tests but ModulePathTest3.java pass on Linux x86_64, MacOSX x86_64, and Windows x86_64. The test ModulePathTest3.java is excluded in test/jdk/ProblemList.txt (8248418 generic-all). Thanks, -Aleksei On 26/06/2020 20:23, Alexey Semenyuk wrote: Hi Aleksei, I think the idea of partial reading data from cfg file when the launcher is restarted has a flaw. It would be better if app launcher can detect if it was restarted and if it was, not read cfg file at all, but pass command line arguments as is in JLI_Launch(). Let my think on ideas how to address this. - Alexey On 6/26/2020 7:16 AM, Aleksei Voitylov wrote: Hi Alexey, Thank you for looking into it. As far as using parent pid, it does not seem to work as example [1] demonstrates. The parent process remains the same after re-execution and does not become the current process. I checked passing arguments in the "ArgOption" section using several cases [2, 3, 4] with the proposed fix and app re-execution. The proposed fix handles this case well, and the results are the same as without the fix when the app is not re-executed. Case [3] where only JavaOptions without ArgOptions are passed to app looks suspicious because default ArgOptions are not used. But it works the same way without the proposed fix, which seems like a different issue. According to jpackage documentation: --arguments main class arguments Command line arguments to pass to the main class if no command line arguments are given to the launcher. I filed a separate issue [5] to handle that. Thanks, -Aleksei [1] cd jdk-dev make jdk-image export PATH=build/linux-x86_64-server-release/images/jdk/bin:$PATH export LD_LIBRARY_PATH=build/linux-x86_64-server-release/images/jdk/lib/server jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --verbose --java-options -Dparam1=Param1 ./output/app/bin/app -Dparam2=Param2 B B2 B - pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app pid: 15927, parent process: /bin/bash JvmLauncher args: 10 [/home/sample/jdk-dev/output/app/bin/app -Dparam1=Param1 --module-path /home/sample/jdk-dev/output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app pid: 15927, parent process: /bin/bash JvmLauncher args: 15 [/home/sample/jdk-dev/output/app/bin/app -Dparam1=Param1 --module-path /home/sample/jdk-dev/output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam1=Param1 --module-path /home/sample/jdk-dev/output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] - [2] jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --java-options -Dparam1=Param1 ./output/app/bin/app JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ] JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ] [3] jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --java-options -Dparam1=Param1 ./output/app/bin/app -Dparam2=Param2 JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ] JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ] [4] jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --java-options -Dparam1=Param1 ./output/app/bin/app -Dparam2=Param2 B B2 B JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] [5] https://bugs.openjdk.java.net/browse/JDK-8248397 On 24/06/2020 19:34, Alexey Semenyuk wrote: Aleksei, If I get it right, the fix would not work for the case when
Re: RFR(M): 8249963: Make the zlib implementation selectively configurable at startup
On 7/24/2020 7:48 AM, Volker Simonis wrote: I think it's reasonable to ask if the JDK really needs to support this complexity. I can't see much complexity here. Then I think we should start there. Until you can see the complexity here that is obvious to Alan, Lance, myself, and others, then there is no point discussing the specific technical issue. Every configuration knob or flag creates complexity, because it creates a transient environmental dependence -- yet another thing that can cause behavior to change even when running the same JDK on the same underlying platform. I think you are looking at _your patch_ and saying "it's not that complex", but that's not the kind of complexity we're talking about. We're talking about the number of potential axes that can interact with each other to cause surprising or hard-to-debug behavior. Can you honestly not imagine something going wrong with having Inflater use one zlib, Deflator another, and crc32 a third? Until now I'm only hearing mostly high-level ,theoretical arguments against the proposal rather than technical objections. That's like when your mother tells you not to run with scissors, complaining that her concern is only theoretical, because not only do we not know anyone who has injured themselves running with scissors, but even if we did, they were not running with THIS pair of scissors." But of course, your mother was right, and Alan (and all the others that reponded) are right too. One of the most important roles of JDK stewards is pushing back on unnecessary complexity. This is what Alan is doing. The change itself is quite small Small changes can still introduce complexity. (It's one line of code to enforce that methods in Java only have an odd number of checked exceptions on tuesdays, but that would surely be new complexity.) and it doesn't change any default behaviour at all so I didn't think a JEP will be required. Which means either it will not be tested, or we have to double the number of test modes. I don't think we have to test all (or even various) zlib which means customers using this will be running on an effectively untested configuration. Does that seem wise? Stepping back, we're in the classic trap where you've skipped over all the important discussion, and as a result we've gotten the obvious outcome, which is that we're talking about the wrong thing. Steps you should have run before getting tied to your solution, at a minimum, include: - Develop a clear and shared understanding about what the problem is - Develop consensus that it is a problem - Develop consensus that it is a problem that needs to be solved in the JDK - Brainstorm possible solutions, with tradeoffs, pros, and cons - Identify the best solution, and build consensus around that - Implement - Test - Review - Iterate But you skipped straight to "Implement", and are now surprised that you're getting pushback against what should have been steps 1 or 2. You are trying to drive the discussion to "what changes do I have to make to have this patch accepted", but the conversation we should be having is "should we solve this problem at all" and then "if so, is this the right solution." And it seems you're not finding anyone who is very compelled by either the problem or the solution. I realize it sucks when you've done a lot of work and someone says "but we don't want/need that"; this is why you socialize the idea first. Alan said in his first mail "I don't think the JDK should be in the business of ..." -- that's a clear "this is not a problem that needs to be solved in the JDK." That's why we start at the top of the list.
Re: RFR: JDK-8248239: jpackage adds some arguments twice in case it is re-executed by JLI
looks good /Andy On 7/21/2020 2:27 PM, Alexey Semenyuk wrote: Hi Aleksei, Looks good! - Alexey On 7/21/2020 11:42 AM, Aleksei Voitylov wrote: Hi, This is the updated fix which checks if LD_LIBRARY_PATH has been changed during jpackage executions: http://cr.openjdk.java.net/~avoitylov/webrev.8248239.03/ The fix stores hash from LD_LIBRARY_PATH in _JPACKAGE_LAUNCHER env variable. Until C++14 becomes mandatory for OpenJDK, a custom hash algorithm is used because standard C++ hash requires -std=c++11 or -std=gnu++11 compiler options. All test/jdk/tools/jpackage tests but ModulePathTest3.java pass on Linux x86_64, MacOSX x86_64, and Windows x86_64. The test ModulePathTest3.java is excluded in test/jdk/ProblemList.txt (8248418 generic-all). Thanks, -Aleksei On 26/06/2020 20:23, Alexey Semenyuk wrote: Hi Aleksei, I think the idea of partial reading data from cfg file when the launcher is restarted has a flaw. It would be better if app launcher can detect if it was restarted and if it was, not read cfg file at all, but pass command line arguments as is in JLI_Launch(). Let my think on ideas how to address this. - Alexey On 6/26/2020 7:16 AM, Aleksei Voitylov wrote: Hi Alexey, Thank you for looking into it. As far as using parent pid, it does not seem to work as example [1] demonstrates. The parent process remains the same after re-execution and does not become the current process. I checked passing arguments in the "ArgOption" section using several cases [2, 3, 4] with the proposed fix and app re-execution. The proposed fix handles this case well, and the results are the same as without the fix when the app is not re-executed. Case [3] where only JavaOptions without ArgOptions are passed to app looks suspicious because default ArgOptions are not used. But it works the same way without the proposed fix, which seems like a different issue. According to jpackage documentation: --arguments main class arguments Command line arguments to pass to the main class if no command line arguments are given to the launcher. I filed a separate issue [5] to handle that. Thanks, -Aleksei [1] cd jdk-dev make jdk-image export PATH=build/linux-x86_64-server-release/images/jdk/bin:$PATH export LD_LIBRARY_PATH=build/linux-x86_64-server-release/images/jdk/lib/server jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --verbose --java-options -Dparam1=Param1 ./output/app/bin/app -Dparam2=Param2 B B2 B - pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app pid: 15927, parent process: /bin/bash JvmLauncher args: 10 [/home/sample/jdk-dev/output/app/bin/app -Dparam1=Param1 --module-path /home/sample/jdk-dev/output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app pid: 15927, parent process: /bin/bash JvmLauncher args: 15 [/home/sample/jdk-dev/output/app/bin/app -Dparam1=Param1 --module-path /home/sample/jdk-dev/output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam1=Param1 --module-path /home/sample/jdk-dev/output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] - [2] jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --java-options -Dparam1=Param1 ./output/app/bin/app JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ] JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ] [3] jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --java-options -Dparam1=Param1 ./output/app/bin/app -Dparam2=Param2 JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ] JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ] [4] jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --java-options -Dparam1=Param1 ./output/app/bin/app -Dparam2=Param2 B B2 B JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] [5] https://bugs.openjdk.java.net/browse/JDK-8248397 On 24/06/2020 19:34, Alexey Semenyuk wrote: Aleksei, If I get it right, the fix would not work for the case when there are `arguments` properties in `ArgOptions` section of a config file. Why not just
Re: No way to disable Java Container Metrics
I’ve been monitoring the discussion on your jdk8u alias and I can’t help wondering if my original decision to provide two different implementations of this container detection logic was the best way to go. I didn’t want to have an all Java implementation since the VM needs to initialize it’s memory and cpu sizing very early on prior to its ability to run Java code. If we put all of the logic in the VM, then we’d end up with a pretty wide interface to the VM and more overhead extracting values (due to JNI) although the Java logic typically ends up in native code anyway. Having the functionality in the VM makes it easier to add JFR events. Having a pure Java implementation makes it easier to support the OS MBeans. The maintenance of supporting both implementations is a bit of a pain. Assuming no one has the time or desire to migrate the logic to the VM and provide Java wrapper logic, I’m ok with what you are proposing. It’s one step on the path. The VM support and the Java level support are really for two different consumers but I think it would be cleaner and less confusing to disable both on one flag rather than support two options. Bob. > On Jul 24, 2020, at 12:13 PM, Severin Gehwolf wrote: > > Hi, > > For hotspot one can disable container detection with a simple switch: > > $ java -XX:-UseContainerSupport -Xlog:os+container=trace -version > [0.000s][trace][os,container] OSContainer::init: Initializing Container > Support > [0.000s][trace][os,container] Container Support not enabled > openjdk version "15-internal" 2020-09-15 > OpenJDK Runtime Environment (build > 15-internal+0-adhoc.sgehwolf.openjdk-head-2) > OpenJDK 64-Bit Server VM (build 15-internal+0-adhoc.sgehwolf.openjdk-head-2, > mixed mode, sharing) > > The same cannot be achieved with the Java code, > jdk.internal.platform.Metrics.java and friends in the JDK. At the time > Metrics were added the only consumer of them was the Java Launcher via > -XshowSettings:system. This has changed with JDK-8226575: > OperatingSystemMXBean should be made container aware. > > It is known that in certain cases the container detection code will > detect for a system to be supposedly in a container where it actually > isn't: > https://bugs.openjdk.java.net/browse/JDK-8227006?focusedCommentId=14275604=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14275604 > > For hotspot there is a flag, to turn auto-detection off for exactly the > case when heuristics are wrong: -XX:-UseContainerSupport. However, this > flag has no effect on the Java metrics code. There is a risk that on > some systems the auto-detection will be wrong and might cause > regressions. > > I propose to make the Java metrics code adhere to -XX:+/- > UseContainerSupport flag. Do you think this would be useful? If yes, > I'll file a bug and propose a patch for it. > > Thoughts? Opinions? > > Thanks, > Severin >
OS X jpackage exception FYI
Doesn’t appear to be causing any problems to the application build?
WARNING: Using incubator modules: jdk.incubator.jpackage
14.0.2
Running [/usr/bin/SetFile, -c, icnC,
/var/folders/dh/91wmrk0n6lzfmr4tjhjmcfp4gn/T/jdk.incubator.jpackage15486534363011883928/images/FastRGraalHP/.VolumeIcon.icns]
Running [/usr/bin/SetFile, -a, C,
/var/folders/dh/91wmrk0n6lzfmr4tjhjmcfp4gn/T/jdk.incubator.jpackage15486534363011883928/images/FastRGraalHP]
Running [osascript,
/var/folders/dh/91wmrk0n6lzfmr4tjhjmcfp4gn/T/jdk.incubator.jpackage15486534363011883928/config/FastRGraalHP-dmg-setup.scpt]
/var/folders/dh/91wmrk0n6lzfmr4tjhjmcfp4gn/T/jdk.incubator.jpackage15486534363011883928/config/FastRGraalHP-dmg-setup.scpt:57:61:
execution error: Finder got an error: Can’t get disk "FastRGraalHP". (-1728)
java.io.IOException: Command [osascript,
/var/folders/dh/91wmrk0n6lzfmr4tjhjmcfp4gn/T/jdk.incubator.jpackage15486534363011883928/config/FastRGraalHP-dmg-setup.scpt]
exited with 1 code
at
jdk.incubator.jpackage/jdk.incubator.jpackage.internal.Executor.executeExpectSuccess(Executor.java:73)
at
jdk.incubator.jpackage/jdk.incubator.jpackage.internal.IOUtils.exec(IOUtils.java:179)
at
jdk.incubator.jpackage/jdk.incubator.jpackage.internal.IOUtils.exec(IOUtils.java:150)
at
jdk.incubator.jpackage/jdk.incubator.jpackage.internal.MacDmgBundler.buildDMG(MacDmgBundler.java:348)
at
jdk.incubator.jpackage/jdk.incubator.jpackage.internal.MacDmgBundler.bundle(MacDmgBundler.java:75)
at
jdk.incubator.jpackage/jdk.incubator.jpackage.internal.MacDmgBundler.execute(MacDmgBundler.java:451)
at
jdk.incubator.jpackage/jdk.incubator.jpackage.internal.Arguments.generateBundle(Arguments.java:641)
at
jdk.incubator.jpackage/jdk.incubator.jpackage.internal.Arguments.processArguments(Arguments.java:514)
at
jdk.incubator.jpackage/jdk.incubator.jpackage.main.Main.execute(Main.java:97)
at
jdk.incubator.jpackage/jdk.incubator.jpackage.main.Main.main(Main.java:51)
Command issued:
${PACKAGER} \
--verbose \
--input ../HalfPipe12.app/Contents/Java \
--icon GenericApp.icns \
--install-dir outputdir \
--name FastRGraalHP \
--main-jar halfpipe.jar \
--main-class us.hall.hp.common.LoaderLaunchStub \
--runtime-image
/Library/Java/JavaVirtualMachines/graalvm-ce-java11-20.1.0-dev/Contents/Home \
--java-options '-Xmx1024m -XX:+UseG1GC -XX:MaxGCPauseMillis=50
-Djava.security.policy=$APPDIR/all.policy -Dapple.laf.useScreenMenuBar=true
-Dcom.apple.mrj.application.apple.menu.about.name=HalfPipe -Dconsole=pane' \
--mac-package-identifier us.hall.FastRGraalHP
Re: [15] 8235792: LineNumberReader.getLineNumber() behavior is inconsistent with respect to EOF
The CSR [1] has been updated as described below.
Thanks,
Brian
[1] https://bugs.openjdk.java.net/browse/JDK-8241020
> On Jul 22, 2020, at 11:36 AM, Brian Burkhalter
> wrote:
>
> Yes, I think that the sentence
>
> + * Line terminators are compressed into single newline
> + * ('\n') characters.
>
> should be added to the specs of read(char[],int,int) and readLine(), or some
> equivalent statement be added to the class level doc.
>
>> Besides, the JDK 14 API states that the invocation
>> "Returns:
>> The number of *bytes* read, or -1 if the end of the stream has already been
>> reached"
>>
>> I think it should say "characters" rather than "bytes" but perhaps this has
>> already been corrected.
>
> No, it has not been correct and I agree that it should be “characters,” not
> “bytes.”
No way to disable Java Container Metrics
Hi, For hotspot one can disable container detection with a simple switch: $ java -XX:-UseContainerSupport -Xlog:os+container=trace -version [0.000s][trace][os,container] OSContainer::init: Initializing Container Support [0.000s][trace][os,container] Container Support not enabled openjdk version "15-internal" 2020-09-15 OpenJDK Runtime Environment (build 15-internal+0-adhoc.sgehwolf.openjdk-head-2) OpenJDK 64-Bit Server VM (build 15-internal+0-adhoc.sgehwolf.openjdk-head-2, mixed mode, sharing) The same cannot be achieved with the Java code, jdk.internal.platform.Metrics.java and friends in the JDK. At the time Metrics were added the only consumer of them was the Java Launcher via -XshowSettings:system. This has changed with JDK-8226575: OperatingSystemMXBean should be made container aware. It is known that in certain cases the container detection code will detect for a system to be supposedly in a container where it actually isn't: https://bugs.openjdk.java.net/browse/JDK-8227006?focusedCommentId=14275604=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14275604 For hotspot there is a flag, to turn auto-detection off for exactly the case when heuristics are wrong: -XX:-UseContainerSupport. However, this flag has no effect on the Java metrics code. There is a risk that on some systems the auto-detection will be wrong and might cause regressions. I propose to make the Java metrics code adhere to -XX:+/- UseContainerSupport flag. Do you think this would be useful? If yes, I'll file a bug and propose a patch for it. Thoughts? Opinions? Thanks, Severin
Re: RFR(M): 8249963: Make the zlib implementation selectively configurable at startup
On Fri, Jul 24, 2020 at 2:15 PM Alan Bateman wrote: > > > On 24/07/2020 12:48, Volker Simonis wrote: > > : > > > > I can't see much complexity here. If you look at the change you'll see > > that it's rather trivial. All it does is substituting some direct > > calls into the zlib library by indirect calls through > > function pointers. > > > I don't think the JDK should be in the business of loading several > versions of zlib at the same time and using some functions from one > version, and some functions from another. Have you explored solutions > that don't burden the JDK? Has there been any attempt to bring the > performance improvements from the different sources into one build as > that seems to be what you are really looking for. > I really don't want to maintain yet another native zlib clone or a merge of existing ones. There are plenty of them available with their own strengths and weaknesses. I only want to make it easy and convenient for everybody to consume them from the JDK. The whole proposal is really just a simpler, more standard (because system independant) and more powerful way of using LD_LIBRAY_PATH. Additionally, it gives you the choice to use an alternative implementation even on platforms like Windows where we currently can't link dynamically because a default system version doesn't exist. Until now I'm only hearing mostly high-level ,theoretical arguments against the proposal rather than technical objections. Why are you so strictly against providing this additional choice to users which comes at almost zero costs for the JDK? > I would expect most/all of the Linux distributions to configure > --with-zlib=system as they don't want a zlib in the JDK run-time image. > So it might be unusual to build with --with-zlib=bundled and then expect > to be able to use an alternative zlib. There was a good discussion on > this topic on build-dev in 2016 as there was interest from Intel > engineers at the time to be able to use their accelerated library. > Yes, I know that discussion [1] and the previous one from Sandhya in 2015 [2] as well. The latter was a similar, but much more limited idea compared to my solution. It prosed an option/property to switch between the bundled and the system version at startup. The former was merely about changing the build default from "bundled" to "system" on Linux/Solaris. During that discussion, concerns were raised and finally disregarded that dynamic linking might introduce unpredictable risks compared to the bundled solution. I think my proposal is really combining and extending all the requests and concerns of the previous discussions in an ideal way by offering maximum flexibility paired with the ability of a safe fall-back at virtually no costs. [1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/thread.html#38649 [2] https://mail.openjdk.java.net/pipermail/core-libs-dev/2015-March/thread.html#32106 > Separately, I think it would be useful to explore some of the examples > to see if they make use of the Vector API or if there are opportunities > to do pure Java implementations that would benefit from the runtime > compilers. > I've briefly looked into the alternative implementations and they all make use of vector instructions, assembler coding and optimizations like manual loop unrolling etc. Most of these optimizations are currently hard to realize in a bundled, native version because we don't have a framework in the class library for dynamically checking CPU features. Implementing Inflation/Deflation in pure Java would surely be an interesting project but from my point of view not very realistic in the foreseeable future (I remember you were already throwing in similar ideas in the 2016 discussion mentioned above :) Best regards, Volker > -Alan
Re: RFR: JDK-8248239: jpackage adds some arguments twice in case it is re-executed by JLI
Thanks Alexey for the review! Unless we need another review, would you sponsor the change? Thanks, -Aleksei On 21/07/2020 21:27, Alexey Semenyuk wrote: > Hi Aleksei, > > Looks good! > > - Alexey > > On 7/21/2020 11:42 AM, Aleksei Voitylov wrote: >> Hi, >> >> This is the updated fix which checks if LD_LIBRARY_PATH has been changed >> during jpackage executions: >> >> http://cr.openjdk.java.net/~avoitylov/webrev.8248239.03/ >> >> The fix stores hash from LD_LIBRARY_PATH in _JPACKAGE_LAUNCHER env >> variable. Until C++14 becomes mandatory for OpenJDK, a custom hash >> algorithm is used because standard C++ hash requires -std=c++11 or >> -std=gnu++11 compiler options. >> >> All test/jdk/tools/jpackage tests but ModulePathTest3.java pass on Linux >> x86_64, MacOSX x86_64, and Windows x86_64. The test ModulePathTest3.java >> is excluded in test/jdk/ProblemList.txt (8248418 generic-all). >> >> Thanks, >> >> -Aleksei >> >> On 26/06/2020 20:23, Alexey Semenyuk wrote: >>> Hi Aleksei, >>> >>> I think the idea of partial reading data from cfg file when the >>> launcher is restarted has a flaw. >>> It would be better if app launcher can detect if it was restarted and >>> if it was, not read cfg file at all, but pass command line arguments >>> as is in JLI_Launch(). >>> Let my think on ideas how to address this. >>> >>> - Alexey >>> >>> On 6/26/2020 7:16 AM, Aleksei Voitylov wrote: Hi Alexey, Thank you for looking into it. As far as using parent pid, it does not seem to work as example [1] demonstrates. The parent process remains the same after re-execution and does not become the current process. I checked passing arguments in the "ArgOption" section using several cases [2, 3, 4] with the proposed fix and app re-execution. The proposed fix handles this case well, and the results are the same as without the fix when the app is not re-executed. Case [3] where only JavaOptions without ArgOptions are passed to app looks suspicious because default ArgOptions are not used. But it works the same way without the proposed fix, which seems like a different issue. According to jpackage documentation: --arguments main class arguments Command line arguments to pass to the main class if no command line arguments are given to the launcher. I filed a separate issue [5] to handle that. Thanks, -Aleksei [1] cd jdk-dev make jdk-image export PATH=build/linux-x86_64-server-release/images/jdk/bin:$PATH export LD_LIBRARY_PATH=build/linux-x86_64-server-release/images/jdk/lib/server jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --verbose --java-options -Dparam1=Param1 ./output/app/bin/app -Dparam2=Param2 B B2 B - pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app pid: 15927, parent process: /bin/bash JvmLauncher args: 10 [/home/sample/jdk-dev/output/app/bin/app -Dparam1=Param1 --module-path /home/sample/jdk-dev/output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app pid: 15927, parent process: /bin/bash JvmLauncher args: 15 [/home/sample/jdk-dev/output/app/bin/app -Dparam1=Param1 --module-path /home/sample/jdk-dev/output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam1=Param1 --module-path /home/sample/jdk-dev/output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] - [2] jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --java-options -Dparam1=Param1 ./output/app/bin/app JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ] JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ] [3] jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --java-options -Dparam1=Param1 ./output/app/bin/app -Dparam2=Param2 JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ] JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ] [4] jpackage --dest output --name app --type app-image --module-path input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" --java-options -Dparam1=Param1
Re: RFR(M): 8249963: Make the zlib implementation selectively configurable at startup
On 24/07/2020 12:48, Volker Simonis wrote: : I can't see much complexity here. If you look at the change you'll see that it's rather trivial. All it does is substituting some direct calls into the zlib library by indirect calls through function pointers. I don't think the JDK should be in the business of loading several versions of zlib at the same time and using some functions from one version, and some functions from another. Have you explored solutions that don't burden the JDK? Has there been any attempt to bring the performance improvements from the different sources into one build as that seems to be what you are really looking for. I would expect most/all of the Linux distributions to configure --with-zlib=system as they don't want a zlib in the JDK run-time image. So it might be unusual to build with --with-zlib=bundled and then expect to be able to use an alternative zlib. There was a good discussion on this topic on build-dev in 2016 as there was interest from Intel engineers at the time to be able to use their accelerated library. Separately, I think it would be useful to explore some of the examples to see if they make use of the Vector API or if there are opportunities to do pure Java implementations that would benefit from the runtime compilers. -Alan
Re: 8248248: [macos] EmptyFolderPackageTest.java fails EmptyFolderPackageTest-dmg-setup.scpt exited with 134 code
looks good. /Andy On 7/23/2020 7:17 PM, alexander.matv...@oracle.com wrote: http://cr.openjdk.java.net/~almatvee/8248248/webrev.01/ - Added INFINITE_TIMEOUT instead of -1. - Fix actually did not work correctly and process never timeout, since we were reading process output and wait with timeout never executed, since reading output was continue until process terminated. Fix by forcing using file for output, since in this case we will wait for process before reading output. Thanks, Alexander On 7/20/20 1:49 PM, Alexey Semenyuk wrote: Looks good. Minor suggestion: introduce a constant for infinite timeout and use it instead of "-1". - Alexey On 7/20/2020 4:43 PM, alexander.matv...@oracle.com wrote: Please review the jpackage fix for bug [1] at [2]. It is not clear why script was hanging for more than 7 minutes which caused test to timeout. Fixed by limiting script execution time to 3 minutes. Also, EmptyFolderPackageTest was removed from ProblemList. [1] https://bugs.openjdk.java.net/browse/JDK-8248248 [2] http://cr.openjdk.java.net/~almatvee/8248248/webrev.00/ Thanks, Alexander
Re: RFR(M): 8249963: Make the zlib implementation selectively configurable at startup
On Thu, Jul 23, 2020 at 8:48 PM Alan Bateman wrote: > > On 23/07/2020 18:18, Volker Simonis wrote: > > Hi, > > Hi Alan, thanks for promptly looking into my proposal. Please find my answers inline: > > can I please get some reviews for the following small enhancement > > which will allow you to configure different zlib implementations at VM > > start-up and get up to 100% better throughput for compression and > > about 50% better throughput for decompression (depending on the > > selected zlib implementation). We're using a similar change > > productively in Amazon since quite a while with good results on > > compression/decompression heavy workloads. > > > > As usual, my write-up is moch longer and much more complex than the > > change itself :) > Yes, this needs discussion and agreement before starting an RFR. That's why we're here :) I'm happy to discuss all your concerns. But I don't think it does any harm to have a working implementation and detailed benchmark data as a basis for such a discussion :) > As you've summarized in the JIRA issue, we have configure > --with-zlib=system and --with-zlib=bundled today. The former is the way > to build when what to make it possible to use an alternative zlib. The > new requirement that I think you are brining up is the case where > someone wants to use several zlib libraries at the same time, because > they somehow know that one version has superior performance than the > other on some functions. That's true. And in addition it allows always choosing an alternative (e.g. the system) version even if the JDK was configured with a "bundled" zlib. That's not possible today and from my point of view already worth this change alone. > I think it's reasonable to ask if the JDK > really needs to support this complexity. I can't see much complexity here. If you look at the change you'll see that it's rather trivial. All it does is substituting some direct calls into the zlib library by indirect calls through function pointers. > I could imagine testing or > support folks in tears when they see the implications of this. Have you I don't see any implications for testing here. Please see my answer to Lance's mail for more details. > looked at alternatives that would benefit other users of zlib, e.g. an > interposer that dispatches to the appropriate zlib based on configuration? But that's exactly what I've implemented - a simple interposer which dispatches between various zlib implemations. It's pretty simple and convenient to use because of the fact that a lot of alternative, but fully API-cpmatible zlib implementations already exist today. I don't want to create and maintain a new, native library. I just want to enable users to leverage the existing ones. Best regards, Volker > > -Alan >
Re: RFR(M): 8249963: Make the zlib implementation selectively configurable at startup
On Thu, Jul 23, 2020 at 9:35 PM Lance Andersen wrote: > > Hi Volker, > Hi Lance, thanks for looking at my proposal. Please find my comments inline: > > A change such as this I believe would require a JEP to fully define the > changes/risks/benefits > The change itself is quite small and it doesn't change any default behaviour at all so I didn't think a JEP will be required. I've summarized the risks in the JBS isse and my initial mail. The only risk I see is related to the wrong usage of the ZipEntry in conjunction with ZipInputStream/ZipOutputStream. These problems already exist today. They may only be revealed more often if somebody takes advantage of the proposed feature. But I have to repeat myself: the proposed feature gives interested parties an option to selectively use alternative implementations. It's an "opt-in" feature and we always have the safe-guard to fall back to the default implementation (be it the bundled or the system library). The benefit of being able to compress data at double speed seems to make this change worthwhile, especially if we take into account that it won't affect anybody who is not using this feature. However, if it's the general consensus and the only way to get this in, I'll be happy to rewrite all this into a JEP. > I do have concerns about using multiple implementations for > compression/decompression. From a testing and support perspective, this adds > additional burden. What type of testing matrix are you proposing? > I don't think we have to test all (or even various) zlib implementations as part of the regular OpenJDK testing. I'll be happy to add some tests which test the (rather trivial) loading and dispatching mechanism to make sure it works correctly and has no impact on the default implementation. Besides that, everybody who chooses to use an alternative implementation can test this however he likes. One simple possibility would be to run the "jdk/java/util/zip" and "jdk/jdk/nio/zipfs" JTreg tests with "vmoptions" set to his favourite implementation (e.g. "vmoptions:-Dorg.openjdk.zlib.implementation.deflate=super-libz.so"). But that's up to the user. We only make sure the selection mechanism is working. We also don't do any special testing today just because the user can use LD_LIBRARY_PATH/LD_PRELOAD to choose a different zlib version. If the user does that, it's up to him to make sure that his version behaves as expected. The feature proposed by me is just a more convenient and more powerful way for simplifying things which are already possible today. > Thought has to be given as to what it would mean to be compatible which could > require updates to the compatibility rules for JCK testing. > Sorry, but I can't see how this is related to JCK at all. The JDK has a long history of implementing some of its features by leveraging native libraries. And it is not uncommon that various native libraries can be used to implement the same feature. Two examples which pop into my head immediately are the two font renderers T2K / Freetype and the Marlin / Pisces graphics renderers. The latter can be controlled with the system property "-Dsun.java2d.renderer" which is very similar to what I propose. The mere fact of having several implementations doesn't have any impact on JCK testing at all (i.e. there's no T2K or Pices mode in the TCK). You just certify a default implementation but give users the option to choose other implementations in cases where they can be beneficial. You may argue that all the different implementations I've mentioned in my previous example are (or have been bundled) with the JDK itself. But from my point of view that makes no fundamental difference. I would be happy to add two new zlib implementations to the OpenJDK sources (and in fact that's how our internal solution is working), but I think that would be a much more controversial change. Finally, you are already doing JCK testing today with a JDK which is dynamically linked against the zlib on the test system but which is in general different from the zlib version on the system where the JDK will finally be installed and running and I don't think that has been a problem until now. > I am also concerned whether the proposed change adds enough value for the > additional complexity (and support burden) it brings. > The support burden is zero. The only change to OpenJDK is to replace the direct calls into the zlib library by indirect calls through function pointers. Once these (rather trivial) changes are reviewed they can live there without being touched for the next twenty years (much like the current implementation has outlived the last twenty years without any basic changes :) I hope the possibility to compress data at double speed will be appealing to others. For us at Amazon it's definitely worth it :) > Perhaps a proposal to replace the current zlib with an alternative might be > an easier path forward > As I wrote, there's no single library which performs
