Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 6 Jun 2024 21:44:44 GMT, Scott Gibbons wrote: >> @asgibbons I generally just stop pushing ANY RFE's a week or two before the >> fork. Even if you did run the fuzzer with it - there are often last-minute >> changes. And your code here is rather large, so even if you are confident, >> there must be at least one bug hiding. >> >> Running the fuzzer is nice as pre-integration, but it mostly only catches >> things post-integration. > > @eme64 Are you OK with me integrating? @asgibbons yes, ship it! Thanks for waiting! - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2154015592
Re: RFR: 8333599: Improve description of \b matcher in j.u.r.Pattern
On Thu, 6 Jun 2024 18:16:14 GMT, Raffaello Giulietti wrote: > A documentation-only change to match the original intent and the implemented > behavior. Yes, this one needs a CSR. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19583#pullrequestreview-2103635744
RFR: 8333768: Minor doc updates to java.lang.{Float, Double}
Misc small doc updates and addition of `@Overrides` annotations.
-
Commit messages:
- JDK-8333768: Minor doc updates to java.lang.{Float, Double}
Changes: https://git.openjdk.org/jdk/pull/19590/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19590=00
Issue: https://bugs.openjdk.org/browse/JDK-8333768
Stats: 55 lines in 2 files changed: 40 ins; 2 del; 13 mod
Patch: https://git.openjdk.org/jdk/pull/19590.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19590/head:pull/19590
PR: https://git.openjdk.org/jdk/pull/19590
Re: [External] : Status of project "Brisbane"?
Thanks for asking, Volker. At this stage we're still bootstrapping the project and going through internal processes to publish its OpenJDK repositories with code. We’re also juggling other projects. I can let you know once the mailing list and repository are available. In the meantime, feel free to ping me again if you don't hear anything in the next couple of months. Best regards, Denis From: Volker Simonis Date: Tuesday, 4 June 2024 at 1:03 AM To: core-libs-dev@openjdk.org , security-dev , Denis Gauthier Subject: [External] : Status of project "Brisbane"? Hi, What's the status of Project Brisbane? According to [1], the Project was approved two month ago on April 4th, but until now I can't find it listed on openjdk.org nor can I find a corresponding mailing list? Best regards, Volker [1] https://mail.openjdk.org/pipermail/announce/2024-April/000350.html
Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v3]
On Thu, 6 Jun 2024 14:02:03 GMT, Jaikiran Pai wrote: >> Can I please get a review of this doc-only change which proposes to improve >> the code snippet that's in `java.util.zip.Deflater` and >> `java.util.zip.Inflater` to better explain the usage of those classes? This >> addresses https://bugs.openjdk.org/browse/JDK-8026127. >> >> The commit in the PR cleans up the snippet to correctly compress/decompress >> till the `Deflater` and `Inflater` are `finished()`. Additionally, the >> snippet also shows that the `Deflater` and `Inflater` are expected to be >> closed when their usage it done, to release the resources held by those >> instances. >> >> I've run `make docs-image` locally to verify that the generated snippet >> content as well as the link from `Inflater` work fine in the rendered >> javadoc HTML. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - minor change to the comment > - move the snippet to an external snippet Thank you Lance for the review. - PR Comment: https://git.openjdk.org/jdk/pull/19507#issuecomment-2153639493
Integrated: 8026127: Deflater/Inflater documentation incomplete/misleading
On Sat, 1 Jun 2024 04:33:57 GMT, Jaikiran Pai wrote: > Can I please get a review of this doc-only change which proposes to improve > the code snippet that's in `java.util.zip.Deflater` and > `java.util.zip.Inflater` to better explain the usage of those classes? This > addresses https://bugs.openjdk.org/browse/JDK-8026127. > > The commit in the PR cleans up the snippet to correctly compress/decompress > till the `Deflater` and `Inflater` are `finished()`. Additionally, the > snippet also shows that the `Deflater` and `Inflater` are expected to be > closed when their usage it done, to release the resources held by those > instances. > > I've run `make docs-image` locally to verify that the generated snippet > content as well as the link from `Inflater` work fine in the rendered javadoc > HTML. This pull request has now been integrated. Changeset: d8af5894 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/d8af58941b5dedb9774c0971895c4924e57ac28b Stats: 155 lines in 3 files changed: 96 ins; 57 del; 2 mod 8026127: Deflater/Inflater documentation incomplete/misleading Reviewed-by: lancea - PR: https://git.openjdk.org/jdk/pull/19507
Re: RFR: 8333599: Improve description of \b matcher in j.u.r.Pattern
On Thu, 6 Jun 2024 18:16:14 GMT, Raffaello Giulietti wrote: > A documentation-only change to match the original intent and the implemented > behavior. Oh, this needs a CSR too, since it's a change to a normative assertion. Should be pretty simple though. - PR Comment: https://git.openjdk.org/jdk/pull/19583#issuecomment-2153628785
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v2]
On Thu, 6 Jun 2024 19:30:21 GMT, Jorn Vernee wrote:
>> Chen Liang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> mt -> md (desc)
>
> src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 76:
>
>> 74: * type and parameter types can be described nominally.
>> 75: */
>> 76: public static MethodTypeDesc methodDesc(MethodType type) {
>
> Please name these methods `methodTypeDesc`, since we also have the `Method`
> type.
Thanks for the suggestion. Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630306158
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v12]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Loosening the timeout value - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/c6476270..df5f8f91 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=11 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=10-11 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 30 May 2024 16:20:02 GMT, Emanuel Peter wrote: >> @vnkozlov OK. I'll defer to you all. I've contacted the author of the >> fuzzer to see what I can do to set up a local instance. Would this be >> sufficient to increase confidence for future submissions? We can run it >> perpetually on fixes (provided I can set it up). Had I done that, we could >> have had 6 months of fuzzing on top of our tests. Would that have >> alleviated this concern? > > @asgibbons I generally just stop pushing ANY RFE's a week or two before the > fork. Even if you did run the fuzzer with it - there are often last-minute > changes. And your code here is rather large, so even if you are confident, > there must be at least one bug hiding. > > Running the fuzzer is nice as pre-integration, but it mostly only catches > things post-integration. @eme64 Are you OK with me integrating? - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2153456076
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v3]
> In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. Chen Liang 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 five additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into cleanup/consolidate-todesc - Add referenceClassDesc, move ReferenceClassDescImpl.ofValidatedBinaryName to ConstantUtils.binaryNameToDesc - mt -> md (desc) - Missed license header - Consolidate class/mt to desc operations - Changes: - all: https://git.openjdk.org/jdk/pull/19585/files - new: https://git.openjdk.org/jdk/pull/19585/files/3e28306f..89bc5d8d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19585=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19585=01-02 Stats: 2350 lines in 73 files changed: 2189 ins; 26 del; 135 mod Patch: https://git.openjdk.org/jdk/pull/19585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19585/head:pull/19585 PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8332400: isspace argument should be a valid unsigned char
On Wed, 5 Jun 2024 20:08:10 GMT, Robert Toyonaga wrote:
> ### Summary
> This change ensures we don't get undefined behavior when
> calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html).
> `isspace` accepts an `int` argument that "the application shall ensure is a
> character representable as an unsigned char or equal to the value of the
> macro EOF.".
>
> Previously, there was no checking of the values passed to `isspace`. I've
> replaced direct calls with a new wrapper `os::is_space` that performs a range
> check and prevents the possibility of undefined behavior from happening. For
> instances outside of Hotspot, I've added casts to `unsigned char`.
>
> **Testing**
> - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check
> `os::is_space` is working correctly.
> - tier1
Sorry I think this is well-intentioned but unnecessary in nearly all cases. If
you pass a char there is no potential problem. Only passing an actual int could
be a problem.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 664:
> 662: char after_key = line[key_len];
> 663: if (strncmp(line, key, key_len) == 0
> 664: && os::is_space(after_key) != 0
I think this is excessive. The value is a char so cannot be a problem. The only
calls to isspace that need checking are those that actually pass an int, and
even then there could be adequate safeguards in place.
src/hotspot/os/linux/os_linux.cpp line 1356:
> 1354: if (s) {
> 1355: // Skip blank chars
> 1356: do { s++; } while (s && os::is_space(*s));
Again not needed.
-
PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2103234054
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630265925
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630266490
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
On Thu, 6 Jun 2024 17:08:50 GMT, Naoto Sato wrote: >> test/jdk/java/io/Console/restoreEcho.exp line 57: >> >>> 55: test "$rpprompt" "$rpinput" "-echo" "$rpexpected" >>> 56: # See if the initialEcho is restored with `stty -a` >>> 57: expect " $initialEcho " >> >> If you leave out those whitespace characters inside the quotes and >> `$initialEcho` expands to `-echo`, it will be treated as an option to >> `expect`, right? If so, consider this instead: >> >> expect -- $initialEcho >> >> But more importantly: could a test match `echo` in `-echo` and therefore >> falsely pass? > > The spaces before/after `$initialEcho` are exactly to distinguish "echo" from > "-echo", otherwise the test falsely succeeds as you pointed out. Although the > test works as expected as it is, adding `--` would be safer. It's clever. However, we now depend on `(-)echo` being in the middle of the line, no? If `stty -a` format (IEEE Std 1003.2) allows `(-)echo` to appear in an arbitrary position of a line, our check won't work. If `(-)echo` appears in a leading position, it might be preceded by a TAB (similarly to `-echoprt` below). If `(-)echo` appears in a trailing position, it is followed by newline (similarly to `echoctl` below). lflags: icanon isig iexten echo echoe -echok echoke -echonl echoctl$ ^I-echoprt -altwerase -noflsh -tostop -flusho pendin -nokerninfo$ ^I-extproc$ But I guess, it's good enough and certainly much better than the initially proposed unit test. - PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1630230512
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v52]
On Thu, 6 Jun 2024 17:41:20 GMT, Scott Gibbons wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix copyright & a couple of comment typos > > Hi, everyone. I see that JDK 23 has now been forked, and new commits go into > the JDK 24 branch. I would like to get this in as soon as possible to have > as much time with fuzzers, etc. for everyone to be confident in the code. > > I have 3 positive reviews on this PR and would like to integrate. Please > reply as soon as you reasonably can with objections or approval and I will > integrate. Thanks. @asgibbons, my testing almost finished. No new failures. I think this can be pushed now. Thank you for waiting! - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2153366787
Integrated: 8333456: CompactNumberFormat integer parsing fails when string has no suffix
On Mon, 3 Jun 2024 22:32:54 GMT, Justin Lu wrote:
> Please review this PR which handles incorrect CompactNumberFormat integer
> only parsing when there is no suffix.
>
> See the following snippet,
>
>
> var fmt = NumberFormat.getCompactNumberInstance(Locale.US,
> NumberFormat.Style.SHORT)
> fmt.setParseIntegerOnly(true)
> fmt.parse("5K") // returns 5000
> fmt.parse("50.00") // returns 50
> fmt.parse("5") // unexpectedly throws StringIndexOutOfBoundsException
> fmt.parse("5 ") // returns 5
>
>
> Within the `parse` method, there is code that advances `position` past the
> fractional portion to find the suffix when `parseIntegerOnly()` is true.
> However, if a valid string input is given with no suffix,
> `DecimalFormat.subparseNumber()` will fully iterate through the string and
> `position` will be equal to the length of the string, throwing
> StringIndexOutOfBoundsException when `charAt` is invoked (line 1740).
>
> We should check to make sure position does not exceed the string length
> before deciding to check for a decimal symbol.
This pull request has now been integrated.
Changeset: 6238bc8d
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/6238bc8da2abe7a1f0cdd98c0af01e9ba1869ec3
Stats: 26 lines in 2 files changed: 24 ins; 0 del; 2 mod
8333456: CompactNumberFormat integer parsing fails when string has no suffix
Reviewed-by: naoto
-
PR: https://git.openjdk.org/jdk/pull/19533
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
On Thu, 6 Jun 2024 17:51:50 GMT, Naoto Sato wrote: > Turned out that removing the classpath ends up not finding the test class: > > ``` > Error: Could not find or load main class RestoreEchoTest > Caused by: java.lang.ClassNotFoundException: RestoreEchoTest > ]; > ``` Hm... this is surprising. On my machine, `testSrc` expands to the `JTwork/classes/0/java/io/Console/RestoreEchoTest.d` directory, which is included in `CLASSPATH` and which contains `RestoreEchoTest.class`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1630200122
Re: RFR: 8333599: Improve description of \b matcher in j.u.r.Pattern
On Thu, 6 Jun 2024 18:16:14 GMT, Raffaello Giulietti wrote: > A documentation-only change to match the original intent and the implemented > behavior. Looks good. The old version with the lookahead and lookbehind was hard to understand and it was inaccurate as well. The descriptive text is easier to understand and more accurate. - Marked as reviewed by smarks (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19583#pullrequestreview-2103079803
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v2]
On Thu, 6 Jun 2024 19:09:36 GMT, Claes Redestad wrote: > There are some pre-existing places where we call > `ReferenceClassDescImpl.ofValidated` directly that could probably be switched > over to `ConstantUtils.classDesc` for slightly nicer code. Or - if it matters > - add a `referenceClassDesc` which avoids the `type.isPrimitive` test. I think the main advantage of `ofValidated` is that it avoids descriptor string computation from class objects and should be kept in performance-sensitive code. `referenceClassDesc` would be feasible for less performance-demanding code's initialization and known safe conversions (a few APIs in CF API expects non-primitive CDs already). But should its implementation be `ofValidated` or `ofValidatedBinaryName` (same question for ClassDesc)? We used `ofValidatedBinaryName` here instead of a `toClassDesc` (which calls `ofValidated` via `descriptorString`) - PR Comment: https://git.openjdk.org/jdk/pull/19585#issuecomment-2153281690
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v2]
On Thu, 6 Jun 2024 19:24:14 GMT, Chen Liang wrote:
>> In java.base, especially in bytecode generators, we have many different
>> methods converting known valid Class and MethodType into ClassDesc and
>> MethodTypeDesc. These conversions should be consolidated into the same
>> utility method for the ease of future maintenance.
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> mt -> md (desc)
src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 76:
> 74: * type and parameter types can be described nominally.
> 75: */
> 76: public static MethodTypeDesc methodDesc(MethodType type) {
Please name these methods `methodTypeDesc`, since we also have the `Method`
type.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630114413
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v2]
On Thu, 6 Jun 2024 19:01:02 GMT, Claes Redestad wrote: >> Chen Liang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> mt -> md (desc) > > src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java > line 471: > >> 469: case Allocate allocate -> >> emitAllocBuffer(allocate); >> 470: case BoxAddress boxAddress -> >> emitBoxAddress(boxAddress); >> 471: case SegmentBase _ -> emitSegmentBase(); > > Seem a bit too far detached from the changes at hand for a drive-by code > cleanup? My uncontrollable urges to fix these minor IDE warnings... Please allow these fixes, as I have similar changes in ProxyGenerator as well. - PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630100819
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v2]
> In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: mt -> md (desc) - Changes: - all: https://git.openjdk.org/jdk/pull/19585/files - new: https://git.openjdk.org/jdk/pull/19585/files/cb4f6d13..3e28306f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19585=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19585=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19585/head:pull/19585 PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base
On Thu, 6 Jun 2024 18:56:51 GMT, Claes Redestad wrote: >> In java.base, especially in bytecode generators, we have many different >> methods converting known valid Class and MethodType into ClassDesc and >> MethodTypeDesc. These conversions should be consolidated into the same >> utility method for the ease of future maintenance. > > src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 469: > >> 467: // o instanceof Wrapped(float) >> 468: cb.aload(SELECTOR_OBJ); >> 469: >> cb.instanceOf(classDesc(Wrapper.forBasicType(classLabel) > > I have a patch somewhere to cache the wrapper class desc in > `sun.invoke.util.Wrapper`, both as a micro-optimization and to help > disambigutate the unfortunately named (my bad) `Wrapper::classDescriptor` > method. Maybe we can roll that into this..? Feel free, or you can get your patch merged first and then I base off yours if yours is ready. - PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630091038
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base
On Thu, 6 Jun 2024 18:35:01 GMT, Chen Liang wrote: > In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. Looks good to me. Probably should get someone to OK changes in foreign/abi (@JornVernee perhaps?). There are some pre-existing places where we call `ReferenceClassDescImpl.ofValidated` directly that could probably be switched over to `ConstantUtils.classDesc` for slightly nicer code. Or - if it matters - add a `referenceClassDesc` which avoids the `type.isPrimitive` test. src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 256: > 254: // the field name holding the method handle for this method > 255: String fieldName = "m" + count++; > 256: var mt = methodDesc(m.getReturnType(), > JLRA.getExecutableSharedParameterTypes(m)); Suggestion: var md = methodDesc(m.getReturnType(), JLRA.getExecutableSharedParameterTypes(m)); src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 469: > 467: // o instanceof Wrapped(float) > 468: cb.aload(SELECTOR_OBJ); > 469: > cb.instanceOf(classDesc(Wrapper.forBasicType(classLabel) I have a patch somewhere to cache the wrapper class desc in `sun.invoke.util.Wrapper`, both as a micro-optimization and to help disambigutate the unfortunately named (my bad) `Wrapper::classDescriptor` method. Maybe we can roll that into this..? src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 471: > 469: case Allocate allocate -> > emitAllocBuffer(allocate); > 470: case BoxAddress boxAddress -> > emitBoxAddress(boxAddress); > 471: case SegmentBase _ -> emitSegmentBase(); Seem a bit too far detached from the changes at hand for a drive-by code cleanup? - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19585#pullrequestreview-2102953208 PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630049028 PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630061889 PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630074187
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Wed, 21 Feb 2024 02:18:08 GMT, Chris Hennick wrote: >> This provides a slightly more accurate bounding limit for >> `computeNextExponentialSoftCapped` when calling it from >> `computeNextGaussian`. This could cause the `while >> (computeNextExponentialSoftCapped(rng, limit) < limit)` check in >> `computeNextGaussian` on line 1402 to always be true, making the >> `nextGaussian` runtime unbounded in the worst case; but more likely, it >> would give a result that was truncated too close to zero. >> >> This change is being tested prior to submission to OpenJDK by >> https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf. > > Chris Hennick has updated the pull request incrementally with one additional > commit since the last revision: > > Bug fix: add-exports was for wrong package keep open - PR Comment: https://git.openjdk.org/jdk/pull/17703#issuecomment-2153184927
Re: RFR: 8333377: Migrate Generic Signature parsing to ClassFile API [v2]
> Core reflection's generic signature parsing uses an ancient library with > outdated visitor pattern on a tree model and contains unnecessary > boilerplates. This is a duplication of ClassFile API's signature model. We > should just move to ClassFile API, which is more throughoutly tested as well. > > To ensure compatibility, new tests are added to ensure consistent behavior > when encountering malformed signatures or signatures with missing types. The > reflective objects have been preserved and the only change is that lazy > expansion now happens from CF objects, to reduce compatibility risks. Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 16 commits: - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - Remove redundant try-catch in getEnclosingMethod/Constructor - Merge branch 'test/signature-error' into feature/new-generic-info - Fix everything - Fixxes - Stage - Stage new tests - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - ... and 6 more: https://git.openjdk.org/jdk/compare/2a37764e...19ee8797 - Changes: https://git.openjdk.org/jdk/pull/19281/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19281=01 Stats: 4605 lines in 66 files changed: 1027 ins; 3483 del; 95 mod Patch: https://git.openjdk.org/jdk/pull/19281.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19281/head:pull/19281 PR: https://git.openjdk.org/jdk/pull/19281
RFR: 8333749: Consolidate ConstantDesc conversion in java.base
In java.base, especially in bytecode generators, we have many different methods converting known valid Class and MethodType into ClassDesc and MethodTypeDesc. These conversions should be consolidated into the same utility method for the ease of future maintenance. - Commit messages: - Missed license header - Consolidate class/mt to desc operations Changes: https://git.openjdk.org/jdk/pull/19585/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19585=00 Issue: https://bugs.openjdk.org/browse/JDK-8333749 Stats: 160 lines in 8 files changed: 71 ins; 31 del; 58 mod Patch: https://git.openjdk.org/jdk/pull/19585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19585/head:pull/19585 PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v11]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Restored classpath - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/0e72fe46..c6476270 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=10 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=09-10 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v52]
On Thu, 30 May 2024 16:16:45 GMT, Scott Gibbons wrote: >> Re-write the IndexOf code without the use of the pcmpestri instruction, only >> using AVX2 instructions. This change accelerates String.IndexOf on average >> 1.3x for AVX2. The benchmark numbers: >> >> >> Benchmark Score >> Latest >> StringIndexOf.advancedWithMediumSub 343.573317.934 >> 0.925375393x >> StringIndexOf.advancedWithShortSub11039.081 1053.96 >> 1.014319384x >> StringIndexOf.advancedWithShortSub255.828110.541 >> 1.980027943x >> StringIndexOf.constantPattern9.361 11.906 >> 1.271872663x >> StringIndexOf.searchCharLongSuccess 4.216 4.218 >> 1.000474383x >> StringIndexOf.searchCharMediumSuccess3.133 3.216 >> 1.02649218x >> StringIndexOf.searchCharShortSuccess 3.763.761 >> 1.000265957x >> StringIndexOf.success9.186 >> 9.713 1.057369911x >> StringIndexOf.successBig 14.34146.343 >> 3.231504079x >> StringIndexOfChar.latin1_AVX2_String 6220.918 12154.52 >> 1.953814533x >> StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 >> 1.006629895x >> StringIndexOfChar.latin1_SSE4_String 6978.854 6818.689 >> 0.977049957x >> StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 >> 0.967675646x >> StringIndexOfChar.latin1_Short_String 7132.541 >> 6863.3590.962260014x >> StringIndexOfChar.latin1_Short_char 16013.389 16162.437 >> 1.009307711x >> StringIndexOfChar.latin1_mixed_String 7386.12314771.622 >> 1.15517x >> StringIndexOfChar.latin1_mixed_char9901.671 9782.245 >> 0.987938803 > > Scott Gibbons has updated the pull request incrementally with one additional > commit since the last revision: > > Fix copyright & a couple of comment typos Let me do quick testing with latest mainline (JDK 24 now). - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2153142794
RFR: 8333599: Improve description of \b matcher in j.u.r.Pattern
A documentation-only change to match the original intent and the implemented behavior. - Commit messages: - 8333599: Improve description of \b matcher in j.u.r.Pattern Changes: https://git.openjdk.org/jdk/pull/19583/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19583=00 Issue: https://bugs.openjdk.org/browse/JDK-8333599 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19583.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19583/head:pull/19583 PR: https://git.openjdk.org/jdk/pull/19583
Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v3]
On Thu, 6 Jun 2024 14:02:03 GMT, Jaikiran Pai wrote: >> Can I please get a review of this doc-only change which proposes to improve >> the code snippet that's in `java.util.zip.Deflater` and >> `java.util.zip.Inflater` to better explain the usage of those classes? This >> addresses https://bugs.openjdk.org/browse/JDK-8026127. >> >> The commit in the PR cleans up the snippet to correctly compress/decompress >> till the `Deflater` and `Inflater` are `finished()`. Additionally, the >> snippet also shows that the `Deflater` and `Inflater` are expected to be >> closed when their usage it done, to release the resources held by those >> instances. >> >> I've run `make docs-image` locally to verify that the generated snippet >> content as well as the link from `Inflater` work fine in the rendered >> javadoc HTML. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - minor change to the comment > - move the snippet to an external snippet thank you for converting the examples to take advantage of an external snippet Looks good Jai and thank you - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19507#pullrequestreview-2102777897
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
On Thu, 6 Jun 2024 17:08:45 GMT, Naoto Sato wrote: >> test/jdk/java/io/Console/RestoreEchoTest.java line 72: >> >>> 70: "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED", >>> 71: "-Djdk.console=java.base", >>> 72: "-classpath", testClasses, >> >> Consider this. If we remove `-classpath` (and `var testClasses`), not only >> will nothing break, but we'll be also able to use JUnit assertions and >> assumptions in `main` instead of manual check-then-throw. This will work >> because the expect-process will inherit the environment, which captures >> `CLASSPATH` ( see >> https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/ProcessBuilder.html#start() >> ). >> >> Again, the above is just something to consider. For all I know, you might've >> considered it already and rejected. > > I haven't considered that. Removed. Turned out that removing the classpath ends up not finding the test class: Error: Could not find or load main class RestoreEchoTest Caused by: java.lang.ClassNotFoundException: RestoreEchoTest ]; - PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629984403
Re: RFR: 8333477: Delete extra empty spaces in Makefiles
On Tue, 4 Jun 2024 07:47:46 GMT, SendaoYan wrote: > Hi all, > This PR several extra empty spaces and extra empty lines in several > Makefiles. It's trivial fix, no risk. > > Thanks. Changes requested by liach (Author). test/jdk/java/rmi/reliability/benchmark/bench/rmi/Makefile line 1: > 1: # This file change is dubious: 1. It does not have any trailing whitespace that can fail the skara checks. 2. If the duplicate blank lines in the end of this Makefile is indeed problematic (as fixed here), please fix the only other occasion in the JDK, which is the Makefile in the parent directory. (Checked with `\n$^\n$\Z` pattern in all Makefiles) Recommended actions: Either 1. Revert changes in this file; 2. Also update `test/jdk/java/rmi/reliability/benchmark/bench/Makefile` to remove the trailing blank line. - PR Review: https://git.openjdk.org/jdk/pull/19537#pullrequestreview-2102735910 PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1629981196
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v52]
On Thu, 30 May 2024 16:16:45 GMT, Scott Gibbons wrote: >> Re-write the IndexOf code without the use of the pcmpestri instruction, only >> using AVX2 instructions. This change accelerates String.IndexOf on average >> 1.3x for AVX2. The benchmark numbers: >> >> >> Benchmark Score >> Latest >> StringIndexOf.advancedWithMediumSub 343.573317.934 >> 0.925375393x >> StringIndexOf.advancedWithShortSub11039.081 1053.96 >> 1.014319384x >> StringIndexOf.advancedWithShortSub255.828110.541 >> 1.980027943x >> StringIndexOf.constantPattern9.361 11.906 >> 1.271872663x >> StringIndexOf.searchCharLongSuccess 4.216 4.218 >> 1.000474383x >> StringIndexOf.searchCharMediumSuccess3.133 3.216 >> 1.02649218x >> StringIndexOf.searchCharShortSuccess 3.763.761 >> 1.000265957x >> StringIndexOf.success9.186 >> 9.713 1.057369911x >> StringIndexOf.successBig 14.34146.343 >> 3.231504079x >> StringIndexOfChar.latin1_AVX2_String 6220.918 12154.52 >> 1.953814533x >> StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 >> 1.006629895x >> StringIndexOfChar.latin1_SSE4_String 6978.854 6818.689 >> 0.977049957x >> StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 >> 0.967675646x >> StringIndexOfChar.latin1_Short_String 7132.541 >> 6863.3590.962260014x >> StringIndexOfChar.latin1_Short_char 16013.389 16162.437 >> 1.009307711x >> StringIndexOfChar.latin1_mixed_String 7386.12314771.622 >> 1.15517x >> StringIndexOfChar.latin1_mixed_char9901.671 9782.245 >> 0.987938803 > > Scott Gibbons has updated the pull request incrementally with one additional > commit since the last revision: > > Fix copyright & a couple of comment typos Hi, everyone. I see that JDK 23 has now been forked, and new commits go into the JDK 24 branch. I would like to get this in as soon as possible to have as much time with fuzzers, etc. for everyone to be confident in the code. I have 3 positive reviews on this PR and would like to integrate. Please reply as soon as you reasonably can with objections or approval and I will integrate. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2153072708
Re: [External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal
Just bumping this one more time. I intend to start by opening a JIRA to add the two proposed methods to `ReflectionFactory`, and go from there. I guess that we might need a JEP for the proposed serialization restrictions, which is going to be considerably more involved, so I'm putting that off as a second step for now, pending further discussion. On Wed, May 15, 2024 at 9:00 PM - wrote: > Indeed, ReflectionFactory. newConstructorForSerialization can be used to > access otherwise-private constructors. Before JDK-8315810, it could even > allocate one class and call the constructor of another class. > > I strongly support your proposal to restrict ReflectionFactory. > > Regards, Chen > > On Wed, May 15, 2024 at 6:23 PM David Lloyd > wrote: > >> >> >> On Tue, May 14, 2024 at 10:01 AM David Lloyd >> wrote: >> >>> (Moving to core-libs-dev) >>> >>> On Tue, May 14, 2024 at 9:40 AM Alan Bateman >>> wrote: >>> On 14/05/2024 14:42, David Lloyd wrote: ReflectionFactory allows access to serialization facilities without any access checking (other than the defunct SecurityManager checks). Perhaps this class could gain some more methods, like this: * `newGetterForSerialization(Field field)` - includes ability to access `objectStreamFields` and `serialVersionUID`, or these could be separate methods * `newSetterForSerialziation(Field field)` Does this seem workable? The intention with ReflectionFactory is that serialization libraries go through the readObject/writeObject and other magic methods, to avoid field access. Probably best to being this to core-libs-dev for further discussion. >>> >>> This doesn't match my recollection. The `readObject` and `writeObject` >>> methods are optional private methods which serializable classes *may* >>> provide when they want a bespoke serialization strategy (and the other >>> methods that are accessed via this special class are similar in this >>> regard). They are not in any way magical in that they do not provide the >>> means to perform this part of the serialization process, and thus they are >>> not a substitute for field access in serialization. According to >>> JDK-8164908, these methods were added because at the time, Unsafe was still >>> state of the art for custom serialization and IIOP to access fields (with >>> libraries using Field actively moving to Unsafe instead). However Unsafe >>> did not and does not provide access to methods, so in order to avoid the >>> aforementioned `add-opens` explosion, these methods were added as a >>> compromise. Now that Unsafe is being deprecated, it is my opinion that this >>> does need to be revisited because at the time, Unsafe was the recommended >>> approach. >>> >>> [1] https://bugs.openjdk.org/browse/JDK-8164908 >>> >> It seems to me that it might be a good idea (as part of 8305968 >> (Integrity by default)) to put the `ReflectionFactory` API behind a >> restricted method call somehow, even considered separately from the changes >> suggested above. Maybe in conjunction with a non-standard switch that works >> similarly to `--enable-native-access`, e.g. >> `--enable-serialization=org.serial.framework`? That would also somewhat >> mitigate the security risk of adding the aforementioned methods or >> something like them to this class. >> >> Should I open a bug for either (or both) of these suggestions? Or perhaps >> there is a better way to proceed? >> >> -- >> - DML • he/him >> > -- - DML • he/him
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
On Thu, 6 Jun 2024 09:05:23 GMT, Pavel Rappo wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removed unnecessary add-opens > > test/jdk/java/io/Console/RestoreEchoTest.java line 66: > >> 64: OutputAnalyzer output = ProcessTools.executeProcess( >> 65: "expect", >> 66: "-n", > > What does `-n` do? It is for not reading the user's expect settings (`~/.expect.rc` if any) > test/jdk/java/io/Console/RestoreEchoTest.java line 72: > >> 70: "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED", >> 71: "-Djdk.console=java.base", >> 72: "-classpath", testClasses, > > Consider this. If we remove `-classpath` (and `var testClasses`), not only > will nothing break, but we'll be also able to use JUnit assertions and > assumptions in `main` instead of manual check-then-throw. This will work > because the expect-process will inherit the environment, which captures > `CLASSPATH` ( see > https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/ProcessBuilder.html#start() > ). > > Again, the above is just something to consider. For all I know, you might've > considered it already and rejected. I haven't considered that. Removed. > test/jdk/java/io/Console/restoreEcho.exp line 57: > >> 55: test "$rpprompt" "$rpinput" "-echo" "$rpexpected" >> 56: # See if the initialEcho is restored with `stty -a` >> 57: expect " $initialEcho " > > If you leave out those whitespace characters inside the quotes and > `$initialEcho` expands to `-echo`, it will be treated as an option to > `expect`, right? If so, consider this instead: > > expect -- $initialEcho > > But more importantly: could a test match `echo` in `-echo` and therefore > falsely pass? The spaces before/after `$initialEcho` are exactly to distinguish "echo" from "-echo", otherwise the test falsely succeeds as you pointed out. Although the test works as expected as it is, adding `--` would be safer. - PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629915235 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629915623 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629915890
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v10]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflects review comments - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/c583c633..0e72fe46 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=09 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=08-09 Stats: 11 lines in 2 files changed: 1 ins; 6 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8322256: Define and document GZIPInputStream concatenated stream semantics [v5]
> `GZIPInputStream` supports reading data from multiple concatenated GZIP data > streams since [JDK-4691425](https://bugs.openjdk.org/browse/JDK-4691425). In > order to do this, after a GZIP trailer frame is read, it attempts to read a > GZIP header frame and, if successful, proceeds onward to decompress the new > stream. If the attempt to decode a GZIP header frame fails, or happens to > trigger an `IOException`, it just ignores the trailing garbage and/or the > `IOException` and returns EOF. > > There are several issues with this: > > 1. The behaviors of (a) supporting concatenated streams and (b) ignoring > trailing garbage are not documented, much less precisely specified. > 2. Ignoring trailing garbage is dubious because it could easily hide errors > or other data corruption that an application would rather be notified about. > Moreover, the API claims that a `ZipException` will be thrown when corrupt > data is read, but obviously that doesn't happen in the trailing garbage > scenario (so N concatenated streams where the last one has a corrupted header > frame is indistinguishable from N-1 valid streams). > 3. There's no way to create a `GZIPInputStream` that does _not_ support > stream concatenation. > > On the other hand, `GZIPInputStream` is an old class with lots of existing > usage, so it's important to preserve the existing behavior, warts and all > (note: my the definition of "existing behavior" here includes the bug fix in > [JDK-7036144](https://bugs.openjdk.org/browse/JDK-7036144)). > > So this patch adds a new constructor that takes two new parameters > `allowConcatenation` and `allowTrailingGarbage`. The legacy behavior is > enabled by setting both to true; otherwise, they do what they sound like. In > particular, when `allowTrailingGarbage` is false, then the underlying input > must contain exactly one (if `allowConcatenation` is false) or exactly N (if > `allowConcatenation` is true) concatenated GZIP data streams, otherwise an > exception is guaranteed. Archie Cobbs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - Bump @since from 23 to 24. - Merge branch 'master' into JDK-8322256 - Relabel "trailing garbage" as "extra bytes" to sound less accusatory. - Simplify code by eliminating an impossible case. - Field name change and Javadoc wording tweaks. - Merge branch 'master' into JDK-8322256 - Javadoc wording tweaks. - Merge branch 'master' into JDK-8322256 - Clarify exceptions: sometimes ZipException, sometimes EOFException. - Merge branch 'master' into JDK-8322256 - ... and 2 more: https://git.openjdk.org/jdk/compare/75dc2f85...f845a75b - Changes: https://git.openjdk.org/jdk/pull/18385/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18385=04 Stats: 318 lines in 2 files changed: 292 ins; 13 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/18385.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18385/head:pull/18385 PR: https://git.openjdk.org/jdk/pull/18385
Integrated: 8330182: Start of release updates for JDK 24
On Mon, 15 Apr 2024 19:01:08 GMT, Joe Darcy wrote: > Get JDK 24 underway. This pull request has now been integrated. Changeset: 75dc2f85 Author:Joe Darcy Committer: Jesper Wilhelmsson URL: https://git.openjdk.org/jdk/commit/75dc2f8518d0adea30f7065d6732b807c0220756 Stats: 2083 lines in 50 files changed: 2019 ins; 7 del; 57 mod 8330182: Start of release updates for JDK 24 8330183: Add SourceVersion.RELEASE_24 8330184: Add source 24 and target 24 to javac Reviewed-by: iris, vromero, asotona, dholmes - PR: https://git.openjdk.org/jdk/pull/18787
RFR: 8332400: isspace argument should be a valid unsigned char
### Summary This change ensures we don't get undefined behavior when calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html). `isspace` accepts an `int` argument that "the application shall ensure is a character representable as an unsigned char or equal to the value of the macro EOF.". Previously, there was no checking of the values passed to `isspace`. I've replaced direct calls with a new wrapper `os::is_space` that performs a range check and prevents the possibility of undefined behavior from happening. For instances outside of Hotspot, I've added casts to `unsigned char`. **Testing** - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check `os::is_space` is working correctly. - tier1 - Commit messages: - Prevent undefined behaviour when calling isspace. Changes: https://git.openjdk.org/jdk/pull/19567/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19567=00 Issue: https://bugs.openjdk.org/browse/JDK-8332400 Stats: 31 lines in 8 files changed: 20 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/19567.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19567/head:pull/19567 PR: https://git.openjdk.org/jdk/pull/19567
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 09:47:30 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Fix default description of keep-packaged-modules OK. I'm aware about the timelines for JDK 23. Thanks for bringing clarity to this. My aim will be to bring this into JDK 24 with a JEP then. Hopefully we can bring this to a successful conclusion that way. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2152833052
Integrated: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3
On Fri, 31 May 2024 14:55:57 GMT, Daniel Fuchs wrote: > HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests verify that > loggers are GC'ed (or not GC'ed) after a reset or an update. For that they > poll a ReferenceQueue in a loop. The number of iteration is adjusted > according to the jtreg timeout factor. However, the logic in the test did not > expect that the timeout might be less than 1. > > This fix does two things: > > 1. fix the adjustCount logic - so that the number of iteration can only be > increased > 2. use remove(timeout) instead of poll() in order to minimize the waiting > time. This pull request has now been integrated. Changeset: d02cb742 Author:Daniel Fuchs URL: https://git.openjdk.org/jdk/commit/d02cb742f79e88c6438ca58a6357fe432fb286cb Stats: 16 lines in 2 files changed: 0 ins; 2 del; 14 mod 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3 Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/19503
Re: RFR: 8325984: 4 jcstress tests are failing in Tier6 4 times each
On Thu, 6 Jun 2024 10:48:51 GMT, Aleksey Shipilev wrote: > I think only Oracle CIs run these tests through jtreg wrappers? We do run them in our CI. Not sure who else runs them that way. - PR Comment: https://git.openjdk.org/jdk/pull/19565#issuecomment-2152799029
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 12:46:36 GMT, jengebr wrote: >> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless >> cloning of Object[0] instances. This cloning is intended to prevent callers >> from changing array contents, but many `CopyOnWriteArrayList`s are allocated >> to size zero, or are otherwise maintained empty, so cloning is unnecessary. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 74.487 ± 1.793 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 27.918 ± 0.759 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.656 ± 0.375 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 15.415 ± 0.489 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.608 ± 0.363 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 15.374 ± 0.260 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 15.688 ± 0.350 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2625.125 ± 71.802 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2607.447 ± 46.400 ns/op >> >> >> After: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 75.365 ± 2.092 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 20.803 ± 0.539 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.808 ± 0.582 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 12.980 ± 0.418 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.627 ± 0.173 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 12.864 ± 0.408 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 12.931 ± 0.255 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2615.500 ± 30.771 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2583.892 ± 62.086 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Adding benchmarks for readObject Yes, I think this is now OK (and maybe barely worth doing). - PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2152726032
Lambda Classes reachability from GC roots in JDK11 and JDK17
Hello Lambda Experts, I am looking for clarifications on the following questions. 1. I observed in my tests that lambda classes generated for inline lambda expressions (ex: LambdaUtil::print), are STRONGLY linked to defining class loader and never gets collected on GC. This behaviour is same in both JDK11 and JDK17. Can you please confirm is this accurate? 2. Also, I observed that the lambda classes generated for inline lambda expressions (ex: LambdaUtil::print) not adding additional overhead to Metaspace in JDK17 compared to that of JDK11 . Can you please confirm is this accurate? Regards, Prasad
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 12:46:36 GMT, jengebr wrote: >> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless >> cloning of Object[0] instances. This cloning is intended to prevent callers >> from changing array contents, but many `CopyOnWriteArrayList`s are allocated >> to size zero, or are otherwise maintained empty, so cloning is unnecessary. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 74.487 ± 1.793 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 27.918 ± 0.759 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.656 ± 0.375 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 15.415 ± 0.489 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.608 ± 0.363 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 15.374 ± 0.260 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 15.688 ± 0.350 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2625.125 ± 71.802 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2607.447 ± 46.400 ns/op >> >> >> After: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 75.365 ± 2.092 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 20.803 ± 0.539 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.808 ± 0.582 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 12.980 ± 0.418 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.627 ± 0.173 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 12.864 ± 0.408 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 12.931 ± 0.255 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2615.500 ± 30.771 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2583.892 ± 62.086 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Adding benchmarks for readObject I think this is good. I'll let Doug and Viktor to confirm their comments were addressed. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19527#pullrequestreview-2102178280
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Wed, 5 Jun 2024 14:56:26 GMT, jengebr wrote: > clone() performs a shallow copy, so there is currently no Object[] allocated > and therefore nothing to optimize. Yes, I believe so. - PR Review Comment: https://git.openjdk.org/jdk/pull/19527#discussion_r1629666551
Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v3]
> Can I please get a review of this doc-only change which proposes to improve > the code snippet that's in `java.util.zip.Deflater` and > `java.util.zip.Inflater` to better explain the usage of those classes? This > addresses https://bugs.openjdk.org/browse/JDK-8026127. > > The commit in the PR cleans up the snippet to correctly compress/decompress > till the `Deflater` and `Inflater` are `finished()`. Additionally, the > snippet also shows that the `Deflater` and `Inflater` are expected to be > closed when their usage it done, to release the resources held by those > instances. > > I've run `make docs-image` locally to verify that the generated snippet > content as well as the link from `Inflater` work fine in the rendered javadoc > HTML. Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision: - minor change to the comment - move the snippet to an external snippet - Changes: - all: https://git.openjdk.org/jdk/pull/19507/files - new: https://git.openjdk.org/jdk/pull/19507/files/098212a6..7a72a760 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19507=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19507=01-02 Stats: 158 lines in 3 files changed: 97 ins; 58 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19507.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19507/head:pull/19507 PR: https://git.openjdk.org/jdk/pull/19507
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 10:42:20 GMT, Alan Bateman wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix default description of keep-packaged-modules > > I've read through all src changes. I think Sundar is looking at the code > changes too. > > The overall design seems solid. I know it took a long time to get there but > this is nature of a feature like this. At this point I regret that there > isn't a JEP. A JEP would have captured the motivation, outlined the design, > the reasoning for the restrictions, and document the design > choices/directions that have been prototyped. If there isn't a JEP then I > suppose it can come later if the feature is progressed and there is > discussion about making it the default rather than opt-in at build configure > time. > > As cleanup, I think we will need to bring some consistency to the terminology > and phrasing in documentation, code and comments. Right now there is > "run-time linking", "linkable run-time", "run-time linkable JDK image", > "linkable jimage". > > Also as cleanup, I think the code needs more comments. There is no JEP right > now with an authoritive description of the feature so anyone maintaining this > code will have to figure out a lot of details. It feels like there should be > somehting that documents the effect of --enable-runtime-link-image, how the > diffs are generated and saved, and how they are used by jlink. There is also > a lot of new code in ImageFileCreator and JlinkTask that is asking for some > method descriptions so that anyone touching this code can quickly understand > what these methods are doing. I don't want to get into code style issues now > but I think it would be helpful for future maintainers to avoid more of the > overfly long lines if possible (some of them are 150, 160, 170+ and really > hard to look at code side-by-side). > @AlanBateman Sure, I appreciate the feedback. Do I understand it correctly > that this won't get into JDK 23 then? If so, perhaps the better way would be > to draft a JEP for JDK 24 and get that proposed early. What I'd like to avoid > is for this change to don't see much movement for a long time between now and > RDP 1 of JDK 24 and have a similar crunch when JDK 24 is close to forking. > You mention clean-up a lot. Is that suggesting it _can_ go into JDK 23 and > clean-up in ramp-down? I'm confused... > > Some clarity on how to best approach getting this integrated that would be > acceptable for all involved would be appreciated. Thanks! The fork for JDK 23 is today so if I was running with this feature then I wouldn't integrate it today. If you are willing to put the time into writing a JEP then I think it's the right thing to do. We should probably have brought this up long before now. I'm happy to help review iterations. There is a lot to write down and this will be very valuable for future phases of this work. I assume future phases. We agreed some restrictions to reduce complexity for this first phase. Future phases may remove these and maybe there is confidence in the future to make it default. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2152576950
Re: RFR: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher [v2]
> Can I please get a review for this change which proposes to remove the > `CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code? > > This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that > JBS issue, in a recent PR discussion, it was suggested > https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this > macro should be removed and the failure of a JNI specified operation (the > ones for which this macro is being used) should be determined based on a > `NULL` value returned from that function. The commit in this PR removes this > macros and updates the call sites to do a `NULL` check. > > Given the nature of this change, no new tests have been added. tier1, tier2 > and tier3 testing passed successfully with these changes. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: improve comments - Changes: - all: https://git.openjdk.org/jdk/pull/19576/files - new: https://git.openjdk.org/jdk/pull/19576/files/1c2a3dfb..1b3d630a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19576=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19576=00-01 Stats: 20 lines in 1 file changed: 0 ins; 8 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/19576.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19576/head:pull/19576 PR: https://git.openjdk.org/jdk/pull/19576
Lambda Classes reachability from GC roots in JDK11 and JDK17
Hello Lambda Experts, I am looking for clarifications on the following questions. 1. I observed in my tests that lambda classes generated for inline lambda expressions (ex: LambdaUtil::print), are STRONGLY linked to defining class loader and never gets collected on GC. This behaviour is same in both JDK11 and JDK17. Can you please confirm is this accurate? 2. Also, I observed that the lambda classes generated for inline lambda expressions (ex: LambdaUtil::print) not adding additional overhead to Metaspace. Can you please confirm is this accurate? Regards, Prasad
RFR: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher
Can I please get a review for this change which proposes to remove the `CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code? This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that JBS issue, in a recent PR discussion, it was suggested https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this macro should be removed and the failure of a JNI specified operation (the ones for which this macro is being used) should be determined based on a `NULL` value returned from that function. The commit in this PR removes this macros and updates the call sites to do a `NULL` check. Given the nature of this change, no new tests have been added. tier1, tier2 and tier3 testing passed successfully with these changes. - Commit messages: - simplify function comments - 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher Changes: https://git.openjdk.org/jdk/pull/19576/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19576=00 Issue: https://bugs.openjdk.org/browse/JDK-8333714 Stats: 76 lines in 1 file changed: 45 ins; 9 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19576.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19576/head:pull/19576 PR: https://git.openjdk.org/jdk/pull/19576
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
On Wed, 5 Jun 2024 17:48:25 GMT, Naoto Sato wrote:
>> This test intends to verify the behavior of JdkConsole for the java.base
>> module, wrt restoring the echo. The test assumes the internal methods that
>> sets/gets the echo status of the platform.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removed unnecessary add-opens
This feels like a better, end-to-end test. Thank you for your perseverance.
Below are some comments.
test/jdk/java/io/Console/RestoreEchoTest.java line 66:
> 64: OutputAnalyzer output = ProcessTools.executeProcess(
> 65: "expect",
> 66: "-n",
What does `-n` do?
test/jdk/java/io/Console/RestoreEchoTest.java line 70:
> 68: initialEcho,
> 69: jdkDir + "/bin/java",
> 70: "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED",
We don't seem to need `--add-opens`.
test/jdk/java/io/Console/RestoreEchoTest.java line 72:
> 70: "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED",
> 71: "-Djdk.console=java.base",
> 72: "-classpath", testClasses,
Consider this. If we remove `-classpath` (and `var testClasses`), not only will
nothing break, but we'll be also able to use JUnit assertions and assumptions
in `main` instead of manual check-then-throw. This will work because the
expect-process will inherit the environment, which captures `CLASSPATH` ( see
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/ProcessBuilder.html#start()
).
Again, the above is just something to consider. For all I know, you might've
considered it already and rejected.
test/jdk/java/io/Console/RestoreEchoTest.java line 78:
> 76: if (eval != 0) {
> 77: throw new RuntimeException("Test failed. Exit value from
> 'expect' command: " + eval);
> 78: }
It could've been `assertEquals(0, output.getExitValue())`.
test/jdk/java/io/Console/RestoreEchoTest.java line 93:
> 91: // testing readLine()
> 92: String input = con.readLine("prompt: ");
> 93: con.printf("input is %s\n", input);
I know that this test is only run on Linux and macOS, and yet `%n` would be
better than `\n`. It's one of those cases where it's simpler to use a general
solution than to use a less general one and explain why it still does the job.
test/jdk/java/io/Console/RestoreEchoTest.java line 97:
> 95: // testing readPassword()
> 96: input = String.valueOf(con.readPassword("password prompt: "));
> 97: con.printf("password is %s\n", input);
Ditto on `%n`.
test/jdk/java/io/Console/restoreEcho.exp line 57:
> 55: test "$rpprompt" "$rpinput" "-echo" "$rpexpected"
> 56: # See if the initialEcho is restored with `stty -a`
> 57: expect " $initialEcho "
If you leave out those whitespace characters inside the quotes and
`$initialEcho` expands to `-echo`, it will be treated as an option to `expect`,
right? If so, consider this instead:
expect -- $initialEcho
But more importantly: could a test match `echo` in `-echo` and therefore
falsely pass?
-
PR Review: https://git.openjdk.org/jdk/pull/19315#pullrequestreview-2101329552
PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629085477
PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629331808
PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629348345
PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629359368
PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629361442
PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629361824
PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629445000
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless > cloning of Object[0] instances. This cloning is intended to prevent callers > from changing array contents, but many `CopyOnWriteArrayList`s are allocated > to size zero, or are otherwise maintained empty, so cloning is unnecessary. > > Results from the included JMH benchmark: > Before: > > BenchmarkMode Cnt > Score Error Units > CopyOnWriteArrayListBenchmark.clear avgt5 > 74.487 ± 1.793 ns/op > CopyOnWriteArrayListBenchmark.clearEmpty avgt5 > 27.918 ± 0.759 ns/op > CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 > 16.656 ± 0.375 ns/op > CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 > 15.415 ± 0.489 ns/op > CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 > 21.608 ± 0.363 ns/op > CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 > 15.374 ± 0.260 ns/op > CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 > 15.688 ± 0.350 ns/op > > > After: > > BenchmarkMode Cnt > Score Error Units > CopyOnWriteArrayListBenchmark.clear avgt5 > 75.365 ± 2.092 ns/op > CopyOnWriteArrayListBenchmark.clearEmpty avgt5 > 20.803 ± 0.539 ns/op > CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 > 16.808 ± 0.582 ns/op > CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 > 12.980 ± 0.418 ns/op > CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 > 21.627 ± 0.173 ns/op > CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 > 12.864 ± 0.408 ns/op > CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 > 12.931 ± 0.255 ns/op jengebr has updated the pull request incrementally with one additional commit since the last revision: Adding benchmarks for readObject - Changes: - all: https://git.openjdk.org/jdk/pull/19527/files - new: https://git.openjdk.org/jdk/pull/19527/files/9ab83c9d..b1920f7a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19527=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19527=01-02 Stats: 43 lines in 1 file changed: 41 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19527.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19527/head:pull/19527 PR: https://git.openjdk.org/jdk/pull/19527
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 10:42:20 GMT, Alan Bateman wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix default description of keep-packaged-modules > > I've read through all src changes. I think Sundar is looking at the code > changes too. > > The overall design seems solid. I know it took a long time to get there but > this is nature of a feature like this. At this point I regret that there > isn't a JEP. A JEP would have captured the motivation, outlined the design, > the reasoning for the restrictions, and document the design > choices/directions that have been prototyped. If there isn't a JEP then I > suppose it can come later if the feature is progressed and there is > discussion about making it the default rather than opt-in at build configure > time. > > As cleanup, I think we will need to bring some consistency to the terminology > and phrasing in documentation, code and comments. Right now there is > "run-time linking", "linkable run-time", "run-time linkable JDK image", > "linkable jimage". > > Also as cleanup, I think the code needs more comments. There is no JEP right > now with an authoritive description of the feature so anyone maintaining this > code will have to figure out a lot of details. It feels like there should be > somehting that documents the effect of --enable-runtime-link-image, how the > diffs are generated and saved, and how they are used by jlink. There is also > a lot of new code in ImageFileCreator and JlinkTask that is asking for some > method descriptions so that anyone touching this code can quickly understand > what these methods are doing. I don't want to get into code style issues now > but I think it would be helpful for future maintainers to avoid more of the > overfly long lines if possible (some of them are 150, 160, 170+ and really > hard to look at code side-by-side). @AlanBateman Sure, I appreciate the feedback. Do I understand it correctly that this won't get into JDK 23 then? If so, perhaps the better way would be to draft a JEP for JDK 24 and get that proposed early. What I'd like to avoid is for this change to don't see much movement for a long time between now and RDP 1 of JDK 24 and have a similar crunch when JDK 24 is close to forking. You mention clean-up a lot. Is that suggesting it *can* go into JDK 23 and clean-up in ramp-down? I'm confused... Some clarity on how to best approach getting this integrated that would be acceptable for all involved would be appreciated. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2152248595
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Thu, 6 Jun 2024 06:41:48 GMT, ExE Boss wrote: >> Sean Gwizdak 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: >> >> - Remove trailing whitespace. >> - Move hashCode benchmark into the newly created MethodBenchmark file >> - Merge branch 'master' into method-hashcode-JDK-8332249 >> - Remove changes to JavaDoc per guidance. >> - Fix whitespace issues pointed by the bot >> - Micro-optimize Method.hashCode > > src/java.base/share/classes/java/lang/reflect/Method.java line 392: > >> 390: .hashCode(); >> 391: } >> 392: return hc; > > The `hash` field should probably somehow be shared with the `Method.root` > instance, so that it doesn’t need to be recomputed when different code gets a > `Method` reference. Currently the hashCode computation is quite cheap. I think we can consider this delegation if it gets more complex, say if the hash code now considers parameters. - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1629365372
Re: RFR: 8325984: 4 jcstress tests are failing in Tier6 4 times each
On Wed, 5 Jun 2024 19:21:56 GMT, Jorn Vernee wrote: > These 4 tests were failing due to an incompatibility with jcstress. They were > problemlisted in past (https://bugs.openjdk.org/browse/JDK-8326062). > > Now that jcstress has been updated > (https://github.com/openjdk/jdk/pull/19332) with the relevant fix > (https://github.com/openjdk/jcstress/pull/147), we can re-enable these tests. > > Testing: I've verified that all 4 tests now pass on Linux-x64 I think only Oracle CIs run these tests through jtreg wrappers? Anyway, this looks good to me. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19565#pullrequestreview-2101607822
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 09:47:30 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Fix default description of keep-packaged-modules I've read through all src changes. I think Sundar is looking at the code changes too. The overall design seems solid. I know it took a long time to get there but this is nature of a feature like this. At this point I regret that there isn't a JEP. A JEP would have captured the motivation, outlined the design, the reasoning for the restrictions, and document the design choices/directions that have been prototyped. If there isn't a JEP then I suppose it can come later if the feature is progressed and there is discussion about making it the default rather than opt-in at build configure time. As cleanup, I think we will need to bring some consistency to the terminology and phrasing in documentation, code and comments. Right now there is "run-time linking", "linkable run-time", "run-time linkable JDK image", "linkable jimage". Also as cleanup, I think the code needs more comments. There is no JEP right now with an authoritive description of the feature so anyone maintaining this code will have to figure out a lot of details. It feels like there should be somehting that documents the effect of --enable-runtime-link-image, how the diffs are generated and saved, and how they are used by jlink. There is also a lot of new code in ImageFileCreator and JlinkTask that is asking for some method descriptions so that anyone touching this code can quickly understand what these methods are doing. I don't want to get into code style issues now but I think it would be helpful for future maintainers to avoid more of the overfly long lines if possible (some of them are 150, 160, 170+ and really hard to look at code side-by-side). - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2151964298
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v18]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - reverted static initialization of ConstantPoolBuilder and CP entries - fixed naming conventions - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/e814749a..f870a8df Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=17 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=16-17 Stats: 43 lines in 2 files changed: 7 ins; 13 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 09:47:30 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Fix default description of keep-packaged-modules Marked as reviewed by ihse (Reviewer). The wording was much better than what I suggested. Thanks. - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2101492920 PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2151870228
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v31]
On Thu, 6 Jun 2024 09:33:43 GMT, Magnus Ihse Bursie wrote: > As Erik says. You need to add something like: `DEFAULT_DESC: [the inverse of > --enable-runtime-link-image]`. https://github.com/openjdk/jdk/pull/14787/commits/7a8f839e55c5109deeb5022d2338b37387c95c85 does that. Sorry it clashed with your comment. It sets it to `enabled by default unless --enable-runtime-link-image is set`. Hopefully that is OK as well. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2151859093
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Fix default description of keep-packaged-modules - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/b72648ba..7a8f839e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14787=31 - incr: https://webrevs.openjdk.org/?repo=jdk=14787=30-31 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v17]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: use of jdk.internal.constant to improve performance - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/902b02ee..e814749a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=16 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=15-16 Stats: 113 lines in 6 files changed: 35 ins; 7 del; 71 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v31]
On Wed, 5 Jun 2024 17:31:44 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with six > additional commits since the last revision: > > - Move JImageHelper > - Update wording on multi-hop. > - Remove printStackTrace() > - Fix comment. Stream.toList() > - Use pattern matching for instanceof in JRTArchive::equals > - Rename JLINK_PRODUCE_RUNTIME_LINK_JDK to JLINK_PRODUCE_LINKABLE_RUNTIME As Erik says. You need to add something like: `DEFAULT_DESC: [the inverse of --enable-runtime-link-image]`. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2151836837
Re: RFR: 8333265: De-duplicate method references in java.util.stream.FindOps
On Thu, 30 May 2024 12:50:36 GMT, Claes Redestad wrote:
> Extracting duplicate method references to static field reduce proxy class
> spinning and loading. In this case 2 less classes loaded when using
> `findAny()` on each type of stream.
The intermediate `Predicate`s and `Supplier`s don’t need to be fields, and it’s
probably better for them to be locals:
src/java.base/share/classes/java/util/stream/FindOps.java line 204:
> 202:
> 203: static final FindOp OP_FIND_ANY = new FindOp<>(false,
> 204: StreamShape.REFERENCE, Optional.empty(), IS_PRESENT,
> NEW);
Suggestion:
static final TerminalOp OP_FIND_FIRST, OP_FIND_ANY;
static {
Predicate> isPresent = Optional::isPresent;
Supplier>> newSink
= FindSink.OfRef::new;
OP_FIND_FIRST = new FindOp<>(true, StreamShape.REFERENCE,
Optional.empty(), isPresent, newSink);
OP_FIND_ANY = new FindOp<>(false, StreamShape.REFERENCE,
Optional.empty(), isPresent, newSink);
}
src/java.base/share/classes/java/util/stream/FindOps.java line 227:
> 225: StreamShape.INT_VALUE, OptionalInt.empty(),
> IS_PRESENT, NEW);
> 226: static final TerminalOp OP_FIND_ANY =
> new FindOp<>(false,
> 227: StreamShape.INT_VALUE, OptionalInt.empty(),
> IS_PRESENT, NEW);
Suggestion:
static final TerminalOp OP_FIND_FIRST,
OP_FIND_ANY;
static {
Predicate isPresent = OptionalInt::isPresent;
Supplier newSink
= FindSink.OfInt::new;
OP_FIND_FIRST = new FindOp<>(true, StreamShape.INT_VALUE,
OptionalInt.empty(), isPresent, newSink);
OP_FIND_ANY = new FindOp<>(false, StreamShape.INT_VALUE,
OptionalInt.empty(), isPresent, newSink);
}
src/java.base/share/classes/java/util/stream/FindOps.java line 250:
> 248: StreamShape.LONG_VALUE, OptionalLong.empty(),
> IS_PRESENT, NEW);
> 249: static final TerminalOp OP_FIND_ANY =
> new FindOp<>(false,
> 250: StreamShape.LONG_VALUE, OptionalLong.empty(),
> IS_PRESENT, NEW);
Suggestion:
static final TerminalOp OP_FIND_FIRST,
OP_FIND_ANY;
static {
Predicate isPresent = OptionalLong::isPresent;
Supplier newSink
= FindSink.OfLong::new;
OP_FIND_FIRST = new FindOp<>(true, StreamShape.LONG_VALUE,
OptionalLong.empty(), isPresent, newSink);
OP_FIND_ANY = new FindOp<>(false, StreamShape.LONG_VALUE,
OptionalLong.empty(), isPresent, newSink);
}
src/java.base/share/classes/java/util/stream/FindOps.java line 273:
> 271: StreamShape.DOUBLE_VALUE, OptionalDouble.empty(),
> IS_PRESENT, NEW);
> 272: static final TerminalOp OP_FIND_ANY
> = new FindOp<>(false,
> 273: StreamShape.DOUBLE_VALUE, OptionalDouble.empty(),
> IS_PRESENT, NEW);
Suggestion:
static final TerminalOp OP_FIND_FIRST,
OP_FIND_ANY;
static {
Predicate isPresent = OptionalDouble::isPresent;
Supplier newSink
= FindSink.OfDouble::new;
OP_FIND_FIRST = new FindOp<>(true, StreamShape.DOUBLE_VALUE,
OptionalDouble.empty(), isPresent, newSink);
OP_FIND_ANY = new FindOp<>(false, StreamShape.DOUBLE_VALUE,
OptionalDouble.empty(), isPresent, newSink);
}
-
PR Review: https://git.openjdk.org/jdk/pull/19477#pullrequestreview-2101252355
PR Review Comment: https://git.openjdk.org/jdk/pull/19477#discussion_r1629032905
PR Review Comment: https://git.openjdk.org/jdk/pull/19477#discussion_r1629035242
PR Review Comment: https://git.openjdk.org/jdk/pull/19477#discussion_r1629040800
PR Review Comment: https://git.openjdk.org/jdk/pull/19477#discussion_r1629043309
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v16]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed imports - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/019633bd..902b02ee Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=15 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=14-15 Stats: 18 lines in 1 file changed: 6 ins; 7 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v15]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review There are new possibilities with decoupled constants implementation, thank you for the reminder. Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/17108/files - new: https://git.openjdk.org/jdk/pull/17108/files/9360b0eb..019633bd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17108=14 - incr: https://webrevs.openjdk.org/?repo=jdk=17108=13-14 Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v2]
> Can I please get a review of this doc-only change which proposes to improve > the code snippet that's in `java.util.zip.Deflater` and > `java.util.zip.Inflater` to better explain the usage of those classes? This > addresses https://bugs.openjdk.org/browse/JDK-8026127. > > The commit in the PR cleans up the snippet to correctly compress/decompress > till the `Deflater` and `Inflater` are `finished()`. Additionally, the > snippet also shows that the `Deflater` and `Inflater` are expected to be > closed when their usage it done, to release the resources held by those > instances. > > I've run `make docs-image` locally to verify that the generated snippet > content as well as the link from `Inflater` work fine in the rendered javadoc > HTML. Jaikiran Pai 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 two additional commits since the last revision: - merge latest from master branch - 8026127: Deflater/Inflater documentation incomplete/misleading - Changes: - all: https://git.openjdk.org/jdk/pull/19507/files - new: https://git.openjdk.org/jdk/pull/19507/files/d8d86bcb..098212a6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19507=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19507=00-01 Stats: 19514 lines in 482 files changed: 12009 ins; 5593 del; 1912 mod Patch: https://git.openjdk.org/jdk/pull/19507.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19507/head:pull/19507 PR: https://git.openjdk.org/jdk/pull/19507
Re: RFR: 8333334: C2: Make result of `Node::dominates` more precise to enhance scalar replacement [v2]
> This patch changes the algorithm of `Node::dominates` to make the result more > precise, and allows the iterators of `ConcurrentHashMap` to be scalar > replaced. > > The previous algorithm will return a conservative result when encountering a > dead control flow, and only try the first two input paths of a multi-input > Region node, which may prevent the scalar replacement in some cases. > > For example, with G1 GC enabled, C2 generates GC barriers for > `ConcurrentHashMap` iteration operations at some early phases, and then > eliminates them in a later IGVN, but `LoadNode` is also idealized in the same > IGVN. This causes `LoadNode::Ideal` to see some dead barrier control flows, > and refuse to split some instance field loads through Phi due to the > conservative result of `Node::dominates`, and thus the scalar replacement can > not be applied to iterators in the later macro elimination phase. > > This patch allows `Node::dominates` to try other paths of the last > multi-input Region node when the first path is dead, and makes > `ConcurrentHashMap` iteration ~30% faster: > > > Benchmark(nkeys) Mode CntScore > Error Units > Maps.testConcurrentHashMapIterators1 avgt 15 414099.085 ± > 33230.945 ns/op # baseline > Maps.testConcurrentHashMapIterators1 avgt 15 315490.281 ± > 3037.056 ns/op # patch > > > Testing: tier1-4. MaxXing has updated the pull request incrementally with one additional commit since the last revision: Revert last commit, and push the `LoadNode` back to the worklist to wait for the dead code to be removed. - Changes: - all: https://git.openjdk.org/jdk/pull/19496/files - new: https://git.openjdk.org/jdk/pull/19496/files/e3330ece..b5db38dc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19496=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19496=00-01 Stats: 107 lines in 4 files changed: 39 ins; 34 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/19496.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19496/head:pull/19496 PR: https://git.openjdk.org/jdk/pull/19496
Re: RFR: 8333334: C2: Make result of `Node::dominates` more precise to enhance scalar replacement [v2]
On Wed, 5 Jun 2024 05:40:12 GMT, Tobias Hartmann wrote:
>> MaxXing has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert last commit, and push the `LoadNode` back to the worklist to wait
>> for the dead code to be removed.
>
> Impressive results! I haven't looked at the change yet but here are a few
> questions / requests:
> - Could you add a screenshot of the IR of the case you are describing?
> - Wouldn't it help to add the LoadNode back to the IGVN worklist and wait for
> the dead path to be removed?
> - Could you add an [IR
> framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/README.md)
> test that verifies that the optimization works as expected?
>
> Thanks,
> Tobias
@TobiHartmann Hi Tobias, thanks for your reply!
> Could you add a screenshot of the IR of the case you are describing?
Sure. Here's a simple example of iterating over the keys of `ConcurrentHashMap`:
public long sumMapKeys() {
long sum = 0;
Enumeration it = map.keys();
while (it.hasMoreElements()) {
sum += (Long) it.nextElement();
}
return sum;
}
And here's what `-XX:+PrintEscapeAnalysis -XX:+PrintEliminateAllocations` says:
JavaObject(6) NoEscape(NoEscape) [ 183F 189F 205F 196F 191F 179F 186F 202F 208F
233F 637F 1069F [ 104 109 ]] 92 Allocate === 76 6 69 8 1 (90 89 24 1 1 1
22 1 1 43 77 87 ) [[ 93 94 95 102 103 104 ]] rawptr:NotNull ( int:>=0,
java/lang/Object:NotNull *, bool, top, bool ) ConcurrentHashMap::keys @ bci:16
(line 2152) MyBenchmark::sumMapKeys @ bci:6 (line 105) !jvms:
ConcurrentHashMap::keys @ bci:16 (line 2152) MyBenchmark::sumMapKeys @ bci:6
(line 105)
LocalVar(60) [ 92P [ 109 183b 189b 205b ]]104 Proj === 92 [[ 105 109 183
189 205 ]] #5 !jvms: ConcurrentHashMap::keys @ bci:16 (line 2152)
MyBenchmark::sumMapKeys @ bci:6 (line 105)
LocalVar(103) [ 104 92P [ 196b 191b 179b 186b 202b 208b 233b 637b 1069b ]]
109 CheckCastPP === 106 104 [[ 1801 1771 1754 1584 1503 1503 1490 1490 1466
1466 1451 1451 208 1393 1381 1381 179 179 186 186 208 196 191 191 196 202 202
228 297 233 233 1363 1363 1393 1321 1321 1311 637 637 648 648 998 988 1311 1301
477 477 487 487 497 497 569 1301 672 685 767 672 978 920 539 539 1069 1069 557
557 1128 569 631 1061 685 631 ]]
#java/util/concurrent/ConcurrentHashMap$KeyIterator
(java/util/Iterator,java/util/Enumeration):NotNull:exact *
Oop:java/util/concurrent/ConcurrentHashMap$KeyIterator
(java/util/Iterator,java/util/Enumeration):NotNull:exact * !jvms:
ConcurrentHashMap::keys @ bci:16 (line 2152) MyBenchmark::sumMapKeys @ bci:6
(line 105)
NotScalar (Field load) 109 CheckCastPP === 106 104 [[ 1801 1771 1754 1584
1503 1503 1490 1490 1128 569 1451 1451 208 1393 1381 1381 672 685 1069 1069 208
196 191 191 196 978 685 631 297 767 672 569 1301 1393 1321 1321 1311 557 557
631 1061 998 988 1311 1301 477 477 487 487 497 497 ]]
#java/util/concurrent/ConcurrentHashMap$KeyIterator
(java/util/Iterator,java/util/Enumeration):NotNull:exact *,iid=92
Oop:java/util/concurrent/ConcurrentHashMap$KeyIterator
(java/util/Iterator,java/util/Enumeration):NotNull:exact *,iid=92 !jvms:
ConcurrentHashMap::keys @ bci:16 (line 2152) MyBenchmark::sumMapKeys @ bci:6
(line 105)
2186 LoadI === _ 1973 191 [[ 2185 ]]
@java/util/concurrent/ConcurrentHashMap$Traverser+12 *, name=index, idx=9; #int
!orig=[2178],[2171],[1373] !jvms: ConcurrentHashMap$Traverser::advance @ bci:51
(line 3369) ConcurrentHashMap$KeyIterator::next @ bci:28 (line 3468)
ConcurrentHashMap$KeyIterator::nextElement @ bci:1 (line 3472)
MyBenchmark::sumMapKeys @ bci:21 (line 107)
It shows that scalar replacement is aborted due to field load 2186:
https://github.com/openjdk/jdk/assets/5129820/2e35769a-6e0f-4c8a-96ec-09923a5eb2c0;>
As we can see its memory is a Phi, and it should be split by
`LoadNode::split_through_phi` if its address `109 CheckCastPP` dominates the
control flow of the Phi node `1330 Region`.
The control node of `CheckCastPP` is `106 Proj`:
https://github.com/openjdk/jdk/assets/5129820/ebf6f7f8-fcd1-4d83-8734-3f1864dcc946;>
And it does dominate 1330, although not that obvious:
https://github.com/openjdk/jdk/assets/5129820/e53b25de-c966-4f89-b30e-54ccc35f645b;>
But `Node::dominates` think it doesn't because of the dead control flow.
> Wouldn't it help to add the LoadNode back to the IGVN worklist and wait for
> the dead path to be removed?
I tried to revert the change of main algorithm of `Node::dominates`, and just
add some code to add the LoadNode back to the worklist if we met dead path. It
works, still makes the iteration ~30% faster:
Benchmark (nkeys) Mode CntScore
Error Units
Maps.testConcurrentHashMapIterators 1 avgt 15 312720.415 ±
3255.500 ns/op
Thanks for pointing this out. I updated this PR, and the latest commit is
passing test tier1-4.
>
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v14]
On Wed, 5 Jun 2024 17:32:14 GMT, Adam Sotona wrote:
>> java.base java.lang.invoke package heavily uses ASM to generate lambdas and
>> method handles.
>>
>> This patch converts ASM calls to Classfile API.
>>
>> This PR is continuation of https://github.com/openjdk/jdk/pull/12945
>>
>> Any comments and suggestions are welcome.
>>
>> Please review.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 24 commits:
>
> - Merge branch 'master' into JDK-8294960-invoke
>
># Conflicts:
>#
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> - Merge branch 'master' into JDK-8294960-invoke
>
># Conflicts:
># src/java.base/share/classes/java/lang/classfile/Attributes.java
> - fixed CodeBuilder use in j.l.invoke
> - Merge branch 'master' into JDK-8294960-invoke
> - Merge pull request #4 from cl4es/boxunbox_holder
>
>Only create box/unbox MethodRefEntries on request
> - Only create box/unbox MethodRefEntries on request
> - Merge pull request #3 from cl4es/minor_init_improvements
>
>Reduce init overhead of InvokeBytecodeGenerator and StackMapGenerator
> - Remove stray MRE_LF_interpretWithArguments
> - Reduce init overhead of InvokeBytecodeGenerator and StackMapGenerator
> - Deferred initialization of attributes map by moving into a holder class
>
>Co-authored-by: Claes Redestad
> - ... and 14 more: https://git.openjdk.org/jdk/compare/f73922b2...9360b0eb
These can all use `ReferenceClassDescImpl::ofValidated(…)`:
src/java.base/share/classes/jdk/internal/classfile/impl/StackMapGenerator.java
line 1259:
> 1257: LONG_ARRAY_TYPE =
> referenceType(ClassDesc.ofDescriptor("[J")),
> 1258: DOUBLE_ARRAY_TYPE =
> referenceType(ClassDesc.ofDescriptor("[D")),
> 1259: FLOAT_ARRAY_TYPE =
> referenceType(ClassDesc.ofDescriptor("[F")),
Suggestion:
INT_ARRAY_TYPE =
referenceType(ReferenceClassDescImpl.ofValidated("[I")),
BOOLEAN_ARRAY_TYPE =
referenceType(ReferenceClassDescImpl.ofValidated("[Z")),
BYTE_ARRAY_TYPE =
referenceType(ReferenceClassDescImpl.ofValidated("[B")),
CHAR_ARRAY_TYPE =
referenceType(ReferenceClassDescImpl.ofValidated("[C")),
SHORT_ARRAY_TYPE =
referenceType(ReferenceClassDescImpl.ofValidated("[S")),
LONG_ARRAY_TYPE =
referenceType(ReferenceClassDescImpl.ofValidated("[J")),
DOUBLE_ARRAY_TYPE =
referenceType(ReferenceClassDescImpl.ofValidated("[D")),
FLOAT_ARRAY_TYPE =
referenceType(ReferenceClassDescImpl.ofValidated("[F")),
src/java.base/share/classes/jdk/internal/classfile/impl/StackMapGenerator.java
line 1325:
> 1323:
> 1324: private static final ClassDesc CD_Cloneable =
> ClassDesc.ofDescriptor("Ljava/lang/Cloneable;");
> 1325: private static final ClassDesc CD_Serializable =
> ClassDesc.ofDescriptor("Ljava/io/Serializable;");
Suggestion:
private static final ClassDesc CD_Cloneable =
ReferenceClassDescImpl.ofValidated("Ljava/lang/Cloneable;");
private static final ClassDesc CD_Serializable =
ReferenceClassDescImpl.ofValidated("Ljava/io/Serializable;");
-
PR Review: https://git.openjdk.org/jdk/pull/17108#pullrequestreview-2100944857
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1628870728
PR Review Comment: https://git.openjdk.org/jdk/pull/17108#discussion_r1628871015
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Mon, 3 Jun 2024 18:00:35 GMT, Sean Gwizdak wrote: >> Improve the speed of Method.hashCode by caching the hashcode on first use. >> I've seen an application where Method.hashCode is a hot path, and this is a >> fairly simple speedup. The memory overhead is low. >> >> This addresses issue >> [JDK-8332249](https://bugs.openjdk.org/browse/JDK-8332249). >> >> Before: >> >> Benchmark Mode Cnt Score Error Units >> # Intel Skylake >> MethodHashCode.benchmarkHashCode avgt5 1.843 ± 0.149 ns/op >> # Arm Neoverse N1 >> MethodHashCode.benchmarkHashCode avgt5 2.363 ± 0.091 ns/op >> >> >> >> After: >> >> >> Benchmark Mode Cnt Score Error Units >> # Intel Skylake >> MethodHashCode.benchmarkHashCode avgt5 1.121 ± 1.189 ns/op >> # Arm Neoverse N1 >> MethodHashCode.benchmarkHashCode avgt5 1.001 ± 0.001 ns/op > > Sean Gwizdak 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: > > - Remove trailing whitespace. > - Move hashCode benchmark into the newly created MethodBenchmark file > - Merge branch 'master' into method-hashcode-JDK-8332249 > - Remove changes to JavaDoc per guidance. > - Fix whitespace issues pointed by the bot > - Micro-optimize Method.hashCode src/java.base/share/classes/java/lang/reflect/Method.java line 392: > 390: .hashCode(); > 391: } > 392: return hc; The `hash` field should probably somehow be shared with the `Method.root` instance, so that it doesn’t need to be recomputed when different code gets a `Method` reference. - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1628860179
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Thu, 9 May 2024 03:55:15 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 > > Keep open. > > @JimLaskey it looks like you're the author of most of the surrounding code; > can you please confirm that there's a rounding error in the existing > implementation and that this PR will fix it? Or if the `git blame` logs are > giving you more credit than you're willing to take, then could you please > assign it to one ot the real authors or, if that's not possible, to someone > else who's in a relevant role and can capable of reviewing and eventually > merging this PR? Hello Chris @Pr0methean, please keep this open. Some of us are checking if we can find someone experienced with this code to provide reviews. - PR Comment: https://git.openjdk.org/jdk/pull/17703#issuecomment-2151485139
