Re: RFR: 8327176: UnreferencedExecutor.java can fail on libgraal with -Xcomp
On Sun, 3 Mar 2024 17:01:53 GMT, Doug Simon wrote:
> The `java/util/concurrent/Executors/UnreferencedExecutor.java` test can fail
> when run on libgraal and `-Xcomp` is specified. The problem is that libgraal
> in `-Xcomp` temporarily causes some extra memory pressure (probably due to
> [JDK-8310218](https://bugs.openjdk.org/browse/JDK-8310218)) which can cause
> recoverable OOMEs to occur (memory is recovered when the relevant libgraal
> compilations complete). It also seems related to async threads used for
> cleaning weak references when using G1 or ZGC as I cannot reproduce the
> failure under `-XX:+UseSerialGC`.
>
> Installing a global `Thread.UncaughtExceptionHandler` that ignores
> `OutOfMemoryError`s resolves the problem.
Changes requested by dholmes (Reviewer).
test/jdk/java/util/concurrent/Executors/UnreferencedExecutor.java line 58:
> 56: throw new RuntimeException(e);
> 57: }
> 58: }
I'm not clear exactly what you are trying to achieve with this - is it just to
not print the stacktrace when OOME occurs?
A UEH should not rethrow exceptions as they are effectively ignored as we are
already at the final level of handling exceptions (the VM will print a one line
warning if a UEH itself throws). If I understand what you want then I think the
following would be more correct:
// Ignore OOME but do default handling for any other uncaught exception.
public void uncaughtException(Thread t, Throwable e) {
if ( ! e instanceof OutOfMemoryError) {
System.err.print("Exception in thread "" + t.getName() + "" ");
e.printStackTrace(System.err);
}
}
This doesn't stop the thread from terminating of course - the OOME already
caused it to unwind its stack if we get this far.
-
PR Review: https://git.openjdk.org/jdk/pull/18098#pullrequestreview-1950797938
PR Review Comment: https://git.openjdk.org/jdk/pull/18098#discussion_r1533273137
Re: RFR: JDK-8323760 putIfAbsent documentation conflicts with itself [v3]
On Wed, 14 Feb 2024 20:46:17 GMT, John Hendrikx wrote: >> Update the documentation for `@return` tag of `putIfAbsent` to match the >> main description. `putIfAbsent` uses the same wording as `put` for its >> `@return` tag, but that is incorrect. `putIfAbsent` never returns the >> **previous** value, as the whole point of the method is not the replace the >> value if it was present. As such, if it returns a value, it is the >> **current** value, and in all other cases it will return `null`. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Use new suggestion and remove original clarification Sorry for the delay. The current wording looks fine. I've changed the summary of the bug report to "clarify specification of Map::putIfAbsent return value" which I think is a better description. @hjohn please change the PR title to match. I've taken the liberty of updating the CSR. While this is sort-of a borderline case for a CSR, it is a bit more than fixing a typo; it's rewording a bit of normative specification. The CSR doesn't need to say too much beyond documenting the change and the reason for it (which I've done, based on @hjohn's original draft). The CSR should be approved without difficulty. Please review it. (@pavelrappo please mark as reviewed if you get a chance, thanks.) - PR Comment: https://git.openjdk.org/jdk/pull/17438#issuecomment-2011242189
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v16]
On Thu, 21 Mar 2024 00:40:45 GMT, Shaojin Wen wrote: > Should we declare the BigDecimal(CharSequence,MathContext) method as public? > Scenarios like > [MysqlBinaryValueDecoder#decodeDecimal](https://github.com/mysql/mysql-connector-j/blob/release/8.x/src/main/protocol-impl/java/com/mysql/cj/protocol/a/MysqlBinaryValueDecoder.java#L275) > can be used to avoid a memory allocation. Good idea, but since this involves a change in the public APIs and needs extra care with API specification, you should do this in a dependent PR (target the pr/18177 branch of jdk repo) that focus specifically on the addition. A new API is out of scope for this current PR. - PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2011064004
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v16]
On Wed, 20 Mar 2024 22:56:38 GMT, Shaojin Wen wrote:
>> The current BigDecimal(String) constructor calls String#toCharArray, which
>> has a memory allocation.
>>
>>
>> public BigDecimal(String val) {
>> this(val.toCharArray(), 0, val.length()); // allocate char[]
>> }
>>
>>
>> When the length is greater than 18, create a char[]
>>
>>
>> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
>> if (!isCompact) {
>> // ...
>> } else {
>> char[] coeff = new char[len]; // allocate char[]
>> // ...
>> }
>>
>>
>> This PR eliminates the two memory allocations mentioned above, resulting in
>> an approximate 60% increase in performance..
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use while instead for
Should we declare the BigDecimal(CharSequence,MathContext) method as public?
Scenarios like
[MysqlBinaryValueDecoder#decodeDecimal](https://github.com/mysql/mysql-connector-j/blob/release/8.x/src/main/protocol-impl/java/com/mysql/cj/protocol/a/MysqlBinaryValueDecoder.java#L275)
can be used to avoid a memory allocation.
-
PR Comment: https://git.openjdk.org/jdk/pull/18177#issuecomment-2010977844
Re: RFR: 8328316: Finisher cannot emit if stream is sequential and integrator returned false [v4]
> Adds differentiation between direct and transitive short circuiting which > could prevent pushing downstream in the finisher for built-ins that were not > `collect()`. > > Creating this as a draft PR for now, just need to run some benchmarks to > validate no significant regressions first. Viktor Klang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Updating copyright year and switching to use underscores in GathererShortCircuitTest - Adds differentiation between direct and transitive short circutiting for Gatherers - Changes: https://git.openjdk.org/jdk/pull/18351/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18351&range=03 Stats: 71 lines in 2 files changed: 66 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/18351.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18351/head:pull/18351 PR: https://git.openjdk.org/jdk/pull/18351
Re: RFR: 8328316: Finisher cannot emit if stream is sequential and integrator returned false [v3]
> Adds differentiation between direct and transitive short circuiting which > could prevent pushing downstream in the finisher for built-ins that were not > `collect()`. > > Creating this as a draft PR for now, just need to run some benchmarks to > validate no significant regressions first. Viktor Klang has updated the pull request incrementally with 236 additional commits since the last revision: - Adding a comma - Updating copyright year on GathererOp - 8289822: G1: Make concurrent mark code owner of TAMSes Reviewed-by: ayang, iwalulya - 8325362: Allow to create a simple in-memory input JavaFileObject Reviewed-by: jlaskey, darcy - 8326941: Remove StringUTF16::isBigEndian Reviewed-by: rriggs - 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is Reviewed-by: dholmes, alanb - 8308660: C2 compilation hits 'node must be dead' assert Reviewed-by: chagedorn, kvn - 8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed Reviewed-by: chagedorn, epeter - 8328275: CodeCache::print_internals should not be called in PRODUCT code Reviewed-by: ihse, jwaters, dholmes - 8326964: Remove Eclipse Shared Workspaces Reviewed-by: erikj, ihse - ... and 226 more: https://git.openjdk.org/jdk/compare/f32b1c5f...189ffaa3 - Changes: - all: https://git.openjdk.org/jdk/pull/18351/files - new: https://git.openjdk.org/jdk/pull/18351/files/f32b1c5f..189ffaa3 Webrevs: - full: Webrev is not available because diff is too large - incr: Webrev is not available because diff is too large Stats: 384826 lines in 1503 files changed: 18739 ins; 81305 del; 284782 mod Patch: https://git.openjdk.org/jdk/pull/18351.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18351/head:pull/18351 PR: https://git.openjdk.org/jdk/pull/18351
Re: RFR: 8328316: Finisher cannot emit if stream is sequential and integrator returned false [v2]
> Adds differentiation between direct and transitive short circuiting which > could prevent pushing downstream in the finisher for built-ins that were not > `collect()`. > > Creating this as a draft PR for now, just need to run some benchmarks to > validate no significant regressions first. Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/java/util/stream/GathererShortCircuitTest.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/18351/files - new: https://git.openjdk.org/jdk/pull/18351/files/fe867a15..f32b1c5f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18351&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18351&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18351.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18351/head:pull/18351 PR: https://git.openjdk.org/jdk/pull/18351
Re: RFR: 8328316: Finisher cannot emit if stream is sequential and integrator returned false
On Mon, 18 Mar 2024 16:27:13 GMT, Viktor Klang wrote: > Adds differentiation between direct and transitive short circuiting which > could prevent pushing downstream in the finisher for built-ins that were not > `collect()`. > > Creating this as a draft PR for now, just need to run some benchmarks to > validate no significant regressions first. Looks good, just a minor suggestion. test/jdk/java/util/stream/GathererShortCircuitTest.java line 48: > 46: Gatherer.of( > 47: (unused, element, downstream) -> false, > 48: (unused, downstream) -> downstream.push(expected) Suggestion: (_, element, downstream) -> false, (_ downstream) -> downstream.push(expected) - Marked as reviewed by psandoz (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18351#pullrequestreview-1950412052 PR Review Comment: https://git.openjdk.org/jdk/pull/18351#discussion_r1533025318
Re: RFR: 8327994: Update code gen in CallGeneratorHelper [v2]
On Wed, 20 Mar 2024 17:49:51 GMT, Maurizio Cimadamore wrote: > No changes in libTestDowncallStack.c (not even minor ones) ? No, there was a 'missing' space between the prefix parameters and the actual parameters of the stack variants, and between the arguments passed when that callback was called in the upcall lib (So, white space only). libTestDowncall.c didn't have any changes. - PR Comment: https://git.openjdk.org/jdk/pull/18269#issuecomment-2010825750
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v16]
> The current BigDecimal(String) constructor calls String#toCharArray, which
> has a memory allocation.
>
>
> public BigDecimal(String val) {
> this(val.toCharArray(), 0, val.length()); // allocate char[]
> }
>
>
> When the length is greater than 18, create a char[]
>
>
> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
> if (!isCompact) {
> // ...
> } else {
> char[] coeff = new char[len]; // allocate char[]
> // ...
> }
>
>
> This PR eliminates the two memory allocations mentioned above, resulting in
> an approximate 60% increase in performance..
Shaojin Wen has updated the pull request incrementally with one additional
commit since the last revision:
use while instead for
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18177/files
- new: https://git.openjdk.org/jdk/pull/18177/files/3ec5eac6..17f0a736
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=15
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=14-15
Stats: 7 lines in 1 file changed: 4 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/18177.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18177/head:pull/18177
PR: https://git.openjdk.org/jdk/pull/18177
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v15]
> The current BigDecimal(String) constructor calls String#toCharArray, which
> has a memory allocation.
>
>
> public BigDecimal(String val) {
> this(val.toCharArray(), 0, val.length()); // allocate char[]
> }
>
>
> When the length is greater than 18, create a char[]
>
>
> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
> if (!isCompact) {
> // ...
> } else {
> char[] coeff = new char[len]; // allocate char[]
> // ...
> }
>
>
> This PR eliminates the two memory allocations mentioned above, resulting in
> an approximate 60% increase in performance..
Shaojin Wen has updated the pull request incrementally with one additional
commit since the last revision:
Update src/java.base/share/classes/java/math/BigDecimal.java
Co-authored-by: Claes Redestad
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18177/files
- new: https://git.openjdk.org/jdk/pull/18177/files/69be44db..3ec5eac6
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=14
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18177&range=13-14
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18177.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18177/head:pull/18177
PR: https://git.openjdk.org/jdk/pull/18177
Re: RFR: 8328316: Finisher cannot emit if stream is sequential and integrator returned false
On Mon, 18 Mar 2024 16:27:13 GMT, Viktor Klang wrote: > Adds differentiation between direct and transitive short circuiting which > could prevent pushing downstream in the finisher for built-ins that were not > `collect()`. > > Creating this as a draft PR for now, just need to run some benchmarks to > validate no significant regressions first. @PaulSandoz Would you mind reviewing this PR, Paul? - PR Comment: https://git.openjdk.org/jdk/pull/18351#issuecomment-2010809413
Re: CFV: New Core Libraries Group Member: Per-Ake Minborg
Vote: yes
Integrated: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs
On Wed, 17 Jan 2024 23:41:53 GMT, Weijun Wang wrote: > This code change adds an alternative implementation of user-based > authorization `Subject` APIs that doesn't depend on Security Manager APIs. > Depending on if the Security Manager is allowed, the methods store the > current subject differently. See the spec change in the `Subject.java` file > for details. When the Security Manager APIs are finally removed in a future > release, this new implementation will be only implementation for these > methods. > > One major change in the new implementation is that `Subject.getSubject` > always throws an `UnsupportedOperationException` since it has an > `AccessControlContext` argument but the current subject is no longer > associated with an `AccessControlContext` object. > > Now it's the time to migrate from the `getSubject` and `doAs` methods to > `current` and `callAs`. If the user application is simply calling > `getSubject(AccessController.getContext())`, then switching to `current()` > would work. If the `AccessControlContext` argument is retrieved from an > earlier `getContext()` call and the associated subject might be different > from that of the current `AccessControlContext`, then instead of storing the > previous `AccessControlContext` object and passing it into `getSubject` to > get the "previous" subject, the application should store the `current()` > return value directly. This pull request has now been integrated. Changeset: d32746ef Author:Weijun Wang URL: https://git.openjdk.org/jdk/commit/d32746ef4a0ce6fec558274244321991be141698 Stats: 622 lines in 17 files changed: 477 ins; 27 del; 118 mod 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs Reviewed-by: mullan - PR: https://git.openjdk.org/jdk/pull/17472
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v9]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer 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 14 additional commits since the last revision: - Review suggestions Aleksei - Merge branch 'master' into JDK-8325579 - Update module-info text - Merge branch 'master' into JDK-8325579 - Indentation - Merge branch 'master' into JDK-8325579 - Review feedback - Rename back to LdapSSLHandshakeFailureTest to ease reviewing - Merge branch 'master' into JDK-8325579 - Typo - ... and 4 more: https://git.openjdk.org/jdk/compare/7b45211e...8fdc039c - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/10271159..8fdc039c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=07-08 Stats: 292653 lines in 398 files changed: 5187 ins; 5644 del; 281822 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v8]
On Tue, 19 Mar 2024 16:09:18 GMT, Aleksei Efimov wrote:
>> Christoph Langer 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 12 additional
>> commits since the last revision:
>>
>> - Update module-info text
>> - Merge branch 'master' into JDK-8325579
>> - Indentation
>> - Merge branch 'master' into JDK-8325579
>> - Review feedback
>> - Rename back to LdapSSLHandshakeFailureTest to ease reviewing
>> - Merge branch 'master' into JDK-8325579
>> - Typo
>> - Merge branch 'master' into JDK-8325579
>> - Rename test and refine comment
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/ffdffc5b...10271159
>
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 164:
>
>> 162: if
>> (CustomSocketFactory.customSocket.closeMethodCalledCount() <= 0) {
>> 163: System.err.println("Custom Socket was not
>> closed.");
>> 164: System.exit(-1);
>
> Can we update test not to use `System.exit` and replace it with throwing `new
> RuntimeException`, something like:
> Suggestion:
>
> throw new RuntimeException("Custom Socket was not
> closed");
Done.
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 177:
>
>> 175: ctx.close();
>> 176: if (!checkSocketClosed(sock)) {
>> 177: System.exit(-1);
>
> Can be replaced with:
> Suggestion:
>
> throw new RuntimeException("Socket isn't closed");
I simplified this, removing checkSocketClosed() method
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 184:
>
>> 182: e.printStackTrace();
>> 183: System.exit(-1);
>> 184: }
>
> For simplification and `System.exit` removal purposes this catch block can be
> removed with addition of `throws Exception` clause to the `main` method.
> Suggestion:
>
> }
I chose to throw a new RuntimeException(e)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532883964
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532885041
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532884603
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v8]
On Tue, 19 Mar 2024 16:01:34 GMT, Aleksei Efimov wrote:
>> Christoph Langer 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 12 additional
>> commits since the last revision:
>>
>> - Update module-info text
>> - Merge branch 'master' into JDK-8325579
>> - Indentation
>> - Merge branch 'master' into JDK-8325579
>> - Review feedback
>> - Rename back to LdapSSLHandshakeFailureTest to ease reviewing
>> - Merge branch 'master' into JDK-8325579
>> - Typo
>> - Merge branch 'master' into JDK-8325579
>> - Rename test and refine comment
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/b172eb4c...10271159
>
> src/java.naming/share/classes/module-info.java line 42:
>
>> 40: * The value of this environment property specifies the fully
>> 41: * qualified class name of the socket factory used by the LDAP
>> provider.
>> 42: * This class must implement the javax.net.SocketFactory
>> abstract class
>
> You could add a link to the `SocketFactory` class here:
> Suggestion:
>
> * This class must implement the {@link javax.net.SocketFactory}
> abstract class
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532877183
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v14]
On Thu, 14 Mar 2024 00:00:53 GMT, Shaojin Wen wrote:
>> The current BigDecimal(String) constructor calls String#toCharArray, which
>> has a memory allocation.
>>
>>
>> public BigDecimal(String val) {
>> this(val.toCharArray(), 0, val.length()); // allocate char[]
>> }
>>
>>
>> When the length is greater than 18, create a char[]
>>
>>
>> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
>> if (!isCompact) {
>> // ...
>> } else {
>> char[] coeff = new char[len]; // allocate char[]
>> // ...
>> }
>>
>>
>> This PR eliminates the two memory allocations mentioned above, resulting in
>> an approximate 60% increase in performance..
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> bug fix for CharArraySequence#charAt
src/java.base/share/classes/java/math/BigDecimal.java line 598:
> 596: // First compact case, we need not to preserve the
> character
> 597: // and we can just compute the value in place.
> 598: for (; ; c = val.charAt(++offset)) {
This needs to be simplified. Let's at least do this, which is more honest:
while (true) {
...
c = val.charAt(++offset);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18177#discussion_r1532864646
Re: RFR: 8327791: Optimization for new BigDecimal(String) [v14]
On Thu, 14 Mar 2024 00:00:53 GMT, Shaojin Wen wrote:
>> The current BigDecimal(String) constructor calls String#toCharArray, which
>> has a memory allocation.
>>
>>
>> public BigDecimal(String val) {
>> this(val.toCharArray(), 0, val.length()); // allocate char[]
>> }
>>
>>
>> When the length is greater than 18, create a char[]
>>
>>
>> boolean isCompact = (len <= MAX_COMPACT_DIGITS); // 18
>> if (!isCompact) {
>> // ...
>> } else {
>> char[] coeff = new char[len]; // allocate char[]
>> // ...
>> }
>>
>>
>> This PR eliminates the two memory allocations mentioned above, resulting in
>> an approximate 60% increase in performance..
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> bug fix for CharArraySequence#charAt
src/java.base/share/classes/java/math/BigDecimal.java line 576:
> 574: long rs = 0; // the compact value in long
> 575: BigInteger rb = null; // the inflated value in BigInteger
> 576: // use String bounds checking to handle too-long, len == 0,
Suggestion:
// use CharSequence bounds checking to handle too-long, len == 0,
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18177#discussion_r1532860171
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v8]
On Wed, 20 Mar 2024 14:45:50 GMT, Weijun Wang wrote: >> This code change adds an alternative implementation of user-based >> authorization `Subject` APIs that doesn't depend on Security Manager APIs. >> Depending on if the Security Manager is allowed, the methods store the >> current subject differently. See the spec change in the `Subject.java` file >> for details. When the Security Manager APIs are finally removed in a future >> release, this new implementation will be only implementation for these >> methods. >> >> One major change in the new implementation is that `Subject.getSubject` >> always throws an `UnsupportedOperationException` since it has an >> `AccessControlContext` argument but the current subject is no longer >> associated with an `AccessControlContext` object. >> >> Now it's the time to migrate from the `getSubject` and `doAs` methods to >> `current` and `callAs`. If the user application is simply calling >> `getSubject(AccessController.getContext())`, then switching to `current()` >> would work. If the `AccessControlContext` argument is retrieved from an >> earlier `getContext()` call and the associated subject might be different >> from that of the current `AccessControlContext`, then instead of storing the >> previous `AccessControlContext` object and passing it into `getSubject` to >> get the "previous" subject, the application should store the `current()` >> return value directly. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > more allow and years Marked as reviewed by mullan (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17472#pullrequestreview-1950109209
RFR: 8328316: Finisher cannot emit if stream is sequential and integrator returned false
Adds differentiation between direct and transitive short circuiting which could prevent pushing downstream in the finisher for built-ins that were not `collect()`. Creating this as a draft PR for now, just need to run some benchmarks to validate no significant regressions first. - Commit messages: - Adds differentiation between direct and transitive short circutiting for Gatherers Changes: https://git.openjdk.org/jdk/pull/18351/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18351&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328316 Stats: 70 lines in 2 files changed: 66 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18351.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18351/head:pull/18351 PR: https://git.openjdk.org/jdk/pull/18351
Re: RFR: 8328316: Finisher cannot emit if stream is sequential and integrator returned false
On Mon, 18 Mar 2024 16:27:13 GMT, Viktor Klang wrote: > Adds differentiation between direct and transitive short circuiting which > could prevent pushing downstream in the finisher for built-ins that were not > `collect()`. > > Creating this as a draft PR for now, just need to run some benchmarks to > validate no significant regressions first. Just a note to self, consider if this should get backported to 22 or not. - PR Comment: https://git.openjdk.org/jdk/pull/18351#issuecomment-2004384537
Re: RFR: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
On Wed, 20 Mar 2024 17:15:49 GMT, Mandy Chung wrote:
>> This is a draft PR with a potential solution to 8328366 without regressing
>> 8327501.
>>
>> @DougLea To avoid regressions in the future, where would regression tests
>> for this ideally be located?
>
> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1143:
>
>> 1141: @SuppressWarnings("removal")
>> 1142: SecurityManager sm = System.getSecurityManager();
>> 1143: if (sm != null && isCommon)
>
> For common thread pool, if SM is not enabled, it will create
> `ForkJoinWorkerThread` that does not clear thread locals which is different
> than JDK 18 behavior. Is this intentional?
@mlchung Yeah, @DougLea perhaps want to weigh in on that. I wonder if this
should be addressed as this PR or if it should be handled separately?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18374#discussion_r1532781080
Re: CFV: New Core Libraries Group Member: Per-Ake Minborg
Vote: yes On 3/19/24 8:19 AM, Daniel Fuchs wrote: Hi, I hereby nominate Per-Ake Minborg (pminborg) [1] to Membership in the Core Libraries Group [4]. Per-Ake is an OpenJDK Reviewer, a committer in the Leyden and Panama projects, and a member of Oracle’s Java Core Libraries team. Per-Ake has been actively participating in the development of the JDK and Panama projects for several years, and is one of the co-author of the Implementation of Foreign Function and Memory API (Third Preview) [2]. His contributions include more than 80 commits in the JDK [3] Votes are due by 16:00 UTC on April 2, 2024. Only current Members of the Core Libraries Group [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [5]. best regards, -- daniel [1] https://openjdk.org/census#pminborg [2] https://github.com/openjdk/jdk/commit/cbccc4c8172797ea2f1b7c301d00add3f517546d [3] https://github.com/search?q=repo%3Aopenjdk%2Fjdk+author%3Aminborg&type=commits [4] https://openjdk.org/census#core-libs [5] https://openjdk.org/groups/#member-vote
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Wed, 20 Mar 2024 10:24:23 GMT, Severin Gehwolf wrote: > What we really want is some form of API to extend/patch an existing jimage > preserving everything else. Perhaps I should look into that. Would that be > worth doing? I think avoiding the plugin pipeline in creating a linkable image is simpler and more reliable design. I think it's worth understanding how big effort to support that. The reason why I proposed the alternative putting the diffs data as a side file under `lib` is an interim solution that would allow this work to continue while the API for extending jimage can be done as a follow up. > it seems almost too easy to change if it was a file outside the image. Only a few files in the `lib` directory such as libjvm.so or libjvm.dylib, src.zip and a few other files are intended for external use. All other files and directories under it must be treated are private implementation details and their names, format, and content are subject to change without notice. See JEP 220 for details. > Thirdly, it would go against where the hermetic java work is going (one > effort of this project is to try to include most of the conf/resource files > outside the jimage inside the jimage, next to the classes that use them). I agree that putting the diffs file in the jimage is a good direction. I am also ok with the interim solution. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2010349483
Re: RFR: JDK-8326853 Missing @since tags for Charset related methods added in Java 10 [v9]
> # Issue > - JDK-8326853 Incorrect `@since` Tags for Charset Related Methods Added in > JDK 10 > > I changed the `@since` tags to better accurately show when the methods and > constructors were introduced. Nizar Benalla has updated the pull request incrementally with two additional commits since the last revision: - Update full name - Update full name - Changes: - all: https://git.openjdk.org/jdk/pull/18032/files - new: https://git.openjdk.org/jdk/pull/18032/files/56968dcd..99f54fe2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18032&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18032&range=07-08 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18032.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18032/head:pull/18032 PR: https://git.openjdk.org/jdk/pull/18032
Re: RFR: 8327994: Update code gen in CallGeneratorHelper [v2]
On Sat, 16 Mar 2024 22:34:25 GMT, Jorn Vernee wrote: >> Update the code gen code in CallGeneratorHelper to reflect the latest state >> of the libTest(Downcall/Upcall)(Stack).c and shared.h files. >> >> - The previous code wanted users to pipe stdout into a file. But, since we >> have 5 files that need to be created, and the names of those files is >> important too, I've changed the script to write to those files directly >> instead. >> - I moved the definition of `S_` to libVarArgs.c where it's used, as >> it's not actually generated by CallGeneratorHelper. >> - GitHub doesn't render the changes to some of the files, but those files >> only contain either white space changes (some missing spaces after `,`), and >> copyright header updates. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Delete extra space > > Co-authored-by: Andrey Turbanov Nice cleanup. No changes in libTestDowncallStack.c (not even minor ones) ? - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18269#pullrequestreview-1949642772
Re: RFR: JDK-8326853 Missing @since tags for Charset related methods added in Java 10 [v8]
On Tue, 19 Mar 2024 17:02:36 GMT, Nizar Benalla wrote: >> # Issue >> - JDK-8326853 Incorrect @\since Tags for Charset Related Methods Added in >> JDK 10 >> >> I changed the @\since tags to better accurately show when the methods and >> constructors were introduced. > > Nizar Benalla has updated the pull request incrementally with two additional > commits since the last revision: > > - update the copyright year to 2024 > - Revert "update the latter years for the Oracle copyrights" > >This reverts commit 8bcc364fef8287c4d9aff7c60ed6fc0ea9155f64. LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18032#pullrequestreview-1949627443
Re: RFR: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
On Tue, 19 Mar 2024 12:16:29 GMT, Viktor Klang wrote:
> This is a draft PR with a potential solution to 8328366 without regressing
> 8327501.
>
> @DougLea To avoid regressions in the future, where would regression tests for
> this ideally be located?
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1143:
> 1141: @SuppressWarnings("removal")
> 1142: SecurityManager sm = System.getSecurityManager();
> 1143: if (sm != null && isCommon)
For common thread pool, if SM is not enabled, it will create
`ForkJoinWorkerThread` that does not clear thread locals which is different
than JDK 18 behavior. Is this intentional?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18374#discussion_r1532497712
RFR: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
This is a draft PR with a potential solution to 8328366 without regressing 8327501. @DougLea To avoid regressions in the future, where would regression tests for this ideally be located? - Commit messages: - Adding test case to verify that it is possible to change the CCL of the common pool - Restore setContextClassLoader functionality without breaking JDK-8327501 Changes: https://git.openjdk.org/jdk/pull/18374/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18374&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328366 Stats: 14 lines in 2 files changed: 8 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18374.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18374/head:pull/18374 PR: https://git.openjdk.org/jdk/pull/18374
Re: RFR: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
On Tue, 19 Mar 2024 12:16:29 GMT, Viktor Klang wrote: > This is a draft PR with a potential solution to 8328366 without regressing > 8327501. > > @DougLea To avoid regressions in the future, where would regression tests for > this ideally be located? @DougLea @AlanBateman Added a regression test to ForkJoinPool9Test. I think this is the fix we need. Please review. - PR Comment: https://git.openjdk.org/jdk/pull/18374#issuecomment-2009993580
Integrated: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream
On Thu, 14 Dec 2023 20:15:39 GMT, Archie Cobbs wrote: > `GZIPInputStream`, when looking for a concatenated stream, relies on what the > underlying `InputStream` says is how many bytes are `available()`. But this > is inappropriate because `InputStream.available()` is just an estimate and is > allowed (for example) to always return zero. > > The fix is to ignore what's `available()` and just proceed and see what > happens. If fewer bytes are available than required, the attempt to extend to > another stream is canceled just as it was before, e.g., when the next stream > header couldn't be read. This pull request has now been integrated. Changeset: d3f3011d Author:Archie Cobbs Committer: Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/d3f3011d56267360d65841da3550eca79cf1575b Stats: 111 lines in 2 files changed: 96 ins; 9 del; 6 mod 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/17113
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v8]
On Wed, 20 Mar 2024 14:45:50 GMT, Weijun Wang wrote: >> This code change adds an alternative implementation of user-based >> authorization `Subject` APIs that doesn't depend on Security Manager APIs. >> Depending on if the Security Manager is allowed, the methods store the >> current subject differently. See the spec change in the `Subject.java` file >> for details. When the Security Manager APIs are finally removed in a future >> release, this new implementation will be only implementation for these >> methods. >> >> One major change in the new implementation is that `Subject.getSubject` >> always throws an `UnsupportedOperationException` since it has an >> `AccessControlContext` argument but the current subject is no longer >> associated with an `AccessControlContext` object. >> >> Now it's the time to migrate from the `getSubject` and `doAs` methods to >> `current` and `callAs`. If the user application is simply calling >> `getSubject(AccessController.getContext())`, then switching to `current()` >> would work. If the `AccessControlContext` argument is retrieved from an >> earlier `getContext()` call and the associated subject might be different >> from that of the current `AccessControlContext`, then instead of storing the >> previous `AccessControlContext` object and passing it into `getSubject` to >> get the "previous" subject, the application should store the `current()` >> return value directly. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > more allow and years There is no source code change to `java.management` anymore inside this PR. They will be resolved with new bugs at [JDK-8327618](https://bugs.openjdk.org/browse/JDK-8327618) and [JDK-8328263](https://bugs.openjdk.org/browse/JDK-8328263). There are test changes in these areas in this PR to force them running with SM allowed. Ideally, these changes can be reverted when the 2 new bugs are resolved. - PR Comment: https://git.openjdk.org/jdk/pull/17472#issuecomment-2009779837
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v8]
> This code change adds an alternative implementation of user-based > authorization `Subject` APIs that doesn't depend on Security Manager APIs. > Depending on if the Security Manager is allowed, the methods store the > current subject differently. See the spec change in the `Subject.java` file > for details. When the Security Manager APIs are finally removed in a future > release, this new implementation will be only implementation for these > methods. > > One major change in the new implementation is that `Subject.getSubject` > always throws an `UnsupportedOperationException` since it has an > `AccessControlContext` argument but the current subject is no longer > associated with an `AccessControlContext` object. > > Now it's the time to migrate from the `getSubject` and `doAs` methods to > `current` and `callAs`. If the user application is simply calling > `getSubject(AccessController.getContext())`, then switching to `current()` > would work. If the `AccessControlContext` argument is retrieved from an > earlier `getContext()` call and the associated subject might be different > from that of the current `AccessControlContext`, then instead of storing the > previous `AccessControlContext` object and passing it into `getSubject` to > get the "previous" subject, the application should store the `current()` > return value directly. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: more allow and years - Changes: - all: https://git.openjdk.org/jdk/pull/17472/files - new: https://git.openjdk.org/jdk/pull/17472/files/dfa22af0..1e6a7982 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17472&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17472&range=06-07 Stats: 9 lines in 6 files changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17472.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17472/head:pull/17472 PR: https://git.openjdk.org/jdk/pull/17472
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v9]
On Sun, 17 Mar 2024 22:53:55 GMT, Archie Cobbs wrote: >> `GZIPInputStream`, when looking for a concatenated stream, relies on what >> the underlying `InputStream` says is how many bytes are `available()`. But >> this is inappropriate because `InputStream.available()` is just an estimate >> and is allowed (for example) to always return zero. >> >> The fix is to ignore what's `available()` and just proceed and see what >> happens. If fewer bytes are available than required, the attempt to extend >> to another stream is canceled just as it was before, e.g., when the next >> stream header couldn't be read. > > Archie Cobbs 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 10 additional > commits since the last revision: > > - Merge branch 'master' into JDK-7036144 > - Back-out Javadoc addition; to be added in a separate issue. > - Document the handling of concatenated streams. > - Merge branch 'master' into JDK-7036144 > - Merge branch 'master' into JDK-7036144 > - Merge branch 'master' into JDK-7036144 > - Address third round of review comments. > - Address second round of review comments. > - Address review comments. > - Fix bug in GZIPInputStream when underlying available() returns short. Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17113#pullrequestreview-1949062346
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v9]
On Sun, 17 Mar 2024 22:53:55 GMT, Archie Cobbs wrote: >> `GZIPInputStream`, when looking for a concatenated stream, relies on what >> the underlying `InputStream` says is how many bytes are `available()`. But >> this is inappropriate because `InputStream.available()` is just an estimate >> and is allowed (for example) to always return zero. >> >> The fix is to ignore what's `available()` and just proceed and see what >> happens. If fewer bytes are available than required, the attempt to extend >> to another stream is canceled just as it was before, e.g., when the next >> stream header couldn't be read. > > Archie Cobbs 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 10 additional > commits since the last revision: > > - Merge branch 'master' into JDK-7036144 > - Back-out Javadoc addition; to be added in a separate issue. > - Document the handling of concatenated streams. > - Merge branch 'master' into JDK-7036144 > - Merge branch 'master' into JDK-7036144 > - Merge branch 'master' into JDK-7036144 > - Address third round of review comments. > - Address second round of review comments. > - Address review comments. > - Fix bug in GZIPInputStream when underlying available() returns short. Hello Archie, tier1, tier2, tier3 completed without any related failures. I also ran JCK tests related to this class and those too passed. I've also taken Lance's inputs on this PR and he agrees that this look OK. I'll go ahead and approve this now. Thank you for fixing this, as well as your patience during the review. - PR Comment: https://git.openjdk.org/jdk/pull/17113#issuecomment-2009699729
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]
On Tue, 19 Mar 2024 17:58:46 GMT, Bill Huang wrote: >> This task addresses an essential aspect of our testing infrastructure: the >> proper handling and cleanup of temporary files and socket files created >> during test execution. The motivation behind these changes is to prevent the >> accumulation of unnecessary files in the default temporary directory, which >> can affect the system's storage and potentially influence subsequent test >> runs. >> >> Our review identified that several tests create temporary files or socket >> files without ensuring their removal post-execution. >> - Direct calls to java.io.File.createTempFile and >> java.nio.file.Files.createTempFile without adequate cleanup. >> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not >> explicitly removing socket files post-use. > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Implemented review comments Speaking for JFR, we should probably just create a normal file, possibly with a filename to indicate subtest and iteration. That said, test changes for JFR looks fine, and fixing the filename is outside the scope of this bug. - PR Comment: https://git.openjdk.org/jdk/pull/18352#issuecomment-2009218768
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 19 Mar 2024 18:05:31 GMT, Mandy Chung wrote: > Thanks for the details. I feel the pain in extending jlink for this work as > the current jlink implementation is not easily understandable and has been > yearning for rewrite in my perspective (looking forward to Project Leyden's > condenser model). +1 > Really appreciate your effort for this. Sure. > IIUC, the main issue is regarding extending an jimage with additional entries. Yes, it's really extending the jimage with additional entries which isn't supported currently (outside the jlink + archive + plugin pipeline). > The other option is to add the diffs as a separate file in the JDK image > under `lib` instead of part of the jimage. Sure that would work. On the other hand, it's another additional artefact that downstream distributors need to account for. What's more, it seems almost too easy to change if it was a file outside the image. Thirdly, it would go against where the hermetic java work is going (one effort of this project is to try to include most of the conf/resource files outside the jimage inside the jimage, next to the classes that use them). > The per-module mapping metadata files can be generated as part of the normal > linking as the footprint is small. Sure. > Would that simplify it? I'm not sure. What we really want is some form of API to extend/patch an existing jimage preserving everything else. Perhaps I should look into that. Would that be worth doing? > I think the other issues may be solvable. It's not a question of being solvable. All the issues are solvable in some way. Question is how exactly we want to address them. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2009217508
Integrated: 8328524: [x86] StringRepeat.java failure on linux-x86: Could not reserve enough space for 2097152KB object heap
On Tue, 19 Mar 2024 14:59:19 GMT, Goetz Lindenmaier wrote: > …rve enough space for 2097152KB object heap > > I would like to fix this as the two related issues mentioned in the JBS bug. > We see it currently in most GHA runs. This pull request has now been integrated. Changeset: eebcc218 Author:Goetz Lindenmaier URL: https://git.openjdk.org/jdk/commit/eebcc2181fe27f6aa10559233c7c58882a146f56 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8328524: [x86] StringRepeat.java failure on linux-x86: Could not reserve enough space for 2097152KB object heap Reviewed-by: rriggs, mdoerr, sgehwolf - PR: https://git.openjdk.org/jdk/pull/18380
Re: RFR: 8328524: [x86] StringRepeat.java failure on linux-x86: Could not reserve enough space for 2097152KB object heap
On Tue, 19 Mar 2024 14:59:19 GMT, Goetz Lindenmaier wrote: > …rve enough space for 2097152KB object heap > > I would like to fix this as the two related issues mentioned in the JBS bug. > We see it currently in most GHA runs. GHA failure: known Risc-V build problem. Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/18380#issuecomment-2008933994 PR Comment: https://git.openjdk.org/jdk/pull/18380#issuecomment-2008934348
