Integrated: 8333303: Issues with DottedVersion class
On Thu, 30 May 2024 20:10:05 GMT, Alexey Semenyuk wrote: > - Get rid of DottedVersion#greedy field. > - Add support to save the unrecognizable remainder of the version string > (required to handle Wix4 version string). > - Implement DottedVersion#equals(). > - add DottedVersion#compareComponents(DottedVersion, DottedVersion) that > compares recognized components of the given DottedVersion instances. > - remove DottedVersion#compareTo(String) > > [Edit](https://bugs.openjdk.org/secure/EditComment!default.jspa?id=5130726=14677610) > [Delete](https://bugs.openjdk.org/secure/DeleteComment!default.jspa?id=5130726=14677610) This pull request has now been integrated. Changeset: 1b7d59f1 Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk/commit/1b7d59f171d0e2a3bdd234cddffac548b1f8ba57 Stats: 320 lines in 6 files changed: 195 ins; 58 del; 67 mod 803: Issues with DottedVersion class Reviewed-by: almatvee - PR: https://git.openjdk.org/jdk/pull/19488
Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option [v2]
On Thu, 30 May 2024 22:54:12 GMT, Alexander Matveev
wrote:
>> This issue is reproducible with and without `--mac-sign`. jpackage will
>> "_ad-hoc_" sign application bundle when `--mac-sign` is not specified by
>> using pseudo-identity "_-_". This is why jpackage tries to sign added files
>> and this is expected behavior by jpackage. "codesign" fails since added
>> content made application bundle structure invalid. There is nothing we can
>> do on jpackage side to sign such invalid bundles. As proposed solution we
>> will output possible reason for "codesign" failure if it fails and
>> `--app-content` was specified and possible solution. Proposed message: "One
>> of the possible reason for "codesign" failure is additional content provided
>> via "--app-content", which made application bundle structure invalid. Make
>> sure to provide additional content in a way it will not break application
>> bundle structure, otherwise add additional content as post-processing step."
>>
>> Example:
>> Lets assume we have "ReadMe" folder with "ReadMe.txt" file in it.
>> 1) jpackage --type app-image -n Test --app-content ReadMe/ReadMe.txt ...
>> "codesign" will fail with "In subcomponent: Test.app/Contents/ReadMe.txt".
>> This is expected and "ReadMe.txt" placed in "Test.app/Contents" which is
>> also expected.
>> 2) jpackage --type app-image -n Test --app-content ReadMe ...
>> Works and "ReadMe.txt" will be placed under "Test.app/Contents/ReadMe".
>>
>> Sample output before fix:
>>
>> Error: "codesign" failed with following output:
>> Test.app: replacing existing signature
>> Test.app: code object is not signed at all
>> In subcomponent: Test.app/Contents/ReadMe.txt
>>
>>
>> Sample output after fix:
>>
>> "codesign" failed and additional application content was supplied via the
>> "--app-content" parameter. Probably the additional content broke the
>> integrity of the application bundle and caused the failure. Ensure content
>> supplied via the "--app-content" parameter does not break the integrity of
>> the application bundle, or add it in the post-processing step.
>> Error: "codesign" failed with following output:
>> Test.app: replacing existing signature
>> Test.app: code object is not signed at all
>> In subcomponent: Test.app/Contents/ReadMe.txt
>
> Alexander Matveev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8332110: jpackage tries to sign added files without the --mac-sign option
> [v2]
Marked as reviewed by asemenyuk (Reviewer).
test/jdk/tools/jpackage/macosx/SigningOptionsTest.java line 97:
> 95: new String[]{"--app-content", TEST_DUKE},
> 96: null,
> 97: "\"codesign\" failure is additional content provided
> via \"--app-content\""},
Why is this not a `One of the possible reason for "{0}" failure is additional
content provided via "--app-content"`?
-
PR Review: https://git.openjdk.org/jdk/pull/19377#pullrequestreview-2088429523
PR Review Comment: https://git.openjdk.org/jdk/pull/19377#discussion_r1620824169
Integrated: 8333307: Don't suppress jpackage logging in tests when it is detecting packaging tools in the system
On Thu, 30 May 2024 20:26:02 GMT, Alexey Semenyuk wrote: > Change jpackage tests output from: > > > [19:16:29.586] Create: SimplePackageTest.test > [19:16:29.587] [ RUN ] SimplePackageTest.test > [19:16:29.663] TRACE: Bundler rpm supported > [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], > [verify-install], [finalize]] > > > to: > > > [19:16:29.586] Create: SimplePackageTest.test > [19:16:29.587] [ RUN ] SimplePackageTest.test > [19:16:29.621] Running dpkg > [19:16:29.633] Running dpkg-deb > [19:16:29.651] Running rpmbuild > [19:16:29.663] TRACE: Bundler rpm supported > [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], > [verify-install], [finalize]] This pull request has now been integrated. Changeset: e304a8ae Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk/commit/e304a8ae63fdec125e085bd5048d62cf555e2caa Stats: 29 lines in 1 file changed: 27 ins; 0 del; 2 mod 807: Don't suppress jpackage logging in tests when it is detecting packaging tools in the system Reviewed-by: almatvee - PR: https://git.openjdk.org/jdk/pull/19489
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v34]
On Fri, 31 May 2024 01:34:45 GMT, Brent Christian wrote: >> Classes in the `java.lang.ref` package would benefit from an update to bring >> the spec in line with how the VM already behaves. The changes would focus on >> _happens-before_ edges at some key points during reference processing. >> >> A couple key things we want to be able to say are: >> - `Reference.reachabilityFence(x)` _happens-before_ reference processing >> occurs for 'x'. >> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the >> registered cleaning action. >> >> This will bring Cleaner in line (or close) with the memory visibility >> guarantees made for finalizers in [JLS >> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): >> _"There is a happens-before edge from the end of a constructor of an object >> to the start of a finalizer (§12.6) for that object."_ > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > simplify some java.lang.ref package doc links Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16644#pullrequestreview-2089797089
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v34]
> Classes in the `java.lang.ref` package would benefit from an update to bring > the spec in line with how the VM already behaves. The changes would focus on > _happens-before_ edges at some key points during reference processing. > > A couple key things we want to be able to say are: > - `Reference.reachabilityFence(x)` _happens-before_ reference processing > occurs for 'x'. > - `Cleaner.register()` _happens-before_ the Cleaner thread runs the > registered cleaning action. > > This will bring Cleaner in line (or close) with the memory visibility > guarantees made for finalizers in [JLS > 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): > _"There is a happens-before edge from the end of a constructor of an object > to the start of a finalizer (§12.6) for that object."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: simplify some java.lang.ref package doc links - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/4d06..e02bf98e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=33 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=32-33 Stats: 6 lines in 2 files changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v32]
On Thu, 30 May 2024 05:14:30 GMT, Joe Darcy wrote: >> Brent Christian has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 69 commits: >> >> - Merge branch 'master' into refDocs2 >> - add link to Thread.isAlive() >> - small review tweaks; shorten MemoryConsistency links >> - small grammar fixes >> - new section for finalizer memviz >> - add memviz bullet for finalization >> - remove quotes from dequeue >> - package spec updates, mostly about reference queues and dequeueing >> - move reachability section before notification; update section header >> - add details on use of reference queues; swap reachability/memviz sections >> - ... and 59 more: https://git.openjdk.org/jdk/compare/7bf1989f...d7cbf0d3 > > src/java.base/share/classes/java/lang/ref/Reference.java line 491: > >> 489: * method is unsuccessful and returns false. >> 490: * >> 491: * Memory >> consistency effects: > > Note the `https://git.openjdk.org/jdk/pull/16644#discussion_r1621570783
Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]
Now that you mentioned it was added in Java 23, I went back to check
what change brought that in. Turns out it was me who introduced it, that
too as recently as a few months back. I too had forgotten about it :)
-Jaikiran
On 31/05/24 6:40 am, Lance Andersen wrote:
I was looking at jdk 22 and now see the npe was added to the constructor
specification earlier this year and I reviewed it
Sent from my iPad
On May 30, 2024, at 8:56 PM, Jaikiran Pai wrote:
On Thu, 30 May 2024 14:56:00 GMT, Lance Andersen wrote:
Jaikiran Pai has updated the pull request incrementally with one additional
commit since the last revision:
introduce a basic test for GZIPInputStream
src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 97:
95: */
96: private static Inflater createInflater(InputStream in, int size) {
97: Objects.requireNonNull(in);
I don't believe we currently indicate at at NPE will be thrown if the
InputStream is null so this would require a CSR if we need to document it
Hello Lance, both the constructors of `GZIPInputStream` already state that they
throw a NullPointerException when the input stream is null:
* @throwsNullPointerException if {@code in} is null
It was already being thrown from within the super constructor. To prevent
creation of a `Inflater` when `in` is null, I had to add this check here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19475#discussion_r1621554479
Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]
I was looking at jdk 22 and now see the npe was added to the constructor
specification earlier this year and I reviewed it
Sent from my iPad
> On May 30, 2024, at 8:56 PM, Jaikiran Pai wrote:
>
> On Thu, 30 May 2024 14:56:00 GMT, Lance Andersen wrote:
>
>>> Jaikiran Pai has updated the pull request incrementally with one additional
>>> commit since the last revision:
>>>
>>> introduce a basic test for GZIPInputStream
>>
>> src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 97:
>>
>>> 95: */
>>> 96: private static Inflater createInflater(InputStream in, int size) {
>>> 97: Objects.requireNonNull(in);
>>
>> I don't believe we currently indicate at at NPE will be thrown if the
>> InputStream is null so this would require a CSR if we need to document it
>
> Hello Lance, both the constructors of `GZIPInputStream` already state that
> they throw a NullPointerException when the input stream is null:
>
>
> * @throwsNullPointerException if {@code in} is null
>
> It was already being thrown from within the super constructor. To prevent
> creation of a `Inflater` when `in` is null, I had to add this check here.
>
> -
>
> PR Review Comment:
> https://git.openjdk.org/jdk/pull/19475#discussion_r1621554479
RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails
Can I please get a review of this test-only change which updates a couple of places in the test to use `try-with-resource`? As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should prevent leaking of resources in case there's any failure in the test. The test continues to pass with this change. - Commit messages: - 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails Changes: https://git.openjdk.org/jdk/pull/19492/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19492=00 Issue: https://bugs.openjdk.org/browse/JDK-7022325 Stats: 28 lines in 1 file changed: 2 ins; 4 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19492.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19492/head:pull/19492 PR: https://git.openjdk.org/jdk/pull/19492
Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]
On Thu, 30 May 2024 14:56:00 GMT, Lance Andersen wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> introduce a basic test for GZIPInputStream
>
> src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 97:
>
>> 95: */
>> 96: private static Inflater createInflater(InputStream in, int size) {
>> 97: Objects.requireNonNull(in);
>
> I don't believe we currently indicate at at NPE will be thrown if the
> InputStream is null so this would require a CSR if we need to document it
Hello Lance, both the constructors of `GZIPInputStream` already state that they
throw a NullPointerException when the input stream is null:
* @throwsNullPointerException if {@code in} is null
It was already being thrown from within the super constructor. To prevent
creation of a `Inflater` when `in` is null, I had to add this check here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19475#discussion_r1621554479
Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v9]
On Fri, 24 May 2024 16:17:41 GMT, Adam Sotona wrote: >> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only >> bytecode-level class verification. >> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with >> additional class checks inspired by >> `hotspot/share/classfile/classFileParser.cpp`. >> >> Also new `VerifierSelfTest::testParserVerifier` has been added. >> >> Please review. >> >> Thanks, >> 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 37 commits: > > - fixed ParserVerifier and VerifierSelfTest > - Merge branch 'master' into JDK-8320396-verifier-extension > - added verification of TypeAnnotation attributes > - added verification of SMT attribute > - added verification of module-related attributes > - applied the suggested changes > - applied the suggested changes > - fixed error thrown by VerifierImpl > - applied suggested changes > - Merge branch 'master' into JDK-8320396-verifier-extension > - ... and 27 more: https://git.openjdk.org/jdk/compare/cfdc64fc...b352b794 Great work! - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16809#pullrequestreview-2089511401
Re: RFR: 8333303: Issues with DottedVersion class
On Thu, 30 May 2024 20:10:05 GMT, Alexey Semenyuk wrote: > - Get rid of DottedVersion#greedy field. > - Add support to save the unrecognizable remainder of the version string > (required to handle Wix4 version string). > - Implement DottedVersion#equals(). > - add DottedVersion#compareComponents(DottedVersion, DottedVersion) that > compares recognized components of the given DottedVersion instances. > - remove DottedVersion#compareTo(String) > > [Edit](https://bugs.openjdk.org/secure/EditComment!default.jspa?id=5130726=14677610) > [Delete](https://bugs.openjdk.org/secure/DeleteComment!default.jspa?id=5130726=14677610) Looks good. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19488#pullrequestreview-2089507390
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v15]
> This set of changes address causes of poor utilization with small numbers of > cores due to overly aggressive contention avoidance. A number of further > adjustments were needed to still avoid most contention effects in deployments > with large numbers of cores Doug Lea has updated the pull request incrementally with one additional commit since the last revision: diagnostic - Changes: - all: https://git.openjdk.org/jdk/pull/19131/files - new: https://git.openjdk.org/jdk/pull/19131/files/47ce3643..becabd04 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19131=14 - incr: https://webrevs.openjdk.org/?repo=jdk=19131=13-14 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19131.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19131/head:pull/19131 PR: https://git.openjdk.org/jdk/pull/19131
Re: RFR: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
On Thu, 30 May 2024 17:04:44 GMT, Jorn Vernee wrote: > I suggest leaving a comment to document which cases this cache is trying to > address. I think that's mainly cases where the same ValueLayout is created > many times in different places (instead of the same layout being reused, for > which we already have a cache field on the layout instance itself). Not quite. Note that since the recent changes, the handle we get from `Utils::makeSegmentViewVarHandle` is no longer cached *directly* on the value layout (because that handle is "raw" and has no size checks). What we cache is the adapted version of the handle which has the correct checks. When we create a var handle from some other root layout, we don't cache the resulting var handle anywhere - in part because doing so would be relatively expensive (you'd need a map from PathElement[] to VarHandle, and that's just for `varHandle`), and in part because reusing chance are rather low. That said, in both cases, to construct the "final" var handle, we have to create some raw var handle using `Utils::makeSegmentViewVarHandle`, so caching these raw handles seems to add up. But yes, all the above rationale should be captured in a comment. - PR Comment: https://git.openjdk.org/jdk/pull/19485#issuecomment-2140981239
Re: RFR: 8333307: Don't suppress jpackage logging in tests when it is detecting packaging tools in the system
On Thu, 30 May 2024 20:26:02 GMT, Alexey Semenyuk wrote: > Change jpackage tests output from: > > > [19:16:29.586] Create: SimplePackageTest.test > [19:16:29.587] [ RUN ] SimplePackageTest.test > [19:16:29.663] TRACE: Bundler rpm supported > [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], > [verify-install], [finalize]] > > > to: > > > [19:16:29.586] Create: SimplePackageTest.test > [19:16:29.587] [ RUN ] SimplePackageTest.test > [19:16:29.621] Running dpkg > [19:16:29.633] Running dpkg-deb > [19:16:29.651] Running rpmbuild > [19:16:29.663] TRACE: Bundler rpm supported > [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], > [verify-install], [finalize]] Marked as reviewed by almatvee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19489#pullrequestreview-2089482543
Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option [v2]
On Thu, 30 May 2024 22:54:12 GMT, Alexander Matveev wrote: >> This issue is reproducible with and without `--mac-sign`. jpackage will >> "_ad-hoc_" sign application bundle when `--mac-sign` is not specified by >> using pseudo-identity "_-_". This is why jpackage tries to sign added files >> and this is expected behavior by jpackage. "codesign" fails since added >> content made application bundle structure invalid. There is nothing we can >> do on jpackage side to sign such invalid bundles. As proposed solution we >> will output possible reason for "codesign" failure if it fails and >> `--app-content` was specified and possible solution. Proposed message: "One >> of the possible reason for "codesign" failure is additional content provided >> via "--app-content", which made application bundle structure invalid. Make >> sure to provide additional content in a way it will not break application >> bundle structure, otherwise add additional content as post-processing step." >> >> Example: >> Lets assume we have "ReadMe" folder with "ReadMe.txt" file in it. >> 1) jpackage --type app-image -n Test --app-content ReadMe/ReadMe.txt ... >> "codesign" will fail with "In subcomponent: Test.app/Contents/ReadMe.txt". >> This is expected and "ReadMe.txt" placed in "Test.app/Contents" which is >> also expected. >> 2) jpackage --type app-image -n Test --app-content ReadMe ... >> Works and "ReadMe.txt" will be placed under "Test.app/Contents/ReadMe". >> >> Sample output before fix: >> >> Error: "codesign" failed with following output: >> Test.app: replacing existing signature >> Test.app: code object is not signed at all >> In subcomponent: Test.app/Contents/ReadMe.txt >> >> >> Sample output after fix: >> >> "codesign" failed and additional application content was supplied via the >> "--app-content" parameter. Probably the additional content broke the >> integrity of the application bundle and caused the failure. Ensure content >> supplied via the "--app-content" parameter does not break the integrity of >> the application bundle, or add it in the post-processing step. >> Error: "codesign" failed with following output: >> Test.app: replacing existing signature >> Test.app: code object is not signed at all >> In subcomponent: Test.app/Contents/ReadMe.txt > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8332110: jpackage tries to sign added files without the --mac-sign option > [v2] 8332110: jpackage tries to sign added files without the --mac-sign option [v2] - Updated error message as suggested. - PR Comment: https://git.openjdk.org/jdk/pull/19377#issuecomment-2140973262
Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option [v2]
> This issue is reproducible with and without `--mac-sign`. jpackage will > "_ad-hoc_" sign application bundle when `--mac-sign` is not specified by > using pseudo-identity "_-_". This is why jpackage tries to sign added files > and this is expected behavior by jpackage. "codesign" fails since added > content made application bundle structure invalid. There is nothing we can do > on jpackage side to sign such invalid bundles. As proposed solution we will > output possible reason for "codesign" failure if it fails and `--app-content` > was specified and possible solution. Proposed message: "One of the possible > reason for "codesign" failure is additional content provided via > "--app-content", which made application bundle structure invalid. Make sure > to provide additional content in a way it will not break application bundle > structure, otherwise add additional content as post-processing step." > > Example: > Lets assume we have "ReadMe" folder with "ReadMe.txt" file in it. > 1) jpackage --type app-image -n Test --app-content ReadMe/ReadMe.txt ... > "codesign" will fail with "In subcomponent: Test.app/Contents/ReadMe.txt". > This is expected and "ReadMe.txt" placed in "Test.app/Contents" which is also > expected. > 2) jpackage --type app-image -n Test --app-content ReadMe ... > Works and "ReadMe.txt" will be placed under "Test.app/Contents/ReadMe". > > Sample output before fix: > > Error: "codesign" failed with following output: > Test.app: replacing existing signature > Test.app: code object is not signed at all > In subcomponent: Test.app/Contents/ReadMe.txt > > > Sample output after fix: > > "codesign" failed and additional application content was supplied via the > "--app-content" parameter. Probably the additional content broke the > integrity of the application bundle and caused the failure. Ensure content > supplied via the "--app-content" parameter does not break the integrity of > the application bundle, or add it in the post-processing step. > Error: "codesign" failed with following output: > Test.app: replacing existing signature > Test.app: code object is not signed at all > In subcomponent: Test.app/Contents/ReadMe.txt Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8332110: jpackage tries to sign added files without the --mac-sign option [v2] - Changes: - all: https://git.openjdk.org/jdk/pull/19377/files - new: https://git.openjdk.org/jdk/pull/19377/files/0ad02cbb..7c1973ad Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19377=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19377=00-01 Stats: 9 lines in 6 files changed: 1 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19377.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19377/head:pull/19377 PR: https://git.openjdk.org/jdk/pull/19377
Re: RFR: 8333307: Don't suppress jpackage logging in tests when it is detecting packaging tools in the system
On Thu, 30 May 2024 20:26:02 GMT, Alexey Semenyuk wrote: > Change jpackage tests output from: > > > [19:16:29.586] Create: SimplePackageTest.test > [19:16:29.587] [ RUN ] SimplePackageTest.test > [19:16:29.663] TRACE: Bundler rpm supported > [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], > [verify-install], [finalize]] > > > to: > > > [19:16:29.586] Create: SimplePackageTest.test > [19:16:29.587] [ RUN ] SimplePackageTest.test > [19:16:29.621] Running dpkg > [19:16:29.633] Running dpkg-deb > [19:16:29.651] Running rpmbuild > [19:16:29.663] TRACE: Bundler rpm supported > [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], > [verify-install], [finalize]] Retargeted it to 23 - PR Comment: https://git.openjdk.org/jdk/pull/19489#issuecomment-2140965618
Re: RFR: 8333307: Don't suppress jpackage logging in tests when it is detecting packaging tools in the system
On Thu, 30 May 2024 20:26:02 GMT, Alexey Semenyuk wrote: > Change jpackage tests output from: > > > [19:16:29.586] Create: SimplePackageTest.test > [19:16:29.587] [ RUN ] SimplePackageTest.test > [19:16:29.663] TRACE: Bundler rpm supported > [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], > [verify-install], [finalize]] > > > to: > > > [19:16:29.586] Create: SimplePackageTest.test > [19:16:29.587] [ RUN ] SimplePackageTest.test > [19:16:29.621] Running dpkg > [19:16:29.633] Running dpkg-deb > [19:16:29.651] Running rpmbuild > [19:16:29.663] TRACE: Bundler rpm supported > [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], > [verify-install], [finalize]] Looks good, but I am confused why it is targeted to 24? I think you need to postpone integration until 24 or target it to 23. - PR Comment: https://git.openjdk.org/jdk/pull/19489#issuecomment-2140961328
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v33]
> Classes in the `java.lang.ref` package would benefit from an update to bring > the spec in line with how the VM already behaves. The changes would focus on > _happens-before_ edges at some key points during reference processing. > > A couple key things we want to be able to say are: > - `Reference.reachabilityFence(x)` _happens-before_ reference processing > occurs for 'x'. > - `Cleaner.register()` _happens-before_ the Cleaner thread runs the > registered cleaning action. > > This will bring Cleaner in line (or close) with the memory visibility > guarantees made for finalizers in [JLS > 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5): > _"There is a happens-before edge from the end of a constructor of an object > to the start of a finalizer (§12.6) for that object."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: convert sample to snippet; add 'JLS' label to jls links - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/d7cbf0d3..4d06 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=32 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=31-32 Stats: 7 lines in 2 files changed: 1 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v6]
On Thu, 30 May 2024 19:44:29 GMT, David M. Lloyd wrote: >> 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 10 additional >> commits since the last revision: >> >> - Add test to validate ClassReader behavior >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/entry-by-type >> - Null-check the entry class eagerly, avoid returning null or throwing IAE >> - Remove redundant import >> - Use switch >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/entry-by-type >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/entry-by-type >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/entry-by-type >> - Use constants, beautify code >> - 8332614: Type-checked ConstantPool.entryByIndex and >> ClassReader.readEntryOrNull > > src/java.base/share/classes/java/lang/classfile/ClassReader.java line 142: > >> 140: * @throws ConstantPoolException if the index is out of range of the >> 141: * constant pool size, or zero, or the entry is not of the >> given type >> 142: * @since 24 > > I just noticed that these are marked `@since 24`. Am I correct that this > should be `@since 23`? Thanks for pointing out, I was under the assumption that this patch might not come into 23. I will create an issue, and you can make a PR if you feel like contributing (doc-only changes can integrate before RDPs so no rush) - PR Review Comment: https://git.openjdk.org/jdk/pull/19330#discussion_r1621460803
Re: RFR: 8330182: Start of release updates for JDK 24 [v11]
> Get JDK 24 underway. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Temporarily problem list java.lang.instrument tests until jar generation is fixed. - Changes: - all: https://git.openjdk.org/jdk/pull/18787/files - new: https://git.openjdk.org/jdk/pull/18787/files/39a028c3..6a6499dd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18787=10 - incr: https://webrevs.openjdk.org/?repo=jdk=18787=09-10 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18787/head:pull/18787 PR: https://git.openjdk.org/jdk/pull/18787
RFR: 8333307: Don't suppress jpackage logging in tests when it is detecting packaging tools in the system
Change jpackage tests output from: [19:16:29.586] Create: SimplePackageTest.test [19:16:29.587] [ RUN ] SimplePackageTest.test [19:16:29.663] TRACE: Bundler rpm supported [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], [verify-install], [finalize]] to: [19:16:29.586] Create: SimplePackageTest.test [19:16:29.587] [ RUN ] SimplePackageTest.test [19:16:29.621] Running dpkg [19:16:29.633] Running dpkg-deb [19:16:29.651] Running rpmbuild [19:16:29.663] TRACE: Bundler rpm supported [19:16:29.674] TRACE: Actions: [[initialize], [create], [unpack], [verify-install], [finalize]] - Commit messages: - Don't suppress jpackage output when it is detecting what packaging tools available and what bundlers are supported. Changes: https://git.openjdk.org/jdk/pull/19489/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19489=00 Issue: https://bugs.openjdk.org/browse/JDK-807 Stats: 29 lines in 1 file changed: 27 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19489.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19489/head:pull/19489 PR: https://git.openjdk.org/jdk/pull/19489
Re: RFR: 8328995: Launcher can't open jar files where the offset of the manifest is >4GB [v6]
On Thu, 4 Apr 2024 16:57:40 GMT, Liam Miller-Cushon wrote: >> This change fixes a zip64 bug in the launcher that is prevent it from >> reading the manifest of jars where the 'relative offset of local header' >> field in the central directory entry is >4GB. As described in APPNOTE.TXT >> 4.5.3, the offset is too large to be stored in the central directory it is >> stored in a 'Zip64 Extended Information Extra Field'. > > Liam Miller-Cushon has updated the pull request incrementally with one > additional commit since the last revision: > > Move test to test/jdk/tools/launcher @bridgekeeper please keep this open - PR Comment: https://git.openjdk.org/jdk/pull/18479#issuecomment-2140810530
RFR: 8333303: Issues with DottedVersion class
- Get rid of DottedVersion#greedy field. - Add support to save the unrecognizable remainder of the version string (required to handle Wix4 version string). - Implement DottedVersion#equals(). - add DottedVersion#compareComponents(DottedVersion, DottedVersion) that compares recognized components of the given DottedVersion instances. - remove DottedVersion#compareTo(String) [Edit](https://bugs.openjdk.org/secure/EditComment!default.jspa?id=5130726=14677610) [Delete](https://bugs.openjdk.org/secure/DeleteComment!default.jspa?id=5130726=14677610) - Commit messages: - DottedVersion#compareTo() has been replaced with DottedVersion#compareComponents() - DottedVersion refactored. Old parsing code used "==" to test equality of two BiInteger objects and it didn't work right. When the bug was fixed app version check on Windows platform stopped working. It required a bit of work to get it working right. Changes: https://git.openjdk.org/jdk/pull/19488/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19488=00 Issue: https://bugs.openjdk.org/browse/JDK-803 Stats: 320 lines in 6 files changed: 195 ins; 58 del; 67 mod Patch: https://git.openjdk.org/jdk/pull/19488.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19488/head:pull/19488 PR: https://git.openjdk.org/jdk/pull/19488
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 13:49:18 GMT, jengebr wrote: > Improve `java/lang/reflect/Method.java` by eliminating needless cloning of > Class[0] instances. This cloning is intended to prevent callers from > changing array contents, but smany Methods have zero exceptions or zero > parameters, and returning the original `Class[0]` is sufficient. This is a bit of a behavioral change, but I think it falls under the threshold that would require CSR review. - PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2140795211
Re: RFR: 8333303: Issues with DottedVersion class
On Thu, 30 May 2024 20:10:05 GMT, Alexey Semenyuk wrote: > - Get rid of DottedVersion#greedy field. > - Add support to save the unrecognizable remainder of the version string > (required to handle Wix4 version string). > - Implement DottedVersion#equals(). > - add DottedVersion#compareComponents(DottedVersion, DottedVersion) that > compares recognized components of the given DottedVersion instances. > - remove DottedVersion#compareTo(String) > > [Edit](https://bugs.openjdk.org/secure/EditComment!default.jspa?id=5130726=14677610) > [Delete](https://bugs.openjdk.org/secure/DeleteComment!default.jspa?id=5130726=14677610) @sashamatveev please review - PR Comment: https://git.openjdk.org/jdk/pull/19488#issuecomment-2140794827
Integrated: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent
On Fri, 3 May 2024 08:47:03 GMT, Justin Lu wrote:
> Please review this PR which corrects an edge case bug for
> java.text.DecimalFormat that causes incorrect parsing results for strings
> with very large exponent values.
>
> When parsing values with large exponents, if the value of the exponent
> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
> mantissa. Both results are confusing and incorrect.
>
> For example,
>
>
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>
>
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
This pull request has now been integrated.
Changeset: ffb0867e
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/ffb0867e2c07b41cb7124e11fe6cf63d9471f0d2
Stats: 269 lines in 4 files changed: 254 ins; 2 del; 13 mod
8331485: Odd Results when Parsing Scientific Notation with Large Exponent
8331680: NumberFormat is missing some bad exponent strict parse cases
Reviewed-by: naoto
-
PR: https://git.openjdk.org/jdk/pull/19075
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v6]
On Fri, 17 May 2024 21:59:38 GMT, Justin Lu wrote:
>> Please review this PR which corrects an edge case bug for
>> java.text.DecimalFormat that causes incorrect parsing results for strings
>> with very large exponent values.
>>
>> When parsing values with large exponents, if the value of the exponent
>> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value
>> of the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
>> mantissa. Both results are confusing and incorrect.
>>
>> For example,
>>
>>
>> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
>> fmt.parse(".1E2147483648"); // returns 0.0
>> fmt.parse(".1E9223372036854775808"); // returns 0.1
>> // For comparison
>> Double.parseDouble(".1E2147483648"); // returns Infinity
>> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>>
>>
>> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
>
> Justin Lu has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - change impl to support whitebox test
> - adjust impl to increase accuracy when decimalAt/exponent value are both
> around Integer.MAX/MIN
Thanks for the reviews; tests pass after merge with latest master.
-
PR Comment: https://git.openjdk.org/jdk/pull/19075#issuecomment-2140780684
Re: RFR: 8333103: Re-examine the console provider loading [v2]
> There is an initialization code in `Console` class that searches for the > Console implementations. Refactoring the init code not to use lambda/stream > would reduce the (initial) number of loaded classes by about 100 for > java.base implementations. This would become relevant when the java.io.IO > (JEP 477) uses Console as the underlying framework. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Clarify the comment for multiple impls in the module case - Changes: - all: https://git.openjdk.org/jdk/pull/19467/files - new: https://git.openjdk.org/jdk/pull/19467/files/ae06bb2c..42a45323 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19467=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19467=00-01 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19467.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19467/head:pull/19467 PR: https://git.openjdk.org/jdk/pull/19467
Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v6]
On Wed, 29 May 2024 19:27:17 GMT, Chen Liang wrote: >> I propose to add type-checked ConstantPool.entryByIndex and >> ClassReader.readEntryOrNull taking an extra Class parameter, which throws >> ConstantPoolException instead of ClassCastException on type mismatch, which >> can happen to malformed ClassFiles. >> >> Requesting a review from @asotona. Thanks! >> >> Proposal on mailing list: >> https://mail.openjdk.org/pipermail/classfile-api-dev/2024-May/000512.html > > 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 10 additional commits since > the last revision: > > - Add test to validate ClassReader behavior > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/entry-by-type > - Null-check the entry class eagerly, avoid returning null or throwing IAE > - Remove redundant import > - Use switch > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/entry-by-type > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/entry-by-type > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/entry-by-type > - Use constants, beautify code > - 8332614: Type-checked ConstantPool.entryByIndex and > ClassReader.readEntryOrNull src/java.base/share/classes/java/lang/classfile/ClassReader.java line 142: > 140: * @throws ConstantPoolException if the index is out of range of the > 141: * constant pool size, or zero, or the entry is not of the > given type > 142: * @since 24 I just noticed that these are marked `@since 24`. Am I correct that this should be `@since 23`? - PR Review Comment: https://git.openjdk.org/jdk/pull/19330#discussion_r1621350942
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 13:49:18 GMT, jengebr wrote: > Improve `java/lang/reflect/Method.java` by eliminating needless cloning of > Class[0] instances. This cloning is intended to prevent callers from > changing array contents, but smany Methods have zero exceptions or zero > parameters, and returning the original `Class[0]` is sufficient. LGTM; the localized change is concise and clear as to why the clone is unnecessary. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2089235024
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 13:49:18 GMT, jengebr wrote: > Improve `java/lang/reflect/Method.java` by eliminating needless cloning of > Class[0] instances. This cloning is intended to prevent callers from > changing array contents, but smany Methods have zero exceptions or zero > parameters, and returning the original `Class[0]` is sufficient. This looks fine to me. Since this is a first-time contribution, someone else needs to take a look as well. You may want to put a simple microbenchmark somewhere here: https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/lang/reflect - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2089223720
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 14:32:23 GMT, Chen Liang wrote: >> Thanks. Unfortunately the variable `cloneArray` is actually a method >> parameter, and most of the callers pass in `false` - so using this utility >> method to control cloning would actually introduce cloning in several places >> where it is explicitly avoided. We may get a performance gain by modifying >> `Class.getInterfaces()` directly (the sole caller that passes `true`) then >> eliminating the parameter, then modifying each caller, etc. but that feels >> like a separate change inspired by this one. >> >> Thoughts? > > Why can't you do something like this: > > return cloneArray ? ReflectionFactory.copyClasses(interfaces) : interfaces; > > > Alternatively, your proposal is a good one too; we can rename this > `getInterfaces(boolean)` to `getInterfacesShared()` (like > `getSharedEnumConstants()`) and move this copy operation to `getInterfaces()` > as you suggest. I really see no reason to try and save on re-use of this one-line method for _everything_. In fact, I do not quite see a very compelling reason to even have the utility method. Sharing the code between `Method` and `Constructor` is clean enough and provides a good balance between reuse and cleanliness. Everything else can have the copy of the method definition, if needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1610112885
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 14:33:31 GMT, Aleksey Shipilev wrote:
>> Why can't you do something like this:
>>
>> return cloneArray ? ReflectionFactory.copyClasses(interfaces) : interfaces;
>>
>>
>> Alternatively, your proposal is a good one too; we can rename this
>> `getInterfaces(boolean)` to `getInterfacesShared()` (like
>> `getSharedEnumConstants()`) and move this copy operation to
>> `getInterfaces()` as you suggest.
>
> I really see no reason to try and save on re-use of this one-line method for
> _everything_. In fact, I do not quite see a very compelling reason to even
> have the utility method. Sharing the code between `Method` and `Constructor`
> is clean enough and provides a good balance between reuse and cleanliness.
> Everything else can have the copy of the method definition, if needed.
Alternatively, if a utility method is overkill, we can inline these to
`Executable`:
public Class[] getParameterTypes() {
var shared = getSharedParameterTypes();
return shared.length == 0 ? shared : shared.clone();
}
And the overrides in `Method` and `Constructor` will simply call super; the
declarations are kept to preserve the API documentation.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1610561896
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 09:39:50 GMT, Aleksey Shipilev wrote:
>> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of
>> Class[0] instances. This cloning is intended to prevent callers from
>> changing array contents, but smany Methods have zero exceptions or zero
>> parameters, and returning the original `Class[0]` is sufficient.
>
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
> 249:
>
>> 247: * the original can safely be reused.
>> 248: */
>> 249: public Class[] copyClasses(Class[] classes) {
>
> There is no need to make this utility method non-static. This would also
> obviate the need for having an instance of `ReflectionFactory` to access it.
>
> Additionally, at the risk of more bikeshedding, I think this utility method
> is better suited in `AccessibleObject` base class, with package-private
> visibility. Putting the util methods in `ReflectionFactory` erodes this
> comment a bit:
>
>
> The methods in this class are extremely unsafe and can cause
> subversion of both the language and the verifier. For this reason,
> they are all instance methods, and access to the constructor of
> this factory is guarded by a security check, in similar style to
> {@link jdk.internal.misc.Unsafe}.
Can't this be used by class cloning interfaces too?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1609710019
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 13:44:59 GMT, Chen Liang wrote: >> @liach I see such an opportunity in `Proxy.getProxyConstructor`, is that >> what you have in mind? > > No, I mean here: > https://github.com/openjdk/jdk/blob/4f1a10f84bcfadef263a0890b6834ccd3d5bb52f/src/java.base/share/classes/java/lang/Class.java#L1329 > > (That's also why I suggest putting the utiltiy method in ReflectionFactory > instead of AccessibleObject or Executable) > > `Proxy` is meaningless with an empty list of interfaces, as it's solely for > implementing interfaces and delegating method calls to the given > InvocationHandler. So I would ignore that scenario. Thanks. Unfortunately the variable `cloneArray` is actually a method parameter, and most of the callers pass in `false` - so using this utility method to control cloning would actually introduce cloning in several places where it is explicitly avoided. We may get a performance gain by modifying `Class.getInterfaces()` directly (the sole caller that passes `true`) then eliminating the parameter, then modifying each caller, etc. but that feels like a separate change inspired by this one. Thoughts? - PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1610079752
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 12:58:40 GMT, jengebr wrote: >> Can't this be used by class cloning interfaces too? > > @liach I see such an opportunity in `Proxy.getProxyConstructor`, is that what > you have in mind? No, I mean here: https://github.com/openjdk/jdk/blob/4f1a10f84bcfadef263a0890b6834ccd3d5bb52f/src/java.base/share/classes/java/lang/Class.java#L1329 (That's also why I suggest putting the utiltiy method in ReflectionFactory instead of AccessibleObject or Executable) `Proxy` is meaningless with an empty list of interfaces, as it's solely for implementing interfaces and delegating method calls to the given InvocationHandler. So I would ignore that scenario. - PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1609992490
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Thu, 23 May 2024 09:05:09 GMT, Aleksey Shipilev wrote:
>> Alternatively, if a utility method is overkill, we can inline these to
>> `Executable`:
>>
>> public Class[] getParameterTypes() {
>> var shared = getSharedParameterTypes();
>> return shared.length == 0 ? shared : shared.clone();
>> }
>>
>> And the overrides in `Method` and `Constructor` will simply call super; the
>> declarations are kept to preserve the API documentation.
>
> I had to read JLS to confirm that changing the `abstract` method to
> non-abstract one does not break compatibility.
>
> I am still thinking that we are overthinking this: the
> readability/maintainability benefits for introducing a one-liner utility
> method are slim at best. I believe we are spending the disproportionate time
> on this. So if we cannot agree where to put the utility method -- which
> implies there is no good place for it -- let's not do it at all. Inline the
> ternary selector in 4 affected places, and be done with it.
I've reverted to ternary logic.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1611628951
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 19:48:49 GMT, Chen Liang wrote:
>> I really see no reason to try and save on re-use of this one-line method for
>> _everything_. In fact, I do not quite see a very compelling reason to even
>> have the utility method. Sharing the code between `Method` and `Constructor`
>> is clean enough and provides a good balance between reuse and cleanliness.
>> Everything else can have the copy of the method definition, if needed.
>
> Alternatively, if a utility method is overkill, we can inline these to
> `Executable`:
>
> public Class[] getParameterTypes() {
> var shared = getSharedParameterTypes();
> return shared.length == 0 ? shared : shared.clone();
> }
>
> And the overrides in `Method` and `Constructor` will simply call super; the
> declarations are kept to preserve the API documentation.
I had to read JLS to confirm that changing the `abstract` method to
non-abstract one does not break compatibility.
I am still thinking that we are overthinking this: the
readability/maintainability benefits for introducing a one-liner utility method
are slim at best. I believe we are spending the disproportionate time on this.
So if we cannot agree where to put the utility method -- which implies there is
no good place for it -- let's not do it at all. Inline the ternary selector in
4 affected places, and be done with it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1611304563
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 14:24:40 GMT, jengebr wrote: >> No, I mean here: >> https://github.com/openjdk/jdk/blob/4f1a10f84bcfadef263a0890b6834ccd3d5bb52f/src/java.base/share/classes/java/lang/Class.java#L1329 >> >> (That's also why I suggest putting the utiltiy method in ReflectionFactory >> instead of AccessibleObject or Executable) >> >> `Proxy` is meaningless with an empty list of interfaces, as it's solely for >> implementing interfaces and delegating method calls to the given >> InvocationHandler. So I would ignore that scenario. > > Thanks. Unfortunately the variable `cloneArray` is actually a method > parameter, and most of the callers pass in `false` - so using this utility > method to control cloning would actually introduce cloning in several places > where it is explicitly avoided. We may get a performance gain by modifying > `Class.getInterfaces()` directly (the sole caller that passes `true`) then > eliminating the parameter, then modifying each caller, etc. but that feels > like a separate change inspired by this one. > > Thoughts? Why can't you do something like this: return cloneArray ? ReflectionFactory.copyClasses(interfaces) : interfaces; Alternatively, your proposal is a good one too; we can rename this `getInterfaces(boolean)` to `getInterfacesShared()` (like `getSharedEnumConstants()`) and move this copy operation to `getInterfaces()` as you suggest. - PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1610109601
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 10:27:16 GMT, Chen Liang wrote:
>> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
>> 249:
>>
>>> 247: * the original can safely be reused.
>>> 248: */
>>> 249: public Class[] copyClasses(Class[] classes) {
>>
>> There is no need to make this utility method non-static. This would also
>> obviate the need for having an instance of `ReflectionFactory` to access it.
>>
>> Additionally, at the risk of more bikeshedding, I think this utility method
>> is better suited in `AccessibleObject` base class, with package-private
>> visibility. Putting the util methods in `ReflectionFactory` erodes this
>> comment a bit:
>>
>>
>> The methods in this class are extremely unsafe and can cause
>> subversion of both the language and the verifier. For this reason,
>> they are all instance methods, and access to the constructor of
>> this factory is guarded by a security check, in similar style to
>> {@link jdk.internal.misc.Unsafe}.
>
> Can't this be used by class cloning interfaces too?
@liach I see such an opportunity in `Proxy.getProxyConstructor`, is that what
you have in mind?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1609907464
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Fri, 24 May 2024 21:59:58 GMT, Chen Liang wrote: > Can you update the ending copyright years from 2023 to 2024 on the 2nd line > of the 2 modified files? Done, thank you. - PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2135215681
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 09:44:18 GMT, Aleksey Shipilev wrote: > Please also enable GHA testing: https://github.com/jengebr/jdk/actions. You > may need to trigger the test run manually after this, since the PR hook have > already missed it. Go to "OpenJDK Sanity Checks" on the left on that page, > then select your branch on in the drop-down on the right, and trigger the run. Thank you for the detailed instructions, workflow is triggered. - PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2124771545
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 13:49:18 GMT, jengebr wrote:
> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of
> Class[0] instances. This cloning is intended to prevent callers from
> changing array contents, but smany Methods have zero exceptions or zero
> parameters, and returning the original `Class[0]` is sufficient.
Please also enable GHA testing: https://github.com/jengebr/jdk/actions. You may
need to trigger the test run manually after this, since the PR hook have
already missed it. Go to "OpenJDK Sanity Checks" on the left on that page, then
select your branch on in the drop-down on the right, and trigger the run.
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
249:
> 247: * the original can safely be reused.
> 248: */
> 249: public Class[] copyClasses(Class[] classes) {
There is no need to make this utility method non-static. This would also
obviate the need for having an instance of `ReflectionFactory` to access it.
Additionally, at the risk of more bikeshedding, I think this utility method is
better suited in `AccessibleObject` base class, with package-private
visibility. Putting the util methods in `ReflectionFactory` erodes this comment
a bit:
The methods in this class are extremely unsafe and can cause
subversion of both the language and the verifier. For this reason,
they are all instance methods, and access to the constructor of
this factory is guarded by a security check, in similar style to
{@link jdk.internal.misc.Unsafe}.
-
PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2124338419
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1609635793
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 14:07:29 GMT, jengebr wrote:
> Any suggestions?
I would recommend a new method `copyClasses`/`copyTypes` in
`jdk.internal.reflect.ReflectionFactory`, as it already has related
`copyConstructor` and `getExecutableSharedParameterTypes` methods.
Also, can you rename your PR's title to `8332586: Avoid cloning empty arrays in
java.lang.reflect.{Method,Constructor}` for the bot to handle this?
-
PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2122798223
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 13:49:18 GMT, jengebr wrote: > Improve `java/lang/reflect/Method.java` by eliminating needless cloning of > Class[0] instances. This cloning is intended to prevent callers from > changing array contents, but smany Methods have zero exceptions or zero > parameters, and returning the original `Class[0]` is sufficient. I think we should probably create a utility method dedicated to clone only mutable Class arrays. This utility can be used for the other methods in other reflection classes. Can you update the ending copyright years from 2023 to 2024 on the 2nd line of the 2 modified files? - PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2122710633 PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2130420945
RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
Improve `java/lang/reflect/Method.java` by eliminating needless cloning of Class[0] instances. This cloning is intended to prevent callers from changing array contents, but smany Methods have zero exceptions or zero parameters, and returning the original `Class[0]` is sufficient. - Commit messages: - Merge branch 'openjdk:master' into Method_ArrayCloning - Updating copyright year - Merge branch 'openjdk:master' into Method_ArrayCloning - Merge branch 'openjdk:master' into Method_ArrayCloning - Reverting to expressions rather than utility method - Moving utility method to AccessibleObject - Fixing whitespace - Extracted logic to utility method - Expanding no-cloning coverage to Constructor - Reducing array allocation rate by eliminating cloned Class[0] Changes: https://git.openjdk.org/jdk/pull/19327/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19327=00 Issue: https://bugs.openjdk.org/browse/JDK-8332586 Stats: 7 lines in 2 files changed: 0 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19327.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19327/head:pull/19327 PR: https://git.openjdk.org/jdk/pull/19327
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 13:49:18 GMT, jengebr wrote: > Improve `java/lang/reflect/Method.java` by eliminating needless cloning of > Class[0] instances. This cloning is intended to prevent callers from > changing array contents, but smany Methods have zero exceptions or zero > parameters, and returning the original `Class[0]` is sufficient. Thanks for the suggestion! I'll definitely add coverage of the other instances in this package, but I don't see an obvious place to put a utility method. Any suggestions? New method added, PR renamed. OCA process is progressing, once finalized I'll rerun checks and publish the PR. OCA completed, ready for review. - PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2122723979 PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2122984663 PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2127017202 PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2140731037
RFR: 8333301: Remove static builds using --enable-static-build
The original way of building static libraries in the JDK was to use the configure argument --enable-static-build, which set the value of the make variable STATIC_BUILD. (Note that this is not the same as the source code definition STATIC_BUILD, which will be set by other means as well.) This method only ever worked on macOS, and has long since stopped working. It was originally introduced for the Mobile Project, but I've now learn that not even they use it anymore. It just adds clutter to the build system, and should be removed. - Commit messages: - 801: Remove static builds using --enable-static-build Changes: https://git.openjdk.org/jdk/pull/19487/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19487=00 Issue: https://bugs.openjdk.org/browse/JDK-801 Stats: 218 lines in 14 files changed: 4 ins; 206 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19487.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19487/head:pull/19487 PR: https://git.openjdk.org/jdk/pull/19487
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v14]
> This set of changes address causes of poor utilization with small numbers of > cores due to overly aggressive contention avoidance. A number of further > adjustments were needed to still avoid most contention effects in deployments > with large numbers of cores Doug Lea has updated the pull request incrementally with one additional commit since the last revision: Reinstate quiescence signals - Changes: - all: https://git.openjdk.org/jdk/pull/19131/files - new: https://git.openjdk.org/jdk/pull/19131/files/398e5c6a..47ce3643 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19131=13 - incr: https://webrevs.openjdk.org/?repo=jdk=19131=12-13 Stats: 15 lines in 1 file changed: 4 ins; 1 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/19131.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19131/head:pull/19131 PR: https://git.openjdk.org/jdk/pull/19131
Re: RFR: 8330182: Start of release updates for JDK 24 [v9]
On Thu, 30 May 2024 18:39:19 GMT, Chen Liang wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update symbol files for JDK 23 build 25. > > src/jdk.compiler/share/data/symbols/jdk.incubator.foreign-N.sym.txt line 1: > >> 1: # > > Just curious, does CreateSymbols not track module migrations, now that > jdk.incubator.foreign is completely merged into java.base? When writing these symbol files, CreateSymbols does not keep track where a given package was in a given version, and puts packages to files based on the first version where the package was first introduced. Technically, it should not matter, as the assignment of classes/packages to modules is not based on the file from which the description of the class was read. So, so far, it didn't seem necessary to keep track of package movement for writing of these files. But it is feasible to do so, if the need arises. (When writing ct.sym, it keeps track of the exact package module for every version, of course.) - PR Review Comment: https://git.openjdk.org/jdk/pull/18787#discussion_r1621320340
Re: RFR: 8333103: Re-examine the console provider loading
On Thu, 30 May 2024 18:38:59 GMT, Pavel Rappo wrote: >> In fact, this started simply for incorporating JLine implementation into >> Console, and using ServiceLoader was a mere means to load its impl as it >> resides outside the java.base module. So yes, module and its console >> implementation is 1:1, which is reflected by the system property >> `jdk.console` that takes the module name. So that `break;` effectively >> shortcut the unnecessary looping (I don't think it would happen btw). >> That said, I think it needs to be described in the comment above that piece >> of code. I will add it shortly. > > So, it's the previous (stream) version that was more permissive and > inadvertently so, yes? Yes, correct. If hypothetically Jline provided two impls, it were not specified which impl was used. Now we only use the first one found. - PR Review Comment: https://git.openjdk.org/jdk/pull/19467#discussion_r1621284108
Re: RFR: 8333103: Re-examine the console provider loading
On Thu, 30 May 2024 18:10:14 GMT, Naoto Sato wrote: >> Claes has described the issue well. Like I said, `break` short-circuits the >> search. If a module can provide more than one console provider, the >> suggested stream-less replacement is **not** equivalent. >> >> If a module can provide more than one console provider, not only the code >> needs to be fixed, but a relevant test should be added too. The test should >> be in this PR, but tagged with the initial bug, [8295803], not this >> (performance) bug. >> >> [8295803]: https://bugs.openjdk.org/browse/JDK-8295803 > > In fact, this started simply for incorporating JLine implementation into > Console, and using ServiceLoader was a mere means to load its impl as it > resides outside the java.base module. So yes, module and its console > implementation is 1:1, which is reflected by the system property > `jdk.console` that takes the module name. So that `break;` effectively > shortcut the unnecessary looping (I don't think it would happen btw). > That said, I think it needs to be described in the comment above that piece > of code. I will add it shortly. So, it's the previous (stream) version that was more permissive and inadvertently so, yes? - PR Review Comment: https://git.openjdk.org/jdk/pull/19467#discussion_r1621273970
Re: RFR: 8330182: Start of release updates for JDK 24 [v9]
On Thu, 30 May 2024 16:44:21 GMT, Joe Darcy wrote: >> Get JDK 24 underway. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Update symbol files for JDK 23 build 25. src/jdk.compiler/share/data/symbols/jdk.incubator.foreign-N.sym.txt line 1: > 1: # Just curious, does CreateSymbols not track module migrations, now that jdk.incubator.foreign is completely merged into java.base? - PR Review Comment: https://git.openjdk.org/jdk/pull/18787#discussion_r1621274290
Re: RFR: 8333103: Re-examine the console provider loading
On Thu, 30 May 2024 08:23:25 GMT, Pavel Rappo wrote: >> It's only semantically the same if we assume a module can only provide a >> single `JdkConsoleProvider`, no? The `break;` disallows multiple providers >> (for disjoint sets of charsets) in a single module. > > Claes has described the issue well. Like I said, `break` short-circuits the > search. If a module can provide more than one console provider, the suggested > stream-less replacement is **not** equivalent. > > If a module can provide more than one console provider, not only the code > needs to be fixed, but a relevant test should be added too. The test should > be in this PR, but tagged with the initial bug, [8295803], not this > (performance) bug. > > [8295803]: https://bugs.openjdk.org/browse/JDK-8295803 In fact, this started simply for incorporating JLine implementation into Console, and using ServiceLoader was a mere means to load its impl as it resides outside the java.base module. So yes, module and its console implementation is 1:1, which is reflected by the system property `jdk.console` that takes the module name. So that `break;` effectively shortcut the unnecessary looping (I don't think it would happen btw). That said, I think it needs to be described in the comment above that piece of code. I will add it shortly. - PR Review Comment: https://git.openjdk.org/jdk/pull/19467#discussion_r1621231072
Re: RFR: 8330182: Start of release updates for JDK 24 [v10]
On Thu, 30 May 2024 17:11:31 GMT, Joe Darcy wrote: >> Get JDK 24 underway. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 18 commits: > > - Merge branch 'master' into JDK-8330188 > - Update symbol files for JDK 23 build 25. > - Correct release year. > - Merge branch 'master' into JDK-8330188 > - Add symbol files current with JDK 23 build 24. > - Merge branch 'master' into JDK-8330188 > - Merge branch 'master' into JDK-8330188 > - Fix-up test. > - Merge branch 'master' into JDK-8330188 > - Adjust test for deprecated options. > - ... and 8 more: https://git.openjdk.org/jdk/compare/32636dcc...39a028c3 Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18787#pullrequestreview-2089018766
Re: RFR: 8330182: Start of release updates for JDK 24 [v10]
> Get JDK 24 underway. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 18 commits: - Merge branch 'master' into JDK-8330188 - Update symbol files for JDK 23 build 25. - Correct release year. - Merge branch 'master' into JDK-8330188 - Add symbol files current with JDK 23 build 24. - Merge branch 'master' into JDK-8330188 - Merge branch 'master' into JDK-8330188 - Fix-up test. - Merge branch 'master' into JDK-8330188 - Adjust test for deprecated options. - ... and 8 more: https://git.openjdk.org/jdk/compare/32636dcc...39a028c3 - Changes: https://git.openjdk.org/jdk/pull/18787/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18787=09 Stats: 2082 lines in 50 files changed: 2019 ins; 7 del; 56 mod Patch: https://git.openjdk.org/jdk/pull/18787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18787/head:pull/18787 PR: https://git.openjdk.org/jdk/pull/18787
Re: RFR: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
On Thu, 30 May 2024 16:15:22 GMT, Maurizio Cimadamore wrote: > This PR restores a var handle cache in `Utils::makeSegmentViewVarHandle`. The > cache was moved to `ValueLayouts::varHandle` as part of > [pull/19251](https://git.openjdk.org/jdk/pull/19251), on the basis that we > want to optimize the common case like: > > > ValueLayout layout = ... > layout.varHandle().get(...) > > > And that caching more complex var handles didn't seem to add value, given > that, for these var handles, the logic in `LayoutPath` needs to adapt the > returned var handle anyways. > > But, `TestAccessModes` revealed a different picture - w/o any cache in > `Utils` the test end up allocating 8963 var handle instances (instead of just > 4), in each of the 4 runs the test includes. While this is admittedly a > stress test, it seems nice to restore the level of sharing we had before > [pull/19251](https://git.openjdk.org/jdk/pull/19251). I suggest leaving a comment to document which cases this cache is trying to address. I think that's mainly cases where the same ValueLayout is created many times in different places (instead of the same layout being reused, for which we already have a cache field on the layout instance itself). - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19485#pullrequestreview-201021
Re: RFR: 8330182: Start of release updates for JDK 24 [v9]
> Get JDK 24 underway. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update symbol files for JDK 23 build 25. - Changes: - all: https://git.openjdk.org/jdk/pull/18787/files - new: https://git.openjdk.org/jdk/pull/18787/files/45e1c040..0d15aaf4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18787=08 - incr: https://webrevs.openjdk.org/?repo=jdk=18787=07-08 Stats: 341 lines in 4 files changed: 340 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18787/head:pull/18787 PR: https://git.openjdk.org/jdk/pull/18787
Re: RFR: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
On Thu, 30 May 2024 16:15:22 GMT, Maurizio Cimadamore wrote: > This PR restores a var handle cache in `Utils::makeSegmentViewVarHandle`. The > cache was moved to `ValueLayouts::varHandle` as part of > [pull/19251](https://git.openjdk.org/jdk/pull/19251), on the basis that we > want to optimize the common case like: > > > ValueLayout layout = ... > layout.varHandle().get(...) > > > And that caching more complex var handles didn't seem to add value, given > that, for these var handles, the logic in `LayoutPath` needs to adapt the > returned var handle anyways. > > But, `TestAccessModes` revealed a different picture - w/o any cache in > `Utils` the test end up allocating 8963 var handle instances (instead of just > 4), in each of the 4 runs the test includes. While this is admittedly a > stress test, it seems nice to restore the level of sharing we had before > [pull/19251](https://git.openjdk.org/jdk/pull/19251). A different approach could be to find a way to add a per-layout cache. E.g. if the root layout is a group layout, that layout could have a cache for all the different var handles generated from it. The key would be the path element used to retrieve the var handle. - PR Comment: https://git.openjdk.org/jdk/pull/19485#issuecomment-2140148973
RFR: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
This PR restores a var handle cache in `Utils::makeSegmentViewVarHandle`. The cache was moved to `ValueLayouts::varHandle` as part of [pull/19251](https://git.openjdk.org/jdk/pull/19251), on the basis that we want to optimize the common case like: ValueLayout layout = ... layout.varHandle().get(...) And that caching more complex var handles didn't seem to add value, given that, for these var handles, the logic in `LayoutPath` needs to adapt the returned var handle anyways. But, `TestAccessModes` revealed a different picture - w/o any cache in `Utils` the test end up allocating 8963 var handle instances (instead of just 4), in each of the 4 runs the test includes. While this is admittedly a stress test, it seems nice to restore the level of sharing we had before [pull/19251](https://git.openjdk.org/jdk/pull/19251). - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk/pull/19485/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19485=00 Issue: https://bugs.openjdk.org/browse/JDK-8333236 Stats: 11 lines in 1 file changed: 10 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19485.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19485/head:pull/19485 PR: https://git.openjdk.org/jdk/pull/19485
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 30 May 2024 16:16:59 GMT, Scott Gibbons wrote: >> I agree with @eme64 to postpone the integration after JDK 23 is forked in >> one week. It is not about how you confident with code. It is size of code. I >> did only limited (tier1-4) testing in our infra which did not cover all our >> testing configuration. > > @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. - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2140136262
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 30 May 2024 16:10:53 GMT, Vladimir Kozlov wrote: >> About the fuzzer: we have it in our closed tests. But I think it comes from >> this: https://github.com/shipilev/JavaFuzzer > > I agree with @eme64 to postpone the integration after JDK 23 is forked in one > week. It is not about how you confident with code. It is size of code. I did > only limited (tier1-4) testing in our infra which did not cover all our > testing configuration. @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? - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2140124882
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v51]
On Thu, 30 May 2024 16:03:29 GMT, Emanuel Peter wrote:
>> Scott Gibbons has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix bug number in tests
>
> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2023, 2024 Intel Corporation. All rights reserved.
>
> Is the 2023 year intentional? I don't know your policy, so you can just
> ignore this ;)
I started this in November :-)
> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 334:
>
>> 332: // NUMBER_OF_CASES (currently 10) needle sizes for both big and
>> small. There are special
>> 333: // routines for handling needle sizes > NUMBER_OF_CASES
>> (L_{big,small}CaseDefault). These
>> 334: // cases use C@'s arrays_equals() to compare the needle to the
>> haystack. The small cases
>
> Suggestion:
>
> // cases use C2's arrays_equals() to compare the needle to the haystack.
> The small cases
>
> Randomly spotted this.
Fixed.
> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 773:
>
>> 771: // jae done
>> 772: //
>> 773: // Final index of start of needle @((16 - (ndlLen %16)) & 0xf) << 1
>
> What is the meaning of the `@`? Maybe `at`. I'd use the same consistently
Changed to "at".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621034441
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621034583
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621034821
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 30 May 2024 15:16:34 GMT, Emanuel Peter wrote: >> Scott Gibbons has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Stupid EOL at end >> - Add @test block; fix test indentation > > About the fuzzer: we have it in our closed tests. But I think it comes from > this: https://github.com/shipilev/JavaFuzzer I agree with @eme64 to postpone the integration after JDK 23 is forked in one week. It is not about how you confident with code. It is size of code. I did only limited (tier1-4) testing in our infra which did not cover all our testing configuration. - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2140103757
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v52]
> 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: > > > BenchmarkScore > Latest > StringIndexOf.advancedWithMediumSub 343.573 317.934 > 0.925375393x > StringIndexOf.advancedWithShortSub1 1039.081 1053.96 > 1.014319384x > StringIndexOf.advancedWithShortSub2 55.828110.541 > 1.980027943x > StringIndexOf.constantPattern 9.361 11.906 > 1.271872663x > StringIndexOf.searchCharLongSuccess 4.216 4.218 > 1.000474383x > StringIndexOf.searchCharMediumSuccess 3.133 3.216 > 1.02649218x > StringIndexOf.searchCharShortSuccess 3.763.761 > 1.000265957x > StringIndexOf.success 9.186 9.713 > 1.057369911x > StringIndexOf.successBig14.34146.343 > 3.231504079x > StringIndexOfChar.latin1_AVX2_String6220.918 12154.52 > 1.953814533x > StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 > 1.006629895x > StringIndexOfChar.latin1_SSE4_String6978.854 6818.689 > 0.977049957x > StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 > 0.967675646x > StringIndexOfChar.latin1_Short_String 7132.541 6863.359 > 0.962260014x > StringIndexOfChar.latin1_Short_char 16013.389 16162.437 > 1.009307711x > StringIndexOfChar.latin1_mixed_String 7386.12314771.622 > 1.15517x > StringIndexOfChar.latin1_mixed_char 9901.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 - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/6eae46e5..f432320f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=51 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=50-51 Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753 PR: https://git.openjdk.org/jdk/pull/16753
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v51]
On Thu, 30 May 2024 15:48:50 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 bug number in tests
Ok, now it is good for me. But I would definately wait with integration for
after the fork next week.
src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 2:
> 1: /*
> 2: * Copyright (c) 2023, 2024 Intel Corporation. All rights reserved.
Is the 2023 year intentional? I don't know your policy, so you can just ignore
this ;)
src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 334:
> 332: // NUMBER_OF_CASES (currently 10) needle sizes for both big and small.
> There are special
> 333: // routines for handling needle sizes > NUMBER_OF_CASES
> (L_{big,small}CaseDefault). These
> 334: // cases use C@'s arrays_equals() to compare the needle to the
> haystack. The small cases
Suggestion:
// cases use C2's arrays_equals() to compare the needle to the haystack. The
small cases
Randomly spotted this.
src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 773:
> 771: // jae done
> 772: //
> 773: // Final index of start of needle @((16 - (ndlLen %16)) & 0xf) << 1
What is the meaning of the `@`? Maybe `at`. I'd use the same consistently
-
Marked as reviewed by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16753#pullrequestreview-2088739965
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621015782
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621017548
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1621019611
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v50]
On Thu, 30 May 2024 15:33:27 GMT, Emanuel Peter wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review comments > > test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 33: > >> 31: >> 32: /* @test >> 33: * @bug 4162796 4162796 8320448 > > Suggestion: > > * @bug 8320448 Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620988308
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v50]
On Thu, 30 May 2024 15:34:17 GMT, Emanuel Peter wrote: >> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 25: >> >>> 23: >>> 24: /* @test >>> 25: * @bug 4162796 4162796 8320448 >> >> Suggestion: >> >> * @bug 8320448 > > As I said above: I never add old bug numbers to new tests. But here it is > even duplicated ;) The file I used as baseline for this `test/jdk/java/lang/StringBuffer/IndexOf.java` has the bug number listed twice (copy/paste). I'll remove it from here, but leave it in the original unless requested to change it. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620985844
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v51]
> 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: > > > BenchmarkScore > Latest > StringIndexOf.advancedWithMediumSub 343.573 317.934 > 0.925375393x > StringIndexOf.advancedWithShortSub1 1039.081 1053.96 > 1.014319384x > StringIndexOf.advancedWithShortSub2 55.828110.541 > 1.980027943x > StringIndexOf.constantPattern 9.361 11.906 > 1.271872663x > StringIndexOf.searchCharLongSuccess 4.216 4.218 > 1.000474383x > StringIndexOf.searchCharMediumSuccess 3.133 3.216 > 1.02649218x > StringIndexOf.searchCharShortSuccess 3.763.761 > 1.000265957x > StringIndexOf.success 9.186 9.713 > 1.057369911x > StringIndexOf.successBig14.34146.343 > 3.231504079x > StringIndexOfChar.latin1_AVX2_String6220.918 12154.52 > 1.953814533x > StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 > 1.006629895x > StringIndexOfChar.latin1_SSE4_String6978.854 6818.689 > 0.977049957x > StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 > 0.967675646x > StringIndexOfChar.latin1_Short_String 7132.541 6863.359 > 0.962260014x > StringIndexOfChar.latin1_Short_char 16013.389 16162.437 > 1.009307711x > StringIndexOfChar.latin1_mixed_String 7386.12314771.622 > 1.15517x > StringIndexOfChar.latin1_mixed_char 9901.671 9782.245 > 0.987938803 Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Fix bug number in tests - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/57e115d7..6eae46e5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=50 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=49-50 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753 PR: https://git.openjdk.org/jdk/pull/16753
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v13]
> This set of changes address causes of poor utilization with small numbers of > cores due to overly aggressive contention avoidance. A number of further > adjustments were needed to still avoid most contention effects in deployments > with large numbers of cores Doug Lea has updated the pull request incrementally with one additional commit since the last revision: reshadow; avoid test loop bleed - Changes: - all: https://git.openjdk.org/jdk/pull/19131/files - new: https://git.openjdk.org/jdk/pull/19131/files/b13d8ee1..398e5c6a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19131=12 - incr: https://webrevs.openjdk.org/?repo=jdk=19131=11-12 Stats: 23 lines in 2 files changed: 10 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/19131.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19131/head:pull/19131 PR: https://git.openjdk.org/jdk/pull/19131
Integrated: 8331189: Implementation of Scoped Values (Third Preview)
On Wed, 8 May 2024 09:40:38 GMT, Alan Bateman wrote: > JEP 481 proposes Scoped Values to continue to preview in JDK 23 with one > change. The type of the operation parameter of the callWhere method is > changed to a new functional interface to avoid having the API throw > Exception. With that change, the getWhere (and the corresponding method on > Carrier) are no longer needed. The functional interface is not proposed for > j.u.function at this time. This pull request has now been integrated. Changeset: 70715423 Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/707154235b29bebc4c3fdb797e24acd8e9f6916a Stats: 298 lines in 7 files changed: 31 ins; 203 del; 64 mod 8331189: Implementation of Scoped Values (Third Preview) Reviewed-by: aph, jpai, mcimadamore - PR: https://git.openjdk.org/jdk/pull/19136
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v50]
On Thu, 30 May 2024 15:33:16 GMT, Emanuel Peter wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review comments > > test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 25: > >> 23: >> 24: /* @test >> 25: * @bug 4162796 4162796 8320448 > > Suggestion: > > * @bug 8320448 As I said above: I never add old bug numbers to new tests. But here it is even duplicated ;) - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620966568
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v50]
On Thu, 30 May 2024 15:30: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: > > Review comments test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 25: > 23: > 24: /* @test > 25: * @bug 4162796 4162796 8320448 Suggestion: * @bug 8320448 test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 33: > 31: > 32: /* @test > 33: * @bug 4162796 4162796 8320448 Suggestion: * @bug 8320448 - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620964138 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620964720
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 30 May 2024 15:30:26 GMT, Emanuel Peter wrote: >> Done. > >> Done. > > I still see the numbers `4162796 4162796`. I'm not sure if this bug number is > relevant. But certainly it should only be mentioned once ;) I never add old bug number to new tests... - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620963284
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 30 May 2024 15:21:10 GMT, Scott Gibbons wrote: > Done. I still see the numbers `4162796 4162796`. I'm not sure if this bug number is relevant. But certainly it should only be mentioned once ;) - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620960158
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v50]
> 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: > > > BenchmarkScore > Latest > StringIndexOf.advancedWithMediumSub 343.573 317.934 > 0.925375393x > StringIndexOf.advancedWithShortSub1 1039.081 1053.96 > 1.014319384x > StringIndexOf.advancedWithShortSub2 55.828110.541 > 1.980027943x > StringIndexOf.constantPattern 9.361 11.906 > 1.271872663x > StringIndexOf.searchCharLongSuccess 4.216 4.218 > 1.000474383x > StringIndexOf.searchCharMediumSuccess 3.133 3.216 > 1.02649218x > StringIndexOf.searchCharShortSuccess 3.763.761 > 1.000265957x > StringIndexOf.success 9.186 9.713 > 1.057369911x > StringIndexOf.successBig14.34146.343 > 3.231504079x > StringIndexOfChar.latin1_AVX2_String6220.918 12154.52 > 1.953814533x > StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 > 1.006629895x > StringIndexOfChar.latin1_SSE4_String6978.854 6818.689 > 0.977049957x > StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 > 0.967675646x > StringIndexOfChar.latin1_Short_String 7132.541 6863.359 > 0.962260014x > StringIndexOfChar.latin1_Short_char 16013.389 16162.437 > 1.009307711x > StringIndexOfChar.latin1_mixed_String 7386.12314771.622 > 1.15517x > StringIndexOfChar.latin1_mixed_char 9901.671 9782.245 > 0.987938803 Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Review comments - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/3e150fe3..57e115d7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=49 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=48-49 Stats: 6 lines in 2 files changed: 3 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753 PR: https://git.openjdk.org/jdk/pull/16753
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 30 May 2024 13:50:01 GMT, Emanuel Peter wrote: >> Scott Gibbons has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Stupid EOL at end >> - Add @test block; fix test indentation > > test/jdk/java/lang/String/IndexOf.java line 25: > >> 23: >> 24: /* >> 25: * @test > > You should add the `@bug 8320448` for all runs. Done. > test/jdk/java/lang/String/IndexOf.java line 27: > >> 25: * @test >> 26: * @summary test String indexOf() intrinsic >> 27: * @run main/othervm IndexOf > > Suggestion: > > * @run main IndexOf > > You do not need a new VM if you have no arguments ;) Done. > test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 25: > >> 23: >> 24: /* @test >> 25: * @bug 4162796 4162796 > > You need to fix the bug numbers. Done. > test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 27: > >> 25: * @bug 4162796 4162796 >> 26: * @summary Test indexOf and lastIndexOf >> 27: * @run main/othervm -Xbatch -XX:-TieredCompilation >> -XX:CompileCommand=dontinline,ECoreIndexOf.indexOfKernel ECoreIndexOf > > I would also add a line without `-XX:-TieredCompilation`, then C1 can be > tested with this too Done. > test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 32: > >> 30: >> 31: /* @test >> 32: * @bug 4162796 4162796 > > Here too Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620951690 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620949315 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620945040 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620947641 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620945484
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 30 May 2024 13:19:57 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 two additional > commits since the last revision: > > - Stupid EOL at end > - Add @test block; fix test indentation About the fuzzer: we have it in our closed tests. But I think it comes from this: https://github.com/shipilev/JavaFuzzer - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2139901477
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v48]
On Thu, 30 May 2024 14:57:35 GMT, Scott Gibbons wrote: >>> Control question: Are we confident with this potentially going into JDK 23 >>> or should we rather postpone to JDK 24? The fork is next week. >> >> I would hold off. @asgibbons it may pass our tests, and your extensive >> testing. But you never know what the fuzzer can find over a few weeks once >> it runs with your changes. I have made that experience many times. Let's >> just give it a few days, and then we have one JDK version less to worry >> about for backports on possible follow-up bugs ;) > > @eme64 I'm glad to have received your feedback. I see I have erroneously > assumed that by making the exact code change you requested still requires > your acceptance - I won't make that mistake again. I had also erroneously > assumed that your review was complete and you had no further changes for me > to make. I'd also not like to make that mistake again, but I'm unsure how to > conclude that a review is complete - it seems like 7 hours of elapsed time > isn't sufficient to indicate completion, so can you please help me figure > this out? Perhaps it's just my distaste for "trickle-in" comments, which I > should get over, or is there another way you can suggest? > > As for the fuzzer I would be very interested in learning more about this. We > have a significant number of compute resources, so it may be valuable for us > to set up a copy of the fuzzer on-site to improve the quality of our > submissions. Can you help in pointing me to someone that can advise me on > how to do this? > > As for holding off the integration, I'll leave the decision to a sponsor for > this PR. I don't believe increasing the reviewer count just to "force" > reevaluation should be an acceptable practice, although I'm not an insider in > this community. @asgibbons I was done with my review, or at least so I thought Still: if I give comments, it would be nice to quickly finish the conversation, unless if I don't respond in many days and not even to emails. Often I only see the glaring issues. Then you fix them, and then I see something else around it. Then I may give more comments. That is what happened. If I think that I have small suggestions and then I'm done, then I might even approve even though there are suggestions still to be added. I just put up the limit really quick so that nobody else would by accident sponsor it before we have finished the conversation, and I will definitely give you my approval once the little issues are resolved ;) - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2139893561
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v48]
On Thu, 30 May 2024 13:56:30 GMT, Emanuel Peter wrote: >> Control question: Are we confident with this potentially going into JDK 23 >> or should we rather postpone to JDK 24? The fork is next week. > >> Control question: Are we confident with this potentially going into JDK 23 >> or should we rather postpone to JDK 24? The fork is next week. > > I would hold off. @asgibbons it may pass our tests, and your extensive > testing. But you never know what the fuzzer can find over a few weeks once it > runs with your changes. I have made that experience many times. Let's just > give it a few days, and then we have one JDK version less to worry about for > backports on possible follow-up bugs ;) @eme64 I guess to add some confidence.. we did also 'test it independently' to catch blind spots. i.e. `String/IndexOf.java` is from me. I tried to be as paranoid as possible with non-random strings. Passed everything I could throw at it.. - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2139882544
Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]
On Thu, 30 May 2024 12:32:22 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to address the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8210471?
>>
>> `java.util.zip.Inflater` when instantiated will hold on the native resources
>> which are freed only when `Inflater.end()` is called. The constructor of
>> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal
>> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor
>> before returning from the constructor, can run into either a
>> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException`
>> when trying to read a GZIP header from the underlying `InputStream`. In
>> either of these cases, the `Inflater` that the `GZIPInputStream` created
>> internally will end up leaking and the caller has no way to `end()` that
>> `Inflater` or even knowledge of that `Inflater`.
>>
>> The commit in this PR catches the `IOException` when reading the GZIP header
>> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an
>> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)`
>> will now check the `InputStream` to be non-null and `size` to be `>0`, both
>> of which were being done by the `super` constructor.
>>
>> Given the nature of the change, no new test has been added. Existing tests
>> in `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3
>> testing is in progress.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> introduce a basic test for GZIPInputStream
Looks good overall, but I believe we need to address the comment below unless I
missed something
src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 97:
> 95: */
> 96: private static Inflater createInflater(InputStream in, int size) {
> 97: Objects.requireNonNull(in);
I don't believe we currently indicate at at NPE will be thrown if the
InputStream is null so this would require a CSR if we need to document it
-
PR Review: https://git.openjdk.org/jdk/pull/19475#pullrequestreview-2088514194
PR Review Comment: https://git.openjdk.org/jdk/pull/19475#discussion_r1620877576
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v48]
On Thu, 30 May 2024 13:56:30 GMT, Emanuel Peter wrote: >> Control question: Are we confident with this potentially going into JDK 23 >> or should we rather postpone to JDK 24? The fork is next week. > >> Control question: Are we confident with this potentially going into JDK 23 >> or should we rather postpone to JDK 24? The fork is next week. > > I would hold off. @asgibbons it may pass our tests, and your extensive > testing. But you never know what the fuzzer can find over a few weeks once it > runs with your changes. I have made that experience many times. Let's just > give it a few days, and then we have one JDK version less to worry about for > backports on possible follow-up bugs ;) @eme64 I'm glad to have received your feedback. I see I have erroneously assumed that by making the exact code change you requested still requires your acceptance - I won't make that mistake again. I had also erroneously assumed that your review was complete and you had no further changes for me to make. I'd also not like to make that mistake again, but I'm unsure how to conclude that a review is complete - it seems like 7 hours of elapsed time isn't sufficient to indicate completion, so can you please help me figure this out? Perhaps it's just my distaste for "trickle-in" comments, which I should get over, or is there another way you can suggest? As for the fuzzer I would be very interested in learning more about this. We have a significant number of compute resources, so it may be valuable for us to set up a copy of the fuzzer on-site to improve the quality of our submissions. Can you help in pointing me to someone that can advise me on how to do this? As for holding off the integration, I'll leave the decision to a sponsor for this PR. I don't believe increasing the reviewer count just to "force" reevaluation should be an acceptable practice, although I'm not an insider in this community. - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2139814010
Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option
On Fri, 24 May 2024 01:08:03 GMT, Alexander Matveev wrote: > This issue is reproducible with and without `--mac-sign`. jpackage will > "_ad-hoc_" sign application bundle when `--mac-sign` is not specified by > using pseudo-identity "_-_". This is why jpackage tries to sign added files > and this is expected behavior by jpackage. "codesign" fails since added > content made application bundle structure invalid. There is nothing we can do > on jpackage side to sign such invalid bundles. As proposed solution we will > output possible reason for "codesign" failure if it fails and `--app-content` > was specified and possible solution. Proposed message: "One of the possible > reason for "codesign" failure is additional content provided via > "--app-content", which made application bundle structure invalid. Make sure > to provide additional content in a way it will not break application bundle > structure, otherwise add additional content as post-processing step." > > Example: > Lets assume we have "ReadMe" folder with "ReadMe.txt" file in it. > 1) jpackage --type app-image -n Test --app-content ReadMe/ReadMe.txt ... > "codesign" will fail with "In subcomponent: Test.app/Contents/ReadMe.txt". > This is expected and "ReadMe.txt" placed in "Test.app/Contents" which is also > expected. > 2) jpackage --type app-image -n Test --app-content ReadMe ... > Works and "ReadMe.txt" will be placed under "Test.app/Contents/ReadMe". > > Sample output before fix: > > Error: "codesign" failed with following output: > Test.app: replacing existing signature > Test.app: code object is not signed at all > In subcomponent: Test.app/Contents/ReadMe.txt > > > Sample output after fix: > > One of the possible reason for "codesign" failure is additional content > provided via "--app-content", which made application bundle structure > invalid. Make sure to provide additional content in a way it will not break > application bundle structure, otherwise add additional content as > post-processing step. > Error: "codesign" failed with following output: > Test.app: replacing existing signature > Test.app: code object is not signed at all > In subcomponent: Test.app/Contents/ReadMe.txt How about this wording for the message: "codesign" failed and additional application content was supplied via the "--app-content" parameter. Probably the additional content broke the integrity of the application bundle and caused the failure. Ensure content supplied via the "--app-content" parameter does not break the integrity of the application bundle, or add it in the post-processing step. - PR Comment: https://git.openjdk.org/jdk/pull/19377#issuecomment-2139747437
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. Vicente filed a bug on javac to investigate this: https://bugs.openjdk.org/browse/JDK-8333278 I wouldn't mind using condy for constant aka non-capturing lambdas. I recall we had a prototype for that years ago, but switching over was shelved for some reason. - PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2139703977
Re: RFR: 8331189: Implementation of Scoped Values (Third Preview) [v2]
On Wed, 29 May 2024 15:35:41 GMT, Alan Bateman wrote: >> JEP 481 proposes Scoped Values to continue to preview in JDK 23 with one >> change. The type of the operation parameter of the callWhere method is >> changed to a new functional interface to avoid having the API throw >> Exception. With that change, the getWhere (and the corresponding method on >> Carrier) are no longer needed. The functional interface is not proposed for >> j.u.function at this time. > > Alan Bateman 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 seven additional > commits since the last revision: > > - Merge > - Merge > - Merge > - Set JEP number > - Sync up from loom repo > - Merge > - Initial commit Changes look good. I like how the new functional interface makes the API seem smaller. - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19136#pullrequestreview-2088433436
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v12]
> This set of changes address causes of poor utilization with small numbers of > cores due to overly aggressive contention avoidance. A number of further > adjustments were needed to still avoid most contention effects in deployments > with large numbers of cores Doug Lea has updated the pull request incrementally with one additional commit since the last revision: Don't shadow parks - Changes: - all: https://git.openjdk.org/jdk/pull/19131/files - new: https://git.openjdk.org/jdk/pull/19131/files/1b818b48..b13d8ee1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19131=11 - incr: https://webrevs.openjdk.org/?repo=jdk=19131=10-11 Stats: 19 lines in 1 file changed: 5 ins; 9 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/19131.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19131/head:pull/19131 PR: https://git.openjdk.org/jdk/pull/19131
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. For constant method reference, the solution is to use a constant dynamic instead of an invokedynamic. - PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2139645936
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v49]
On Thu, 30 May 2024 13:19:57 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 two additional > commits since the last revision: > > - Stupid EOL at end > - Add @test block; fix test indentation test/jdk/java/lang/String/IndexOf.java line 25: > 23: > 24: /* > 25: * @test You should add the `@bug 8320448` for all runs. test/jdk/java/lang/String/IndexOf.java line 27: > 25: * @test > 26: * @summary test String indexOf() intrinsic > 27: * @run main/othervm IndexOf Suggestion: * @run main IndexOf You do not need a new VM if you have no arguments ;) test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 25: > 23: > 24: /* @test > 25: * @bug 4162796 4162796 You need to fix the bug numbers. test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 27: > 25: * @bug 4162796 4162796 > 26: * @summary Test indexOf and lastIndexOf > 27: * @run main/othervm -Xbatch -XX:-TieredCompilation > -XX:CompileCommand=dontinline,ECoreIndexOf.indexOfKernel ECoreIndexOf I would also add a line without `-XX:-TieredCompilation`, then C1 can be tested with this too test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 32: > 30: > 31: /* @test > 32: * @bug 4162796 4162796 Here too - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620760730 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620756896 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620753321 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620754948 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620753577
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v48]
On Thu, 30 May 2024 12:58:27 GMT, Scott Gibbons wrote: >> test/jdk/java/lang/String/IndexOf.java line 35: >> >>> 33: * @requires vm.cpu.features ~= ".*avx2.*" >>> 34: * @requires vm.compiler2.enabled >>> 35: * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -Xcomp >>> -XX:-TieredCompilation -XX:UseAVX=2 -XX:+UnlockDiagnosticVMOptions >>> -XX:+EnableX86ECoreOpts IndexOf >> >> Same here: why is the test AVX2 specific? Could other platforms not also be >> "tickled" in interesting ways with this test? > > There are two test blocks, so all platforms will be able to take advantage of > the test via the first block. I'm told that's how this works. Yes, that is right. Good. >> test/jdk/java/lang/StringBuffer/IndexOf.java line 188: >> >>> 186: } >>> 187: >>> 188: } >> >> It looks like you just indented basically the whole file by 1 space. Why? > > I hadn't noticed this. It's most likely an artifact of my editor as it > wasn't intentional. I'll change this back. Ok, maybe check your code on GitHub next time ;) - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620768228 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620746147
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v48]
On Thu, 30 May 2024 06:25:32 GMT, Tobias Hartmann wrote: > Control question: Are we confident with this potentially going into JDK 23 or > should we rather postpone to JDK 24? The fork is next week. I would hold off. @asgibbons it may pass our tests, and your extensive testing. But you never know what the fuzzer can find over a few weeks once it runs with your changes. I have made that experience many times. Let's just give it a few days, and then we have one JDK version less to worry about for backports on possible follow-up bugs ;) - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2139615822
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v48]
On Thu, 30 May 2024 13:03:06 GMT, Scott Gibbons wrote: >> Would be a shame to spend so much time on writing a test and then not apply >> it everywhere ;) > > I'll add a separate @test block to this file. It was, however, written > specifically tuned for the new algorithm to exercise known edge cases. A new `@test` sounds like a good idea - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620747402
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v48]
On Thu, 30 May 2024 13:33:40 GMT, Scott Gibbons wrote: >> Control question: Are we confident with this potentially going into JDK 23 >> or should we rather postpone to JDK 24? The fork is next week. > > Thank you all for the comments. @TobiHartmann I'm comfortable with this > going into JDK 23. The code has been functionally stable for me for the past > 2 months. The recent churn centers primarily around restructuring the code > for readability and maintainability and ensuring protection against reading > past the end of strings. Both Vlad (Volodymyr) and @sviswa7 have scoured the > code with me and together we have convinced ourselves that we've covered all > the bases. Of course we may have missed something but my confidence is high. > > The overall performance gain as reported by the StringIndexOf JMH averages > ~7x running on an e-core as compared with baseline on the same core. It's > skewed somewhat towards massive gains for long (~2K) strings (avg 14.4x) and > modest gains for small-ish strings (avg ~1.8x). I've measured up to 60x > performance improvement for some 2K UTF-16 indexOf operations. > > Again, thank you all. It's been a fun exercise and I've learned a lot. @asgibbons generally it would be nice if you waited for me to accept your changes before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2139604424
RFR: 8333086: Using Console.println is unnecessarily slow due to JLine initalization
Consider these two programs:
public class SystemPrint {
public static void main(String... args) {
System.err.println("Hello!");
}
}
and:
public class IOPrint {
public static void main(String... args) {
java.io.IO.println("Hello!");
}
}
They do the same conceptual thing - write a text to the output. But,
`IO.println` delegates to `Console.println`, which then delegates to a
`Console` backend, and the default backend is currently based on JLine.
The issues is that JLine takes a quite a long time to initialize, and in a
program like this, JLine is not really needed - it is used to provide better
editing experience when reading input, but there's no reading in these programs.
For example, on my computer:
$ time java -classpath /tmp SystemPrint
Hello!
real0m0,035s
user0m0,019s
sys 0m0,019s
$ time java -classpath /tmp --enable-preview IOPrint
Hello!
real0m0,165s
user0m0,324s
sys 0m0,042s
The proposal herein is to delegate to the simpler `Console` backend from
`java.base` as long as the user only uses methods that print to output, and
switch to the JLine delegate when other methods (typically input) is used. Note
that while technically `writer()` is a method doing output, it will force JLine
initialization to avoid possible problems if the client caches the writer and
uses it after switching the delegates.
With this patch, I can get timing like this:
$ time java --enable-preview -classpath /tmp/ IOPrint
Hello!
real0m0,051s
user0m0,038s
sys 0m0,020s
which seems much more acceptable.
There is also #19467, which may reduce the time further.
A future work might focus on making JLine initialize faster, but avoiding JLine
initialization in case where we don't need it seems like a good step to me in
any case.
-
Commit messages:
- Cleanup, addint test.
- Using println correctly, flushing the java.base delegate before switching to
JLine.
- Force Terminal when writer is requested.
- Attempting to speedup start by delaying initialization of JLine until really
necessary.
Changes: https://git.openjdk.org/jdk/pull/19479/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19479=00
Issue: https://bugs.openjdk.org/browse/JDK-8333086
Stats: 225 lines in 2 files changed: 212 ins; 1 del; 12 mod
Patch: https://git.openjdk.org/jdk/pull/19479.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19479/head:pull/19479
PR: https://git.openjdk.org/jdk/pull/19479
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. Marked as reviewed by liach (Author). Indeed, CallSites are created per indy instruction instead of per CP indy entry (required by JVMS); thus sharing instruction either in initializers or static methods would have the same effect. javac only deduplicates the CP entry. - PR Review: https://git.openjdk.org/jdk/pull/19477#pullrequestreview-2088345919 PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2139602569
Re: RFR: 8333265: De-duplicate method references in java.util.stream.FindOps
On Thu, 30 May 2024 13:04:46 GMT, Chen Liang wrote: > Should we extract them to private static utility methods for potential > laziness support in the future? Ideally we shouldn't have to do any of this. I thought such method references were already de-duplicated in javac - at least within the same compilation units - but I saw this in a startup profile and was surprised myself that the demonstrated manual de-duplication has an observable effect. - PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2139576911
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v48]
On Thu, 30 May 2024 06:25:32 GMT, Tobias Hartmann wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove duplicate vm.compiler2.enabled > > Control question: Are we confident with this potentially going into JDK 23 or > should we rather postpone to JDK 24? The fork is next week. Thank you all for the comments. @TobiHartmann I'm comfortable with this going into JDK 23. The code has been functionally stable for me for the past 2 months. The recent churn centers primarily around restructuring the code for readability and maintainability and ensuring protection against reading past the end of strings. Both Vlad (Volodymyr) and @sviswa7 have scoured the code with me and together we have convinced ourselves that we've covered all the bases. Of course we may have missed something but my confidence is high. The overall performance gain as reported by the StringIndexOf JMH averages ~7x running on an e-core as compared with baseline on the same core. It's skewed somewhat towards massive gains for long (~2K) strings (avg 14.4x) and modest gains for small-ish strings (avg ~1.8x). I've measured up to 60x performance improvement for some 2K UTF-16 indexOf operations. Again, thank you all. It's been a fun exercise and I've learned a lot. - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2139569361
Integrated: 8331876: JFR: Move file read and write events to java.base
On Tue, 7 May 2024 19:32:57 GMT, Erik Gahlin wrote: > Hi, > > Could I have a review of a change that moves the jdk.FileRead and > jdk.FileWrite events to java.base to remove the use of the ASM > instrumentation. > > Testing: jdk/jdk/jfr, tier1-tier4 > > Thanks > Erik This pull request has now been integrated. Changeset: 4a20691e Author:Erik Gahlin URL: https://git.openjdk.org/jdk/commit/4a20691e9b0276e2dc5e7eb6a4d05393d6b4c99c Stats: 1438 lines in 22 files changed: 651 ins; 762 del; 25 mod 8331876: JFR: Move file read and write events to java.base Reviewed-by: mgronlun, alanb - PR: https://git.openjdk.org/jdk/pull/19129
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v48]
On Thu, 30 May 2024 06:23:05 GMT, Emanuel Peter wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove duplicate vm.compiler2.enabled > > test/jdk/java/lang/String/IndexOf.java line 35: > >> 33: * @requires vm.cpu.features ~= ".*avx2.*" >> 34: * @requires vm.compiler2.enabled >> 35: * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -Xcomp >> -XX:-TieredCompilation -XX:UseAVX=2 -XX:+UnlockDiagnosticVMOptions >> -XX:+EnableX86ECoreOpts IndexOf > > Same here: why is the test AVX2 specific? Could other platforms not also be > "tickled" in interesting ways with this test? There are two test blocks, so all platforms will be able to take advantage of the test via the first block. I'm told that's how this works. > test/jdk/java/lang/StringBuffer/IndexOf.java line 188: > >> 186: } >> 187: >> 188: } > > It looks like you just indented basically the whole file by 1 space. Why? I hadn't noticed this. It's most likely an artifact of my editor as it wasn't intentional. I'll change this back. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620669257 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1620679629
