Re: RFR: 8324573: HashMap::putAll add notes for conservative resizing [v5]
On Fri, 16 Feb 2024 23:30:22 GMT, Joshua Cao wrote:
>> Add notes for `HashMap::putAll()` conservative resizing.
>>
>> Note: everything below this line is from the original change. After
>> discussion, we decided to keep the conservative resizing, but we should add
>> an `@implNote` for the decision.
>>
>> ---
>>
>> This change mirrors what we did for ConcurrentHashMap in
>> https://github.com/openjdk/jdk/pull/17116. When we add all entries from one
>> map to anther, we should resize that map to the size of the sum of both maps.
>>
>> I used the command below to run the benchmarks. I set a high heap to reduce
>> garbage collection noise.
>>
>> java -Xms25G -jar benchmarks.jar -p size=10 -p addSize=10 -gc true
>> org.openjdk.bench.java.util.HashMapBench
>>
>>
>> Before change
>>
>>
>> Benchmark(addSize)(mapType) (size) Mode Cnt Score
>> Error Units
>> HashMapBench.putAll 10 HASH_MAP 10 avgt4 22.927 ±
>> 3.170 ms/op
>> HashMapBench.putAll 10 LINKED_HASH_MAP 10 avgt4 25.198 ±
>> 2.189 ms/op
>>
>>
>> After change
>>
>>
>> Benchmark(addSize)(mapType) (size) Mode Cnt Score
>> Error Units
>> HashMapBench.putAll 10 HASH_MAP 10 avgt4 16.780 ±
>> 0.526 ms/op
>> HashMapBench.putAll 10 LINKED_HASH_MAP 10 avgt4 19.721 ±
>> 0.349 ms/op
>>
>>
>> We see about average time improvements of 26% in HashMap and 20% in
>> LinkedHashMap.
>>
>> ---
>>
>> In the worse case, we may have two maps with identical keys. In this case,
>> we would aggressively resize when we do not need to. I'm also adding an
>> additional `putAllSameKeys` benchmark.
>>
>> Before change:
>>
>>
>> Benchmark (addSize)(mapType)
>> (size) Mode CntScore Error Units
>> HashMapBench.putAllSameKeys10 HASH_MAP
>> 10 avgt 6.956 ms/op
>> HashMapBench.putAllSameKeys:gc.alloc.rate 10 HASH_MAP
>> 10 avgt 1091.383 MB/sec
>> HashMapBench.putAllSameKeys:gc.alloc.rate.norm 10 HASH_MAP
>> 10 avgt 7968871.917B/op
>> HashMapBench.putAllSameKeys:gc.count 10 HASH_MAP
>> 10 avgt ≈ 0 counts
>> HashMapBench.putAllSameKeys10 LINKED_HASH_MAP
>> 10 avgt 8.417 ms/op
>> HashMapBench.putAllSameKeys:gc.alloc.rate 10 LINKED_HASH_MAP
>> 10 avgt 992.543 MB/sec
>> HashM...
>
> Joshua Cao has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains six additional commits since
> the last revision:
>
> - Add note about conservative resizing
> - Merge branch 'master' into hashmap
> - extract msize variable
> - Use max of both sizes and other maps size in case of overflow
> - Add benchmark with all duplicate keys
> - 8324573: HashMap::putAll should resize to sum of both map sizes
I don't think we need to document what this method is *not* doing or what it
*could* do. We should document what the current implementation *is* doing and
give advice how the impact of the current implementation can be mitigated.
Something like:
> {@code HashMap}'s resize policy is intentionally conservative to avoid an
> unnecessarily large capacity if {@code m} contains many duplicate keys. This
> can lead to a potentially expensive, extra resize operation. In order to
> avoid such an additional resize operation callers of {@code putAll} can
> either use the {@code HashMap(int initialCapacity)} constructor to ensure
> that this map has a large enough capacity or they can call {@code
> newHashMap(int numMappings)} before calling {@code putAll()} to ensure that
> the map is only resized and copied once.
Please fix the tags accordingly, I'm not fluent in JavaDoc :)
-
Changes requested by simonis (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17544#pullrequestreview-1908264266
Re: RFD: java.util.jar.Manifest.make72Safe has an invalid @deprecation tag
Hello Eirik, I think that method (and its references in some test code comments) can be removed. Like you note it's a package private method within the java.util.jar package and I don't see its usage (other than test code comments) anywhere within the JDK repo. That @Deprecated annotation on that method seems to have been introduced as part of https://bugs.openjdk.org/browse/JDK-8066619 (review thread https://mail.openjdk.org/pipermail/core-libs-dev/2018-December/057030.html). I'm guessing it likely was an oversight to have added that annotation to an internal method instead of just removing its usage. -Jaikiran On 29/02/24 3:09 am, Eirik Bjørsnøs wrote: Hi, The non-public method java.util.jar.Manifest.make72Safe is marked @Deprecated(since="13"). However, the Javadoc comment has a '@deprecation' tag which presumably should have been `@deprecated`. I first thought of proposing a PR to fix the invalid tag, but then I noticed that the method is non-public and has no callers in OpenJDK. Not sure I understand why this internal method was marked deprecated in the first place and not simply removed? Are we worried about people calling this reflectively? What would be the best way to go about this? (a) Do nothing (b) Just fix the tag @deprecation -> @deprecated (c) Fix the tag, add forRemoval=true (d) Just delete the method altogether (it's internal after all). Cheers, Eirik.
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Thu, 29 Feb 2024 01:54:54 GMT, Chris Hennick wrote: >> Chris Hennick has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Bug fix: add-exports was for wrong package > > @turbanoff @rgiulietti This is a follow-up to my previously merged #8131. @Pr0methean I'm not familiar with this code, nor with the underlying theory, so I'm not really able to give a meaningful review. - PR Comment: https://git.openjdk.org/jdk/pull/17703#issuecomment-1971114920
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries
On Thu, 29 Feb 2024 06:34:42 GMT, David Holmes wrote: > I can imagine it could be used to allow "hot patching" of the installed > JDK/JRE. Whether anyone has ever needed to do so is another matter. I suggest > at least adding a Release Note. Added release note. - PR Comment: https://git.openjdk.org/jdk/pull/18050#issuecomment-1971267771
Re: RFR: 8319648: java/lang/SecurityManager tests ignore vm flags [v2]
On Tue, 27 Feb 2024 13:29:07 GMT, Matthew Donovan wrote: >> In this PR I updated the tests to use the newer >> ProcessTools.createTestJavaProcessBuilder methods to pass VM options to >> child processes. > > Matthew Donovan has updated the pull request incrementally with one > additional commit since the last revision: > > removed unused import statement Marked as reviewed by mullan (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17878#pullrequestreview-1909616438
creating macOS universal applications
For anyone interested in building Java-based macOS universal applications, it is possible to create a bundled application that contains two Java runtimes, one for x86 and one for arm. A custom launcher is required to select the appropriate runtime based on the execution environment. I have created such a launcher; it is a very small modification of the macOS launcher used by jpackage. The custom launcher is available on GitHub: https://github.com/violetlib/mac-universal-java-launcher There is some discussion of this issue in JDK-8266259 [https://bugs.openjdk.org/browse/JDK-8266259]. So far, there does not seem to be much interest in this topic. On a related note, is there a place where developers using Java on macOS who are not associated with OpenJDK can share information?
Re: RFD: java.util.jar.Manifest.make72Safe has an invalid @deprecation tag
Hi Eirik, I also be we should be OK to remove. That being said, as part of the change, we should update open/test/jdk/sun/security/tools/jarsigner/PreserveRawManifestEntryAndDigest.java Best Lance On Feb 29, 2024, at 7:38 AM, Jaikiran Pai wrote: Hello Eirik, I think that method (and its references in some test code comments) can be removed. Like you note it's a package private method within the java.util.jar package and I don't see its usage (other than test code comments) anywhere within the JDK repo. That @Deprecated annotation on that method seems to have been introduced as part of https://bugs.openjdk.org/browse/JDK-8066619 (review thread https://mail.openjdk.org/pipermail/core-libs-dev/2018-December/057030.html). I'm guessing it likely was an oversight to have added that annotation to an internal method instead of just removing its usage. -Jaikiran On 29/02/24 3:09 am, Eirik Bjørsnøs wrote: Hi, The non-public method java.util.jar.Manifest.make72Safe is marked @Deprecated(since="13"). However, the Javadoc comment has a '@deprecation' tag which presumably should have been `@deprecated`. I first thought of proposing a PR to fix the invalid tag, but then I noticed that the method is non-public and has no callers in OpenJDK. Not sure I understand why this internal method was marked deprecated in the first place and not simply removed? Are we worried about people calling this reflectively? What would be the best way to go about this? (a) Do nothing (b) Just fix the tag @deprecation -> @deprecated (c) Fix the tag, add forRemoval=true (d) Just delete the method altogether (it's internal after all). Cheers, Eirik. [oracle_sig_logo.gif] Lance Andersen | Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: RFR: JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns [v5]
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8317756)
> which defines the behavior for creating ChoiceFormats with incorrect
> patterns. The wording is added to both the ChoiceFormat constructor and
> ChoiceFormat::applyPattern method.
>
> While ideally the inconsistent behavior itself could be fixed, this behavior
> has been long-standing for 20+ years and the benefit of consistent error
> handling does not outweigh the risk of breaking applications that may be
> relying on the "expected" incorrect behavior.
>
> Examples of the range of behavior, (all examples violate the pattern syntax
> defined in the class description)
>
>
> // no limit -> throws an expected IllegalArgumentException
> var a = new ChoiceFormat("#foo");
> // no limit or relation in the last subPattern -> discards the incorrect
> portion, 'baz' and continues
> var b = new ChoiceFormat("0#foo|1#bar|baz");
> b.format(2); // returns 'bar'
> // no relation or limit -> discards the incorrect portion, 'foo' and continues
> var c = new ChoiceFormat("foo");
> c.format(1); // throws AIOOBE
Justin Lu has updated the pull request incrementally with two additional
commits since the last revision:
- include ascending order stipulation in constuctor as well
- include apiSpec wording, move to patterns section
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17856/files
- new: https://git.openjdk.org/jdk/pull/17856/files/f1f1dc41..8e5bbe05
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17856&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17856&range=03-04
Stats: 34 lines in 1 file changed: 10 ins; 14 del; 10 mod
Patch: https://git.openjdk.org/jdk/pull/17856.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17856/head:pull/17856
PR: https://git.openjdk.org/jdk/pull/17856
RFR: 8326915: NPE when a validating parser is restricted
Fix a NPE when a validating parser is restricted by the JDKCatalog resolve property. Also slightly improved the error msg with the catalog name. Test: new test added existing test CatalogSupport5 would fail without the Null check on ErrorReporter. It's a separate issue not covered by this fix. - Commit messages: - 8326915: NPE when a validating parser is restricted Changes: https://git.openjdk.org/jdk/pull/18071/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18071&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326915 Stats: 116 lines in 5 files changed: 101 ins; 0 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/18071.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18071/head:pull/18071 PR: https://git.openjdk.org/jdk/pull/18071
Re: RFR: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used
On Wed, 28 Feb 2024 12:32:17 GMT, Matthias Baesken wrote: > Looks okay to me, but could we print here `RuntimeException(jmodFile + " is > expected not to include native debug symbols` not only the jmod but also the > unwanted file(s) ? This information is already printed in isNativeDebugSymbol. So in case of failure, you'll find the culprit in the test output. - PR Comment: https://git.openjdk.org/jdk/pull/17990#issuecomment-1972000461
Re: RFR: JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns [v5]
On Thu, 29 Feb 2024 20:07:14 GMT, Justin Lu wrote:
>> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8317756)
>> which defines the behavior for creating ChoiceFormats with incorrect
>> patterns. The wording is added to both the ChoiceFormat constructor and
>> ChoiceFormat::applyPattern method.
>>
>> While ideally the inconsistent behavior itself could be fixed, this behavior
>> has been long-standing for 20+ years and the benefit of consistent error
>> handling does not outweigh the risk of breaking applications that may be
>> relying on the "expected" incorrect behavior.
>>
>> Examples of the range of behavior, (all examples violate the pattern syntax
>> defined in the class description)
>>
>>
>> // no limit -> throws an expected IllegalArgumentException
>> var a = new ChoiceFormat("#foo");
>> // no limit or relation in the last subPattern -> discards the incorrect
>> portion, 'baz' and continues
>> var b = new ChoiceFormat("0#foo|1#bar|baz");
>> b.format(2); // returns 'bar'
>> // no relation or limit -> discards the incorrect portion, 'foo' and
>> continues
>> var c = new ChoiceFormat("foo");
>> c.format(1); // throws AIOOBE
>
> Justin Lu has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - include ascending order stipulation in constuctor as well
> - include apiSpec wording, move to patterns section
LGTM
src/java.base/share/classes/java/text/ChoiceFormat.java line 406:
> 404: * based on the pattern. The syntax and error related caveats for the
> 405: * ChoiceFormat pattern can be found in the {@linkplain ##patterns
> Patterns}
> 406: * section. Unlike {@link #ChoiceFormat(double[], String[])} this
> method will
Nit: `constructor` better suited than `method`.
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17856#pullrequestreview-1910070004
PR Review Comment: https://git.openjdk.org/jdk/pull/17856#discussion_r1508322149
Withdrawn: 6356745: (coll) Add PriorityQueue(Collection, Comparator)
On Mon, 11 Dec 2023 01:28:29 GMT, Valeh Hajiyev wrote: > This commit addresses the current limitation in the `PriorityQueue` > implementation, which lacks a constructor to efficiently create a priority > queue with a custom comparator and an existing collection. In order to create > such a queue, we currently need to initialize a new queue with custom > comparator, and after that populate the queue using `addAll()` method, which > in the background calls `add()` method (which takes `O(logn)` time) for each > element of the collection (`n` times). This is resulting in an overall time > complexity of `O(nlogn)`. > > > PriorityQueue pq = new PriorityQueue<>(customComparator); > pq.addAll(existingCollection); > > > The pull request introduces a new constructor to streamline this process and > reduce the time complexity to `O(n)`. If you create the queue above using > the new constructor, the contents of the collection will be copied (which > takes `O(n)` time) and then later `heapify()` operation (Floyd's algorithm) > will be called once (another `O(n)` time). Overall the operation will be > reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time. > > > PriorityQueue pq = new PriorityQueue<>(existingCollection, > customComparator); This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/17045
Re: RFR: JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns [v6]
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8317756)
> which defines the behavior for creating ChoiceFormats with incorrect
> patterns. The wording is added to both the ChoiceFormat constructor and
> ChoiceFormat::applyPattern method.
>
> While ideally the inconsistent behavior itself could be fixed, this behavior
> has been long-standing for 20+ years and the benefit of consistent error
> handling does not outweigh the risk of breaking applications that may be
> relying on the "expected" incorrect behavior.
>
> Examples of the range of behavior, (all examples violate the pattern syntax
> defined in the class description)
>
>
> // no limit -> throws an expected IllegalArgumentException
> var a = new ChoiceFormat("#foo");
> // no limit or relation in the last subPattern -> discards the incorrect
> portion, 'baz' and continues
> var b = new ChoiceFormat("0#foo|1#bar|baz");
> b.format(2); // returns 'bar'
> // no relation or limit -> discards the incorrect portion, 'foo' and continues
> var c = new ChoiceFormat("foo");
> c.format(1); // throws AIOOBE
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
wording: punctuation and use 'constructor'
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17856/files
- new: https://git.openjdk.org/jdk/pull/17856/files/8e5bbe05..aa1071e2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17856&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17856&range=04-05
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/17856.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17856/head:pull/17856
PR: https://git.openjdk.org/jdk/pull/17856
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Fri, 1 Mar 2024 01:50:46 GMT, Vladimir Petko wrote:
> This MR fixes segsegv in jspawnhelper when it is called without args.
> This scenario happens when a long running Java process is not restarted
> during upgrade.
>
> It updates test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java to
> check that jspawnhelper exits with code 1:
>
> After test update:
>
> $ make CONF=linux-x86_64-server-fastdebug test
> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
> ...
> Running jspawnhelper without args
> STDERR:
> java.lang.Exception: Parent process exited with 12
> at
> JspawnhelperProtocol.simulateJspawnhelperWithoutArgs(JspawnhelperProtocol.java:126)
> at JspawnhelperProtocol.main(JspawnhelperProtocol.java:267)
> at
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
> at
> com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
> at java.base/java.lang.Thread.run(Thread.java:1575)
> ...
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>>> 1 0 1 0 <<
> ==
> TEST FAILURE
>
> After jspawnhelper change the test passes:
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
> 1 1 0 0
>
> ==
> TEST SUCCESS
>
>
> The user will see the following output in the logs:
>
> An earlier version of Java is trying to call jspawnhelper.
> Please restart Java process.
> Exception in thread "main" java.io.IOException: Cannot run program "ls":
> error=0, Failed to exec spawn helper: pid: 2168121, exit value: 1
> at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143)
> at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073)
> at Test.main(Test.java:3)
> Caused by: java.io.IOException: error=0, Failed to exec spawn helper: pid:
> 2168121, exit value: 1
> at java.base/java.lang.ProcessImpl.forkAndExec(Native Method)
> at java.base/java.lang.ProcessImpl.(ProcessImpl.java:314)
> at java.base/java.lang.ProcessIm...
Manual test:
public class Test {
public static void main(String[] args) throws Throwable {
Process p = new ProcessBuilder("ls", "-alrt", "/tmp").start();
p.waitFor();
}
}
Running older java with a new jspawnhelper will result in the following output:
java --version
openjdk 17.0.9-internal 2023-10-17
OpenJDK Runtime Environment (build 17.0.9-internal+0-adhoc.vladimirp.jdk17u)
OpenJDK 64-Bit Server VM (build 17.0.9-internal+0-adhoc.vladimirp.jdk17u, mixed
mode)
$ ./java -cp . Test
An earlier version of Java is trying to call jspawnhelper.
Please restart Java process.
Exception in thread "main" java.io.IOException: Cannot run program "ls":
error=0, Failed to exec spawn helper: pid: 2168121, exit value: 1
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143)
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073)
at Test.main(Test.java:3)
Caused by: java.io.IOException: error=0, Failed to exec spawn helper: pid:
2168121, exit value: 1
at java.base/java.lang.ProcessImpl.forkAndExec(Native Method)
at java.base/java.lang.ProcessImpl.(ProcessImpl.java:314)
at java.base/java.lang.ProcessImpl.start(ProcessImpl.java:244)
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1110)
-
PR Comment: https://git.openjdk.org/jdk/pull/18074#issuecomment-1972308222
RFR: 8325567: jspawnhelper without args fails with segfault
This MR fixes segsegv in jspawnhelper when it is called without args. This scenario happens when a long running Java process is not restarted during upgrade. It updates test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java to check that jspawnhelper exits with code 1: After test update: $ make CONF=linux-x86_64-server-fastdebug test TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java ... Running jspawnhelper without args STDERR: java.lang.Exception: Parent process exited with 12 at JspawnhelperProtocol.simulateJspawnhelperWithoutArgs(JspawnhelperProtocol.java:126) at JspawnhelperProtocol.main(JspawnhelperProtocol.java:267) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138) at java.base/java.lang.Thread.run(Thread.java:1575) ... == Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java >> 1 0 1 0 << == TEST FAILURE After jspawnhelper change the test passes: == Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java 1 1 0 0 == TEST SUCCESS The user will see the following output in the logs: An earlier version of Java is trying to call jspawnhelper. Please restart Java process. Exception in thread "main" java.io.IOException: Cannot run program "ls": error=0, Failed to exec spawn helper: pid: 2168121, exit value: 1 at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143) at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073) at Test.main(Test.java:3) Caused by: java.io.IOException: error=0, Failed to exec spawn helper: pid: 2168121, exit value: 1 at java.base/java.lang.ProcessImpl.forkAndExec(Native Method) at java.base/java.lang.ProcessImpl.(ProcessImpl.java:314) at java.base/java.lang.ProcessImpl.start(ProcessImpl.java:244) at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1110) - Commit messages: - update copyright year - handle no-args call in jspawnhelper - update JspawnhelperProtocol.java to test jspawnhelper call without args Changes: https://git.openjdk.org/jdk/pull/18074/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18074&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325567 Stats: 46 lines in 2 files changed: 44 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18074.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18074/head:pull/18074 PR: https://git.openjdk.org/jdk/pull/18074
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries
On Wed, 28 Feb 2024 19:29:13 GMT, Erik Joelsson wrote: > Executables and dynamic libraries on Linux can encode a search path that the > dynamic linker will use when looking up library dependencies, generally > referred to as an "rpath". In the JDK we use this with the $ORIGIN feature to > set search paths relative to the location of the binary itself. Typically > executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find > libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to > find each other. > > There are two different types of such rpaths, RPATH and RUNPATH. The former > is the earlier incantation but RUNPATH has been around since about 2003 and > has been default in prominent Linux distros for a long time, and now also > seems to be default in the linker directly from binutils. The toolchain used > by Oracle defaulted to RPATH until at least JDK 11, but since then with some > toolchain upgrade, the default was flipped to RUNPATH. > > The main (relevant in this case) difference between the two is that RPATH is > considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is > only considered after LD_LIBRARY_PATH. For libraries that are part of a Linux > distribution, letting users, or the system, control and override builtin > rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. However, > for the JDK, there really is no usecase for having an externally configured > LD_LIBRARY_PATH potentially getting in the way of the JDK libraries finding > each other correctly. If a user environment sets LD_LIBRARY_PATH, and there > is a library in that path with the same name as a JDK library (e.g. libnet.so > or some other generically named library) that external library will be loaded > instead of the JDK internal library and that is basically guaranteed to break > the JDK. There is no supported usecase that I can think of for injecting > other versions of such libraries in a JDK distribution. > > I propose that we explicitly configure the JDK build to set RPATH instead of > RUNPATH for Linux binaries. This is done with the linker flag > "--disable-new-dtags". test/jdk/tools/launcher/RunpathTest.java line 27: > 25: * @test > 26: * @bug 7190813 8022719 > 27: * @summary Check for extended RPATHs on Linux Pre-existing but really the restriction to Linux should be via `@requires` not a runtime test. - PR Review Comment: https://git.openjdk.org/jdk/pull/18050#discussion_r1508535229
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Fri, 1 Mar 2024 01:50:46 GMT, Vladimir Petko wrote: > This MR fixes segsegv in jspawnhelper when it is called without args. > This scenario happens when a long running Java process is not restarted > during upgrade. > > It updates test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java to > check that jspawnhelper exits with code 1: > > After test update: > > $ make CONF=linux-x86_64-server-fastdebug test > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > ... > Running jspawnhelper without args > STDERR: > java.lang.Exception: Parent process exited with 12 > at > JspawnhelperProtocol.simulateJspawnhelperWithoutArgs(JspawnhelperProtocol.java:126) > at JspawnhelperProtocol.main(JspawnhelperProtocol.java:267) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) > at java.base/java.lang.reflect.Method.invoke(Method.java:580) > at > com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138) > at java.base/java.lang.Thread.run(Thread.java:1575) > ... > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java >>> 1 0 1 0 << > == > TEST FAILURE > > After jspawnhelper change the test passes: > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > > == > TEST SUCCESS > > > The user will see the following output in the logs: > > An earlier version of Java is trying to call jspawnhelper. > Please restart Java process. > Exception in thread "main" java.io.IOException: Cannot run program "ls": > error=0, Failed to exec spawn helper: pid: 2168121, exit value: 1 > at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143) > at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073) > at Test.main(Test.java:3) > Caused by: java.io.IOException: error=0, Failed to exec spawn helper: pid: > 2168121, exit value: 1 > at java.base/java.lang.ProcessImpl.forkAndExec(Native Method) > at java.base/java.lang.ProcessImpl.(ProcessImpl.java:314) > at java.base/java.lang.ProcessIm... Would it be possible to expand a bit on what is going on here? Are you saying that in the course of upgrading a JDK that you somehow get into a state where jspawnhelper has been replaced with a version from a newer JDK? Is the real issue here that the upgrade didn't shutdown the application and/or something copied over an existing installation during the upgrade? - PR Comment: https://git.openjdk.org/jdk/pull/18074#issuecomment-1972674199
