Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v4]
> 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 six additional commits since the last revision: - 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 - Changes: - all: https://git.openjdk.org/jdk/pull/19330/files - new: https://git.openjdk.org/jdk/pull/19330/files/a68519d3..b3b94639 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19330=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19330=02-03 Stats: 6144 lines in 270 files changed: 3709 ins; 843 del; 1592 mod Patch: https://git.openjdk.org/jdk/pull/19330.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19330/head:pull/19330 PR: https://git.openjdk.org/jdk/pull/19330
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v6]
On Tue, 28 May 2024 16:56:18 GMT, Naoto Sato wrote: >> This test intends to verify the behavior of JdkConsole for the java.base >> module, wrt restoring the echo. The test assumes the internal methods that >> sets/gets the echo status of the platform. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 23 commits: > > - incorporated the same fix from 8332922 > - Merge branch 'master' into JDK-8332161-restoreEcho-Test > - Merge branch 'master' into JDK-8332161-restoreEcho-Test > - Merge branch 'JDK-8332084-restoreEcho-readLock' into > JDK-8332161-restoreEcho-Test > - Added comment for restoreEchoLock > - fixing typo > - Merge branch 'JDK-8332084-restoreEcho-readLock' into > JDK-8332161-restoreEcho-Test > - Merge branch 'master' into JDK-8332084-restoreEcho-readLock > - leftover typo > - get/setEcho() -> echo() > - ... and 13 more: https://git.openjdk.org/jdk/compare/b8f2ec90...2477adf4 Marked as reviewed by joehw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19315#pullrequestreview-2084275580
Re: RFR: 8330182: Start of release updates for JDK 24 [v8]
> 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 16 commits: - 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. - Merge branch 'master' into JDK-8330188 - Update deprecated options test. - ... and 6 more: https://git.openjdk.org/jdk/compare/0f3e2cc3...45e1c040 - Changes: https://git.openjdk.org/jdk/pull/18787/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18787=07 Stats: 1742 lines in 49 files changed: 1679 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: 8332597: Remove redundant methods from j.l.classfile.ClassReader API [v2]
On Fri, 24 May 2024 16:41:33 GMT, Adam Sotona wrote: >> j.l.classfile.ClassReader instance is exposed in the Class-File API through >> j.l.classfile.AttributeMapper::readAttribute method only. >> ClassReader only purpose is to serve as a tool for reading content of a >> custom attribute in a user-provided AttribtueMapper. >> It contains useful set of low-level class reading methods for user to >> implement a custom attribute content parser. >> >> However methods ClassReader::thisClassPos and >> ClassReader::skipAttributeHolder are not necessary for a custom attribute >> content parsing and so redundant in the API. >> Class-File API implementation internally use these methods, however they >> should not be exposed in the API. >> >> This patch removes the methods from the API. >> >> 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 two commits: > > - Merge branch 'master' into JDK-8332597-classreader-redundancy > ># Conflicts: ># src/java.base/share/classes/jdk/internal/classfile/impl/ClassImpl.java > - 8332597: Remove redundant methods from j.l.classfile.ClassReader API @asotona Can you transition the CSR to proposed again for reviewing? - PR Comment: https://git.openjdk.org/jdk/pull/19323#issuecomment-2136470497
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo wrote:
>> Please review this PR, which supersedes a now withdrawn
>> https://github.com/openjdk/jdk/pull/14831.
>>
>> This PR replaces `ArraysSupport.vectorizedHashCode` with a set of more
>> user-friendly methods. Here's a summary:
>>
>> - Made the operand constants (i.e. `T_BOOLEAN` and friends) and the
>> `vectorizedHashCode` method private
>>
>> - Made the `vectorizedHashCode` method private, but didn't rename it.
>> Renaming would dramatically increase this PR review cost, because that
>> method's name is used by a lot of VM code. On a bright side, since the
>> method is now private, it's no longer callable by clients of
>> `ArraysSupport`, thus a problem of an inaccurate name is less severe.
>>
>> - Made the `ArraysSupport.utf16HashCode` method private
>>
>> - Moved tiny cases (i.e. 0, 1, 2) to `ArraysSupport`
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix incorrect utf16 hashCode adaptation
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 320:
> 318: * @return the calculated hash value
> 319: */
> 320: public static int hashCode(Object[] a, int fromIndex, int length,
> int initialValue) {
Is the object variant necessary here? The object version is hard for JIT to
profile as it's quite polymorphic compared to other arrays, and the initial
value is always 1.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618129002
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 22:20:39 GMT, Pavel Rappo wrote: >> I believe, it should be `1`. Hear me out. In this method, the `length` is >> scaled down, whereas in `StringUTF16` it is not. In this method, it's >> `length`, in `StringUTF16` it's `((byte[]) value).length`. > > In fact, if I change it to `2`, the following tests will fail: > > - `jdk/jdk/classfile/Utf8EntryTest.java` > - `jdk/java/util/zip/ZipCoding.java` > - `jdk/java/text/Format/MessageFormat/MessageRegression.java` Indeed, the actual length passed at call site is `value.length >> 1` instead of `value.length`; this adjusted char-length carries on to `vectorizedHashCode` call. - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618126401
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v47]
On Tue, 28 May 2024 23:52:27 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: > > Move assert to where it's actually important. Looks good to me. - Marked as reviewed by sviswanathan (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16753#pullrequestreview-2084177134
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
On Tue, 28 May 2024 20:26:40 GMT, Naoto Sato wrote: >> As a non-standard comment, it will trigger a warning (and hence an error), >> since the prevailing standard for `java.base` is to compile with all >> warnings enabled (`-Xlint`) and no warnings found (verified by `-Werror`) >> >> The alternative would be to use `@SuppressWarnings` for the >> `dangling-doc-comment` warning, but that too would be a code change to these >> imported files. > > OK, we will need to live with it. Yes, while there is a strong preference to leave upstream sources unedited in the JDK, to my mind being able to compile the java.base module with all warnings enabled should take precedence. - PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1618046467
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v47]
> 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: Move assert to where it's actually important. - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/355325d0..db0ab75a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=46 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=45-46 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 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 [v46]
> 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: Final review comments - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/751aace8..355325d0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=45 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=44-45 Stats: 95 lines in 3 files changed: 23 ins; 51 del; 21 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 [v19]
On Thu, 16 May 2024 18:09:04 GMT, Scott Gibbons wrote: >> src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 418: >> >>> 416: __ cmpq(haystack_len, 0x10); >>> 417: __ ja_b(L_moreThan16); >>> 418: >> >> An assert here to check for header size >= 16 would be good. >> Also a comment here would he good, something like: >> // Copy 16 or 32 bytes prior to haystack end onto stack >> // This will possibly including some object header bytes when haystack >> length is less than 16 or 32 bytes // Set the new haystack address to >> beginning of copied haystack on stack adjusting for extra bytes copied > > I don't know how to assert header size >= 16 bytes, so I'll add a comment > stating such. If you can tell me how to assert, I'll add that code in place > of the comment. Fixed in library_call.cpp - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617955173
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 21:17:07 GMT, Scott Gibbons wrote: >> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 488: >> >>> 486: __ cmpq(r11, nMinusK); >>> 487: __ ja_b(L_return); >>> 488: __ movq(rax, r11); >> >> At places where we know that return value in r11 is correct, we dont need to >> checkRange so this could have its own label. > > Disabling causes the test to succeed, so we're not finding matches beyond the > end of the string, correct? Are we confident that this test passing can > warrant removing the range check? @sviswa7 ? Removed. >> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 621: >> >>> 619: __ addq(hsPtrRet, index); >>> 620: __ movq(r11, hsPtrRet); >>> 621: __ jmp(L_checkRangeAndReturn); >> >> Why do we have to checkRange here, would it not be always correct? It so we >> could return r11 directly (by moving into rax). > > There are cases where r11 could have a value that, when added to (k - 1) > would go past the end of the haystack. I did all in my power to ensure that > it doesn't but there's no test I know of to ensure that condition. I would > recommend leaving this in for now. Removed checkRangeAndReturn - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617956870 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617956635
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
> With the advent of JEP 467, `///` comments may be treated as documentation > comments, and may be subject to the recently new `javac` warning about > "dangling doc comments" in unexpected places. > > In keeping with the policy to keep the `java.base` module free of all `javac` > warnings, this patch proposes edits to existing uses of `///`. > > There are two dominant policies in the proposed changes. > 1. A long horizontal line of `/` is replaced by `//-` > 2. A long vertical series of lines beginning `///` is replaced by lines > beginning `//|`. > > As with all style changes, I have also tried to honor local usage, for > consistency. > > In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, > `CLOVER:OFF`). I investigated the use of such comments to determine that the > exact form of the comment prefix was not significant. (Phew!) > > > (This PR is informally blocked by JEP 467). Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: incorporate review comments - Changes: - all: https://git.openjdk.org/jdk/pull/19130/files - new: https://git.openjdk.org/jdk/pull/19130/files/3e039b43..e77a4d14 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19130=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19130=00-01 Stats: 24 lines in 7 files changed: 0 ins; 0 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/19130.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19130/head:pull/19130 PR: https://git.openjdk.org/jdk/pull/19130
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 22:08:06 GMT, Pavel Rappo wrote: >> Yes, should be `2` (`a` is semantically a `char[]`). This typo likely pass >> functional testing since `1` can never happen in practice, and the default >> case should work for any value. There might be a String microbenchmark out >> there that might be slightly unhappy, though. > > I believe, it should be `1`. Hear me out. In this method, the `length` is > scaled down, whereas in `StringUTF16` it is not. In this method, it's > `length`, in `StringUTF16` it's `((byte[]) value).length`. In fact, if I change it to `2`, the following tests will fail: - `jdk/jdk/classfile/Utf8EntryTest.java` - `jdk/java/util/zip/ZipCoding.java` - `jdk/java/text/Format/MessageFormat/MessageRegression.java` - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617950633
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 20:38:21 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 301:
>>
>>> 299: return switch (length) {
>>> 300: case 0 -> initialValue;
>>> 301: case 1 -> 31 * initialValue + JLA.getUTF16Char(a,
>>> fromIndex);
>>
>> There seems to be a mismatch here with the original code in StringUTF16,
>> where the length that is tested for is `2` instead of `1`.
>
> Yes, should be `2` (`a` is semantically a `char[]`). This typo likely pass
> functional testing since `1` can never happen in practice, and the default
> case should work for any value. There might be a String microbenchmark out
> there that might be slightly unhappy, though.
I believe, it should be `1`. Hear me out. In this method, the `length` is
scaled down, whereas in `StringUTF16` it is not. In this method, it's `length`,
in `StringUTF16` it's `((byte[]) value).length`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617941436
Integrated: 8333116: test/jdk/tools/jpackage/share/ServiceTest.java test fails
On Tue, 28 May 2024 20:13:24 GMT, Alexey Semenyuk wrote: > Add missing "--add-opens jdk.jpackage/jdk.jpackage.internal=ALL-UNNAMED" to > the test descriptor This pull request has now been integrated. Changeset: 91ab088d Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk/commit/91ab088d5e64e068bafcda8d08f1769c39ba10d6 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod 8333116: test/jdk/tools/jpackage/share/ServiceTest.java test fails Reviewed-by: almatvee - PR: https://git.openjdk.org/jdk/pull/19436
Re: RFR: 8333116: test/jdk/tools/jpackage/share/ServiceTest.java test fails
On Tue, 28 May 2024 20:13:24 GMT, Alexey Semenyuk wrote: > Add missing "--add-opens jdk.jpackage/jdk.jpackage.internal=ALL-UNNAMED" to > the test descriptor Looks good. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19436#pullrequestreview-2083892285
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 @alexeysemenyukoracle please review - PR Comment: https://git.openjdk.org/jdk/pull/19377#issuecomment-2136148629
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 16:37:23 GMT, Sandhya Viswanathan wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix tests > > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 488: > >> 486: __ cmpq(r11, nMinusK); >> 487: __ ja_b(L_return); >> 488: __ movq(rax, r11); > > At places where we know that return value in r11 is correct, we dont need to > checkRange so this could have its own label. Disabling causes the test to succeed, so we're not finding matches beyond the end of the string, correct? Are we confident that this test passing can warrant removing the range check? @sviswa7 ? - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617901070
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v37]
On Fri, 24 May 2024 20:42:12 GMT, Scott Gibbons wrote:
>> test/jdk/java/lang/StringBuffer/ECoreIndexOf.java line 185:
>>
>>> 183: }
>>> 184:
>>> 185: private static int indexOfKernel(String haystack, String needle) {
>>
>> Is the intention of kernels not to be inlined so that it would be part of
>> separate compilation?
>>
>> If so, you probably want to annotate it with
>> `@CompilerControl(CompilerControl.Mode.DONT_INLINE)`
>>
>> i.e.
>> https://github.com/openjdk/jmh/blob/master/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_16_CompilerControl.java
>
> Fixed.
CompilerControl is unavailable here. Added a runtime option instead.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617894475
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v38]
On Tue, 28 May 2024 16:57:54 GMT, Vladimir Kozlov wrote: >> @vnkozlov I'm getting an error in CI tests with this line added. Can you >> please advise? >> >> `TEST RESULT: Error. Parse Exception: Syntax error in @requires expression: >> invalid name: vm.cpu.features` > > You need to add `vm.cpu.features ` line to `test/jdk/TEST.ROOT` file. Similar > to what we have in `test/hotspot/jtreg/TEST.ROOT` Fixed. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617893462
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 20:56:42 GMT, Scott Gibbons wrote: >> We can also do full reads for (n-k) == 31, as we also compare the kth byte. > > I'll change and test. Passes tests, so I'll change. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617886613
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 20:37:43 GMT, Sandhya Viswanathan wrote: >> I listed all registers for clarity. This ensures that we know what can be >> used as values or as scratch registers with no ambiguity. > > Sounds good. We could keep only comment out of the two as it is the same for > both small haystack and big haystack. OK - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617889756
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 17:36:03 GMT, Scott Gibbons wrote: >> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 488: >> >>> 486: __ cmpq(r11, nMinusK); >>> 487: __ ja_b(L_return); >>> 488: __ movq(rax, r11); >> >> At places where we know that return value in r11 is correct, we dont need to >> checkRange so this could have its own label. > > I don't want to change this because its reason for existence is to ensure we > don't return a value that's beyond the end of the haystack. We don't yet > have a good enough test to validate whether we're reading past the end of the > haystack, so I like this as insurance. I would recommend an experiment. Disable the range-check and run String/IndexOf.java test. Particularly run test4(), which is designed exactly to test the reads beyond the end. It wont find all the bad reads, but right now if there are any failures, they are 'hidden' by this range-check. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617888680
Re: RFR: 8333116: test/jdk/tools/jpackage/share/ServiceTest.java test fails
On Tue, 28 May 2024 20:13:24 GMT, Alexey Semenyuk wrote: > Add missing "--add-opens jdk.jpackage/jdk.jpackage.internal=ALL-UNNAMED" to > the test descriptor @sashamatveev please review - PR Comment: https://git.openjdk.org/jdk/pull/19436#issuecomment-2136094953
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 20:35:26 GMT, Sandhya Viswanathan wrote: >> No. For (n-k)==32 we can do full reads. I'll clarify by changing the label >> name. > > We can also do full reads for (n-k) == 31, as we also compare the kth byte. I'll change and test. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617883225
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 20:29:38 GMT, Sandhya Viswanathan wrote: >> No. This is checking for a zero length haystack. The following compare >> checks for needle length longer than haystack, regardless of the value in >> each. The comparison is signed, so a haystack length of 0 with a needle >> length of -1 will pass the following test and assume validity. > > But we have already checked for needle length to be greater than 0 in the > following lines: > __ cmpq(needle_len_p, 0); > __ jg_b(L_nextCheck); OK >> Again, I think we ought to leave this in. Although it executes ~3 >> instructions that may not be necessary in some cases I think it's best to >> perform the check. Once we have a good enough test to check reading past >> the end of the haystack we can change it. > > In this particular case, we are returning -1 (NoMatch), so no need to do > L_checkRangeAndReturn here, we could directly jump to L_returnError. OK. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617876757 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617874637
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 19:13:30 GMT, Jorn Vernee wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix incorrect utf16 hashCode adaptation
>
> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 275:
>
>> 273: return switch (length) {
>> 274: case 0 -> initialValue;
>> 275: case 1 -> 31 * initialValue + (a[fromIndex] & 0xff);
>
> For clarity, if you think it helps:
> Suggestion:
>
> case 1 -> 31 * initialValue + Byte.toUnsignedInt(a[fromIndex]);
I don't care as long as microbenchmarks don't get a hiccup.
> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 301:
>
>> 299: return switch (length) {
>> 300: case 0 -> initialValue;
>> 301: case 1 -> 31 * initialValue + JLA.getUTF16Char(a,
>> fromIndex);
>
> There seems to be a mismatch here with the original code in StringUTF16,
> where the length that is tested for is `2` instead of `1`.
Yes, should be `2` (`a` is semantically a `char[]`). This typo likely pass
functional testing since `1` can never happen in practice, and the default case
should work for any value. There might be a String microbenchmark out there
that might be slightly unhappy, though.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617867797
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617865658
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 18:11:13 GMT, Scott Gibbons wrote: >> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1333: >> >>> 1331: >>> 1332: __ cmpq(nMinusK, 32); >>> 1333: __ jae_b(L_greaterThan32); >> >> Should this check be (n-k+1) >= 32? And so accordingly (n-k) >= 31 >> __ cmpq(nMinusK, 31); >> __ jae_b(L_greaterThan32); > > No. For (n-k)==32 we can do full reads. I'll clarify by changing the label > name. We can also do full reads for (n-k) == 31, as we also compare the kth byte. >> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 1750: >> >>> 1748: // r15 = unused >>> 1749: // XMM_BYTE_0 - first element of needle, broadcast >>> 1750: // XMM_BYTE_K - last element of needle, broadcast >> >> This comment is duplicated for both small haystack case and big haystack >> case, could be made a common comment. >> Also the only registers that are used as input in the switch case are: >> r14 = needle >> rbx = haystack >> rsi = haystack length (n) >> r12 = needle length (k) >> r10 = n - k (where k is needle length) >> XMM_BYTE_0 = first element of needle, broadcast >> XMM_BYTE_K = last element of needle, broadcast >> So we could only list these, making it easier to comprehend. > > I listed all registers for clarity. This ensures that we know what can be > used as values or as scratch registers with no ambiguity. Sounds good. We could keep only comment out of the two as it is the same for both small haystack and big haystack. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617862799 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617865049
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 20:22:24 GMT, Jonathan Gibbons wrote: > What about changing `///` to `//---` to give slightly more prominence to > these comments, over plain old `//` comments. The dashes give a small sense > of a horizontal rule, to delimit sections of code. > > (FWIW, I have locally edited `//|` to `//` and such comments do not stand out > beside existing use of `//`.) `//---` seems okay, it would stand out a bit more. - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2136060411
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 17:30:24 GMT, Scott Gibbons wrote: >> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 278: >> >>> 276: __ bind(L_nextCheck); >>> 277: __ testq(haystack_len_p, haystack_len_p); >>> 278: __ je(L_zeroCheckFailed); >> >> This check could be removed as the next check covers this one. > > No. This is checking for a zero length haystack. The following compare > checks for needle length longer than haystack, regardless of the value in > each. The comparison is signed, so a haystack length of 0 with a needle > length of -1 will pass the following test and assume validity. But we have already checked for needle length to be greater than 0 in the following lines: __ cmpq(needle_len_p, 0); __ jg_b(L_nextCheck); - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617857240
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 18:50:38 GMT, Jonathan Gibbons wrote: >> src/java.base/share/classes/jdk/internal/icu/impl/StringPrepDataReader.java >> line 122: >> >>> 120: * see store.c of gennorm for more information and values >>> 121: */ >>> 122: // /* dataFormat="SPRP" 0x53, 0x50, 0x52, 0x50 */ >> >> This source file is coming from the upstream ICU4J project. Even if this is >> a `non-standard` comment, I would keep it as it is to minimize the merge >> effort. > > As a non-standard comment, it will trigger a warning (and hence an error), > since the prevailing standard for `java.base` is to compile with all warnings > enabled (`-Xlint`) and no warnings found (verified by `-Werror`) > > The alternative would be to use `@SuppressWarnings` for the > `dangling-doc-comment` warning, but that too would be a code change to these > imported files. OK, we will need to live with it. - PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1617854422
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons wrote: > With the advent of JEP 467, `///` comments may be treated as documentation > comments, and may be subject to the recently new `javac` warning about > "dangling doc comments" in unexpected places. > > In keeping with the policy to keep the `java.base` module free of all `javac` > warnings, this patch proposes edits to existing uses of `///`. > > There are two dominant policies in the proposed changes. > 1. A long horizontal line of `/` is replaced by `//-` > 2. A long vertical series of lines beginning `///` is replaced by lines > beginning `//|`. > > As with all style changes, I have also tried to honor local usage, for > consistency. > > In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, > `CLOVER:OFF`). I investigated the use of such comments to determine that the > exact form of the comment prefix was not significant. (Phew!) > > > (This PR is informally blocked by JEP 467). `GregorianCalendar` and ICU4J files LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19130#pullrequestreview-2083793439
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 17:59:49 GMT, Scott Gibbons wrote: >> src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 578: >> >>> 576: // helper jumps to L_checkRangeAndReturn with a (-1) return value. >>> 577: big_case_loop_helper(false, 0, L_checkRangeAndReturn, L_loopTop, >>> mask, hsPtrRet, needleLen, >>> 578: needle, haystack, hsLength, tmp1, tmp2, tmp3, >>> rScratch, ae, _masm); >> >> If we run out of haystack instead of jumping to L_checkRangeAndReturn, we >> could directly jump to L_retrunError. > > Again, I think we ought to leave this in. Although it executes ~3 > instructions that may not be necessary in some cases I think it's best to > perform the check. Once we have a good enough test to check reading past the > end of the haystack we can change it. In this particular case, we are returning -1 (NoMatch), so no need to do L_checkRangeAndReturn here, we could directly jump to L_returnError. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617853337
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 20:01:46 GMT, Alan Bateman wrote: > > OK. I was just trying to honor the apparent intent to make the comment > > stand out more than just a plain `//` comment, but I have no strong > > feelings against reducing `///` to `//` > > In this case I would reduce it to '//' but others will have different > opinions of course. What about changing `///` to `//---` to give slightly more prominence to these comments, over plain old `//` comments. The dashes give a small sense of a horizontal rule, to delimit sections of code. (FWIW, I have locally edited `//|` to `//` and such comments do not stand out beside existing use of `//`.) - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2136042843
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 19:19:51 GMT, Jorn Vernee wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix incorrect utf16 hashCode adaptation
>
> test/hotspot/jtreg/compiler/intrinsics/TestArraysHashCode.java line 88:
>
>> 86: private static int testIntrinsic(byte[] bytes, int type)
>> 87: throws InvocationTargetException, IllegalAccessException {
>> 88: return (int) vectorizedHashCode.invoke(null, bytes, 0, 256, 1,
>> type);
>
> Better to just call `hashCodeOfUnsigned` here I think.
>
> The test for the non-constant type could be dropped. That is no longer a part
> of the 'API' of `ArraySupport`. It looks like the intrinsic bails out when
> the basic type is not constant any ways:
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L6401-L6404
The non-constant test was added because that very bailout caused a crash. The
other test is actually less interesting since it'll likely be covered
indirectly by regular use. But as we are hiding these away this gets ever more
obscure and perhaps the test could be dropped entirely.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617848032
RFR: 8333116: test/jdk/tools/jpackage/share/ServiceTest.java test fails
Add missing "--add-opens jdk.jpackage/jdk.jpackage.internal=ALL-UNNAMED" to the test descriptor - Commit messages: - Add missing "--add-opens jdk.jpackage/jdk.jpackage.internal=ALL-UNNAMED" to test desc. It fixes the following error: Changes: https://git.openjdk.org/jdk/pull/19436/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19436=00 Issue: https://bugs.openjdk.org/browse/JDK-8333116 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19436.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19436/head:pull/19436 PR: https://git.openjdk.org/jdk/pull/19436
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 18:57:07 GMT, Jonathan Gibbons wrote: > OK. I was just trying to honor the apparent intent to make the comment stand > out more than just a plain `//` comment, but I have no strong feelings > against reducing `///` to `//` In this case I would reduce it to '//' but others will have different opinions of course. - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2136012355
Integrated: 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache'
On Tue, 28 May 2024 14:01:44 GMT, Martin Doerr wrote: > Fix obvious typo in micro benchmark. This pull request has now been integrated. Changeset: 9ac8d05a Author:Martin Doerr URL: https://git.openjdk.org/jdk/commit/9ac8d05a2567fbf65b944660739e5f8ad1fc2020 Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache' Reviewed-by: chagedorn, kvn - PR: https://git.openjdk.org/jdk/pull/19427
Re: RFR: 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache'
On Tue, 28 May 2024 14:01:44 GMT, Martin Doerr wrote: > Fix obvious typo in micro benchmark. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/19427#issuecomment-2136008570
Integrated: 8330542: Template for Creating Strict JAXP Configuration File
On Wed, 17 Apr 2024 23:24:06 GMT, Joe Wang wrote: > Add two sample configuration files: > > jaxp-strict.properties: used to set strict configuration, stricter than > jaxp.properties in previous versions such as JDK 22 > >> jaxp-compat.properties: used to regain compatibility from any more >> restricted configuration than previous versions such as JDK 22 > > Updated on 5/16/2024 > > Design change: > The design is changed to include in the JDK two configuration files that are > the default jaxp.properties and jaxp-strict.properties, instead of three, > dropping jaxp-compat.properties. > > Updated on 5/18/2024 > > Withdraw changes to jaxp.properties. The original idea was to match > jaxp-strict.properties with regard to the properties. However, that change > impact the configuration process, resulting in tests that verify the process > to fail. > > Updated on 5/23/2024 > > Provide a template `jaxp-strict.template` instead of a properties file. This > template can be used to create custom configuration files. This pull request has now been integrated. Changeset: 91caec07 Author:Joe Wang URL: https://git.openjdk.org/jdk/commit/91caec07cb2e4d98d4366f5627f55834282caa94 Stats: 287 lines in 5 files changed: 263 ins; 7 del; 17 mod 8330542: Template for Creating Strict JAXP Configuration File Reviewed-by: lancea, erikj, alanb, ihse, mullan, naoto - PR: https://git.openjdk.org/jdk/pull/18831
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo wrote:
>> Please review this PR, which supersedes a now withdrawn
>> https://github.com/openjdk/jdk/pull/14831.
>>
>> This PR replaces `ArraysSupport.vectorizedHashCode` with a set of more
>> user-friendly methods. Here's a summary:
>>
>> - Made the operand constants (i.e. `T_BOOLEAN` and friends) and the
>> `vectorizedHashCode` method private
>>
>> - Made the `vectorizedHashCode` method private, but didn't rename it.
>> Renaming would dramatically increase this PR review cost, because that
>> method's name is used by a lot of VM code. On a bright side, since the
>> method is now private, it's no longer callable by clients of
>> `ArraysSupport`, thus a problem of an inaccurate name is less severe.
>>
>> - Made the `ArraysSupport.utf16HashCode` method private
>>
>> - Moved tiny cases (i.e. 0, 1, 2) to `ArraysSupport`
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix incorrect utf16 hashCode adaptation
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 252:
> 250: return switch (length) {
> 251: case 0 -> initialValue;
> 252: case 1 -> 31 * initialValue + (int) a[fromIndex];
Suggestion:
case 1 -> 31 * initialValue + (int) a[fromIndex]; // sign extension
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 275:
> 273: return switch (length) {
> 274: case 0 -> initialValue;
> 275: case 1 -> 31 * initialValue + (a[fromIndex] & 0xff);
For clarity, if you think it helps:
Suggestion:
case 1 -> 31 * initialValue + Byte.toUnsignedInt(a[fromIndex]);
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 301:
> 299: return switch (length) {
> 300: case 0 -> initialValue;
> 301: case 1 -> 31 * initialValue + JLA.getUTF16Char(a, fromIndex);
There seems to be a mismatch here with the original code in StringUTF16, where
the length that is tested for is `2` instead of `1`.
test/hotspot/jtreg/compiler/intrinsics/TestArraysHashCode.java line 88:
> 86: private static int testIntrinsic(byte[] bytes, int type)
> 87: throws InvocationTargetException, IllegalAccessException {
> 88: return (int) vectorizedHashCode.invoke(null, bytes, 0, 256, 1,
> type);
Better to just call `hashCodeOfUnsigned` here I think.
The test for the non-constant type could be dropped. That is no longer a part
of the 'API' of `ArraySupport`. It looks like the intrinsic bails out when the
basic type is not constant any ways:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L6401-L6404
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617778741
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617778493
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r161798
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617784996
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Thu, 23 May 2024 05:52:57 GMT, Alan Bateman wrote: > > A long vertical series of lines beginning /// is replaced by lines > > beginning //|. > > This one looks unusual when it's just one line, I could imagine deleting the > "|" in these cases. OK. I was just trying to honor the apparent intent to make the comment stand out more than just a plain `//` comment, but I have no strong feelings against reducing `///` to `//` - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2135914838
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Wed, 22 May 2024 20:13:08 GMT, Naoto Sato wrote: >> With the advent of JEP 467, `///` comments may be treated as documentation >> comments, and may be subject to the recently new `javac` warning about >> "dangling doc comments" in unexpected places. >> >> In keeping with the policy to keep the `java.base` module free of all >> `javac` warnings, this patch proposes edits to existing uses of `///`. >> >> There are two dominant policies in the proposed changes. >> 1. A long horizontal line of `/` is replaced by `//-` >> 2. A long vertical series of lines beginning `///` is replaced by lines >> beginning `//|`. >> >> As with all style changes, I have also tried to honor local usage, for >> consistency. >> >> In one place, a pair of comments appeared to contain directives >> (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to >> determine that the exact form of the comment prefix was not significant. >> (Phew!) >> >> >> (This PR is informally blocked by JEP 467). > > src/java.base/share/classes/jdk/internal/icu/impl/StringPrepDataReader.java > line 122: > >> 120: * see store.c of gennorm for more information and values >> 121: */ >> 122: // /* dataFormat="SPRP" 0x53, 0x50, 0x52, 0x50 */ > > This source file is coming from the upstream ICU4J project. Even if this is a > `non-standard` comment, I would keep it as it is to minimize the merge effort. As a non-standard comment, it will trigger a warning, since the prevailing standard for `java.base` is to compile with all warnings enabled and no warnings found. - PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1617755455
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Tue, 28 May 2024 12:48:19 GMT, Sandhya Viswanathan wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix tests > > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 239: > >> 237: // the needle size is less than 32 bytes, we default to a >> 238: // byte-by-byte comparison (this will be rare). >> 239: // > > Is this still true? Yes. For UL, the code within `L_compareFull` effectively does byte-by-byte. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 278: > >> 276: __ bind(L_nextCheck); >> 277: __ testq(haystack_len_p, haystack_len_p); >> 278: __ je(L_zeroCheckFailed); > > This check could be removed as the next check covers this one. No. This is checking for a zero length haystack. The following compare checks for needle length longer than haystack, regardless of the value in each. The comparison is signed, so a haystack length of 0 with a needle length of -1 will pass the following test and assume validity. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 360: > >> 358: __ push(rcx); >> 359: __ push(r8); >> 360: __ push(r9); > > No need to save/restore rcx/r8/r9 on windows platform as well. OK. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 379: > >> 377: >> 378: // Assume failure >> 379: __ movq(rbp, -1); > > We are no more using rbp at return point so this is not needed now? Removed. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 488: > >> 486: __ cmpq(r11, nMinusK); >> 487: __ ja_b(L_return); >> 488: __ movq(rax, r11); > > At places where we know that return value in r11 is correct, we dont need to > checkRange so this could have its own label. I don't want to change this because its reason for existence is to ensure we don't return a value that's beyond the end of the haystack. We don't yet have a good enough test to validate whether we're reading past the end of the haystack, so I like this as insurance. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 566: > >> 564: // rbp: -1 >> 565: // XMM_BYTE_0 - first element of needle broadcast >> 566: // XMM_BYTE_K - last element of needle broadcast > > The only registers that are used as input in the switch case are: > r14 = needle > rbx = haystack > rsi = haystack length (n) > r12 = needle length (k) > r10 = n - k (where k is needle length) > XMM_BYTE_0 = first element of needle, broadcast > XMM_BYTE_K = last element of needle, broadcast > So we could only list these, making it easier to comprehend. I listed these registers to make it clear which registers had no expected value and could be used for temps, etc. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 578: > >> 576: // helper jumps to L_checkRangeAndReturn with a (-1) return value. >> 577: big_case_loop_helper(false, 0, L_checkRangeAndReturn, L_loopTop, >> mask, hsPtrRet, needleLen, >> 578: needle, haystack, hsLength, tmp1, tmp2, tmp3, >> rScratch, ae, _masm); > > If we run out of haystack instead of jumping to L_checkRangeAndReturn, we > could directly jump to L_retrunError. Again, I think we ought to leave this in. Although it executes ~3 instructions that may not be necessary in some cases I think it's best to perform the check. Once we have a good enough test to check reading past the end of the haystack we can change it. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 597: > >> 595: >> 596: // Need a lot of registers here to preserve state across >> arrays_equals call >> 597: > > This comment is no longer valid, could be removed. OK > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 621: > >> 619: __ addq(hsPtrRet, index); >> 620: __ movq(r11, hsPtrRet); >> 621: __ jmp(L_checkRangeAndReturn); > > Why do we have to checkRange here, would it not be always correct? It so we > could return r11 directly (by moving into rax). There are cases where r11 could have a value that, when added to (k - 1) would go past the end of the haystack. I did all in my power to ensure that it doesn't but there's no test I know of to ensure that condition. I would recommend leaving this in for now. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 660: > >> 658: // Haystack always copied to stack, so 32-byte reads OK >> 659: // Haystack length < 32 >> 660: // 10 < needle length < 32 > > Haystack length <= 32 > 10 < needle length <= 32 Changed. > src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 721: > >> 719:false /* char */, knoreg); >> 720: __ testl(rTmp3, rTmp3); >> 721: __ jne(L_checkRangeAndReturn); > > Why do we have to checkRange here, would it not be always correct? It so we > could return r11 directly (by moving into rax). OK >
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v45]
> 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/01cb58fb..751aace8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=44 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=43-44 Stats: 49 lines in 4 files changed: 20 ins; 13 del; 16 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 [v38]
On Tue, 28 May 2024 16:00:10 GMT, Scott Gibbons wrote: >> test/jdk/java/lang/StringBuffer/IndexOf.java line 28: >> >>> 26: * @summary Test indexOf and lastIndexOf >>> 27: * @run main/othervm IndexOf >>> 28: * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -Xcomp >>> -XX:-TieredCompilation -XX:UseAVX=2 -XX:+UnlockDiagnosticVMOptions >>> -XX:+EnableX86ECoreOpts IndexOf >> >> I suggest to split it into 2 subtest jobs and use `@requires vm.cpu.features >> ~= ".*avx2.*"` for second which specified `-XX:UseAVX=2`. >> See `compiler/loopopts/superword/TestDependencyOffsets.java` for example. > > @vnkozlov I'm getting an error in CI tests with this line added. Can you > please advise? > > `TEST RESULT: Error. Parse Exception: Syntax error in @requires expression: > invalid name: vm.cpu.features` You need to add `vm.cpu.features ` line to `test/jdk/TEST.ROOT` file. Similar to what we have in `test/hotspot/jtreg/TEST.ROOT` - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617630712
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v6]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits: - incorporated the same fix from 8332922 - Merge branch 'master' into JDK-8332161-restoreEcho-Test - Merge branch 'master' into JDK-8332161-restoreEcho-Test - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Added comment for restoreEchoLock - fixing typo - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - leftover typo - get/setEcho() -> echo() - ... and 13 more: https://git.openjdk.org/jdk/compare/b8f2ec90...2477adf4 - Changes: https://git.openjdk.org/jdk/pull/19315/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19315=05 Stats: 192 lines in 2 files changed: 192 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v15]
On Tue, 28 May 2024 16:27:24 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated on 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. >> >> Updated on 5/18/2024 >> >> Withdraw changes to jaxp.properties. The original idea was to match >> jaxp-strict.properties with regard to the properties. However, that change >> impact the configuration process, resulting in tests that verify the process >> to fail. >> >> Updated on 5/23/2024 >> >> Provide a template `jaxp-strict.template` instead of a properties file. This >> template can be used to create custom configuration files. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > update properties files with wording suggestions; move summary to after the > test tag Marked as reviewed by mullan (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2083391002
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v43]
On Sat, 25 May 2024 22:19:41 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 tests src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 239: > 237: // the needle size is less than 32 bytes, we default to a > 238: // byte-by-byte comparison (this will be rare). > 239: // Is this still true? src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 278: > 276: __ bind(L_nextCheck); > 277: __ testq(haystack_len_p, haystack_len_p); > 278: __ je(L_zeroCheckFailed); This check could be removed as the next check covers this one. src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 360: > 358: __ push(rcx); > 359: __ push(r8); > 360: __ push(r9); No need to save/restore rcx/r8/r9 on windows platform as well. src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 379: > 377: > 378: // Assume failure > 379: __ movq(rbp, -1); We are no more using rbp at return point so this is not needed now? src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 488: > 486: __ cmpq(r11, nMinusK); > 487: __ ja_b(L_return); > 488: __ movq(rax, r11); At places where we know that return value in r11 is correct, we dont need to checkRange so this could have its own label. src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 566: > 564: // rbp: -1 > 565: // XMM_BYTE_0 - first element of needle broadcast > 566: // XMM_BYTE_K - last element of needle broadcast The only registers that are used as input in the switch case are: r14 = needle rbx = haystack rsi = haystack length (n) r12 = needle length (k) r10 = n - k (where k is needle length) XMM_BYTE_0 = first element of needle, broadcast XMM_BYTE_K = last element of needle, broadcast So we could only list these, making it easier to comprehend. src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 578: > 576: // helper jumps to L_checkRangeAndReturn with a (-1) return value. > 577: big_case_loop_helper(false, 0, L_checkRangeAndReturn, L_loopTop, > mask, hsPtrRet, needleLen, > 578: needle, haystack, hsLength, tmp1, tmp2, tmp3, > rScratch, ae, _masm); If we run out of haystack instead of jumping to L_checkRangeAndReturn, we could directly jump to L_retrunError. src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 597: > 595: > 596: // Need a lot of registers here to preserve state across > arrays_equals call > 597: This comment is no longer valid, could be removed. src/hotspot/cpu/x86/c2_stubGenerator_x86_64_string.cpp line 621: > 619: __ addq(hsPtrRet, index); > 620: __ movq(r11, hsPtrRet); > 621: __ jmp(L_checkRangeAndReturn); Why do we have to checkRange here, would it not be always correct? It so we could return r11 directly (by moving into rax).
Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]
On Fri, 24 May 2024 16:36:32 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated on 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. >> >> Updated on 5/18/2024 >> >> Withdraw changes to jaxp.properties. The original idea was to match >> jaxp-strict.properties with regard to the properties. However, that change >> impact the configuration process, resulting in tests that verify the process >> to fail. >> >> Updated on 5/23/2024 >> >> Provide a template `jaxp-strict.template` instead of a properties file. This >> template can be used to create custom configuration files. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > rename the template to jaxp-strict.properties.template Thanks Sean, Naoto! Updated accordingly. - PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2135662540
Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v15]
> Add two sample configuration files: > > jaxp-strict.properties: used to set strict configuration, stricter than > jaxp.properties in previous versions such as JDK 22 > >> jaxp-compat.properties: used to regain compatibility from any more >> restricted configuration than previous versions such as JDK 22 > > Updated on 5/16/2024 > > Design change: > The design is changed to include in the JDK two configuration files that are > the default jaxp.properties and jaxp-strict.properties, instead of three, > dropping jaxp-compat.properties. > > Updated on 5/18/2024 > > Withdraw changes to jaxp.properties. The original idea was to match > jaxp-strict.properties with regard to the properties. However, that change > impact the configuration process, resulting in tests that verify the process > to fail. > > Updated on 5/23/2024 > > Provide a template `jaxp-strict.template` instead of a properties file. This > template can be used to create custom configuration files. Joe Wang has updated the pull request incrementally with one additional commit since the last revision: update properties files with wording suggestions; move summary to after the test tag - Changes: - all: https://git.openjdk.org/jdk/pull/18831/files - new: https://git.openjdk.org/jdk/pull/18831/files/714095d1..abc1e88b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18831=14 - incr: https://webrevs.openjdk.org/?repo=jdk=18831=13-14 Stats: 21 lines in 3 files changed: 2 ins; 2 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/18831.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18831/head:pull/18831 PR: https://git.openjdk.org/jdk/pull/18831
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v44]
> 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: Revert changes to IndexOf.java - Changes: - all: https://git.openjdk.org/jdk/pull/16753/files - new: https://git.openjdk.org/jdk/pull/16753/files/15994a39..01cb58fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16753=43 - incr: https://webrevs.openjdk.org/?repo=jdk=16753=42-43 Stats: 382 lines in 1 file changed: 0 ins; 222 del; 160 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 [v41]
On Sat, 25 May 2024 06:33:51 GMT, Alan Bateman wrote:
>> Scott Gibbons has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix test; review comments
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 47:
>
>> 45: public class IndexOf {
>> 46:
>> 47: static Random generator = new Random();
>
> @RogerRiggs Would you have cycles to look at Scott's changes to this test? I
> suspect it will need to be re-structured, re-formatted, and commented to get
> into maintainable shape.
I am going to revert my changes to this file as the test
`jdk/java/lang/String/IndexOf.java` covers the code better.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617560447
Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]
On Fri, 24 May 2024 16:36:32 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated on 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. >> >> Updated on 5/18/2024 >> >> Withdraw changes to jaxp.properties. The original idea was to match >> jaxp-strict.properties with regard to the properties. However, that change >> impact the configuration process, resulting in tests that verify the process >> to fail. >> >> Updated on 5/23/2024 >> >> Provide a template `jaxp-strict.template` instead of a properties file. This >> template can be used to create custom configuration files. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > rename the template to jaxp-strict.properties.template LGTM test/jaxp/javax/xml/jaxp/unittest/common/config/ConfigFileTest.java line 41: > 39: * @run driver common.config.ConfigFileTest 0 // verifies jaxp.properties > 40: * @run driver common.config.ConfigFileTest 1 // verifies > jaxp-strict.properties.template > 41: * @summary verifies the default JAXP configuration file jaxp.properties > and Summary would read better after the @test tag - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2079802647 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1615343327
Re: RFR: 8320448: Accelerate IndexOf using AVX2 [v38]
On Fri, 24 May 2024 20:12:07 GMT, Vladimir Kozlov wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Test clarifications > > test/jdk/java/lang/StringBuffer/IndexOf.java line 28: > >> 26: * @summary Test indexOf and lastIndexOf >> 27: * @run main/othervm IndexOf >> 28: * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -Xcomp >> -XX:-TieredCompilation -XX:UseAVX=2 -XX:+UnlockDiagnosticVMOptions >> -XX:+EnableX86ECoreOpts IndexOf > > I suggest to split it into 2 subtest jobs and use `@requires vm.cpu.features > ~= ".*avx2.*"` for second which specified `-XX:UseAVX=2`. > See `compiler/loopopts/superword/TestDependencyOffsets.java` for example. @vnkozlov I'm getting an error in CI tests with this line added. Can you please advise? `TEST RESULT: Error. Parse Exception: Syntax error in @requires expression: invalid name: vm.cpu.features` - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1617556335
Re: RFR: 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache'
On Tue, 28 May 2024 14:01:44 GMT, Martin Doerr wrote: > Fix obvious typo in micro benchmark. Good. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19427#pullrequestreview-2083281226
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v11]
> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use > classfile API for reflection proxy-generation. Actual implementation of > `ProxyGenerator` is focused on performance, however it causes JDK bootstrap > regressions. `ProxyGenerator.TEMPLATE` class model is statically created and > each proxy class is transformed from the template. > > This patch is intended to examine plain proxy generation impact on > performance and JDK bootstrap (vs proxy transformation from template). > > The generated proxy is migrated from static initialization to CONDY bootstrap. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: missing bracket - Changes: - all: https://git.openjdk.org/jdk/pull/19410/files - new: https://git.openjdk.org/jdk/pull/19410/files/9ce8377c..24b60451 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19410=10 - incr: https://webrevs.openjdk.org/?repo=jdk=19410=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19410.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19410/head:pull/19410 PR: https://git.openjdk.org/jdk/pull/19410
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v10]
> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use > classfile API for reflection proxy-generation. Actual implementation of > `ProxyGenerator` is focused on performance, however it causes JDK bootstrap > regressions. `ProxyGenerator.TEMPLATE` class model is statically created and > each proxy class is transformed from the template. > > This patch is intended to examine plain proxy generation impact on > performance and JDK bootstrap (vs proxy transformation from template). > > The generated proxy is migrated from static initialization to CONDY bootstrap. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/19410/files - new: https://git.openjdk.org/jdk/pull/19410/files/b3f6be89..9ce8377c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19410=09 - incr: https://webrevs.openjdk.org/?repo=jdk=19410=08-09 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19410.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19410/head:pull/19410 PR: https://git.openjdk.org/jdk/pull/19410
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v9]
On Tue, 28 May 2024 15:11:39 GMT, Chen Liang wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> removed obsolete entry > > src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 676: > >> 674: toClassDesc(fromClass), >> 675: method.getName(), >> 676: >> MethodType.methodType(method.getReturnType(), >> method.getParameterTypes()).describeConstable().get())); > > Suggestion: > > desc); good catch :) - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1617486750
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v9]
On Tue, 28 May 2024 14:56:35 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > removed obsolete entry src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 676: > 674: toClassDesc(fromClass), > 675: method.getName(), > 676: > MethodType.methodType(method.getReturnType(), > method.getParameterTypes()).describeConstable().get())); Suggestion: desc); - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1617457062
Integrated: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal
On Fri, 10 May 2024 10:06:55 GMT, Alan Bateman wrote: > This is the implementation changes for JEP 471. > > The methods in sun.misc.Unsafe for on-heap and off-heap access are deprecated > for removal. This means a removal warning at compile time. No methods have > been removed. A deprecated message is added to each of the methods but > unlikely to be seen as the JDK does not generate or publish the API docs for > this class. > > A new command line option --sun-misc-unsafe-memory-access=$value is > introduced to allow or deny access to these methods. The default proposed for > JDK 23 is "allow" so no change in behavior compared to JDK 22 or previous > releases. > > A new test is added to test the command line option settings. The existing > micros for FFM that use Unsafe are updated to suppress the removal warning at > compile time. A new micro is introduced with a small sample of methods to > ensure the changes doesn't cause any perf regressions ([sample > results](https://cr.openjdk.org/~alanb/8331670-results.txt)). > > For now, the changes include the update to the man page for the "java" > command. It might be that this has to be separated out so that it goes with > other updates in the release. This pull request has now been integrated. Changeset: 0f3e2cc3 Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/0f3e2cc334e5926d53bbbce22e4a6bfeb2752140 Stats: 1234 lines in 8 files changed: 1150 ins; 5 del; 79 mod 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal Reviewed-by: mcimadamore, jpai, pminborg - PR: https://git.openjdk.org/jdk/pull/19174
Re: RFR: 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache'
On Tue, 28 May 2024 14:01:44 GMT, Martin Doerr wrote: > Fix obvious typo in micro benchmark. Thanks for the review! - PR Comment: https://git.openjdk.org/jdk/pull/19427#issuecomment-2135446758
Re: RFR: 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache'
On Tue, 28 May 2024 14:01:44 GMT, Martin Doerr wrote: > Fix obvious typo in micro benchmark. Looks good and trivial! - Marked as reviewed by chagedorn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19427#pullrequestreview-2083086912
Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]
On Fri, 24 May 2024 16:36:32 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated on 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. >> >> Updated on 5/18/2024 >> >> Withdraw changes to jaxp.properties. The original idea was to match >> jaxp-strict.properties with regard to the properties. However, that change >> impact the configuration process, resulting in tests that verify the process >> to fail. >> >> Updated on 5/23/2024 >> >> Provide a template `jaxp-strict.template` instead of a properties file. This >> template can be used to create custom configuration files. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > rename the template to jaxp-strict.properties.template Just some wording suggestions. src/java.xml/share/conf/jaxp-strict.properties.template line 36: > 34: # This property determines whether XSLT and XPath extension functions are > allowed. > 35: # The value type is boolean and the default value is true (allowing > 36: # extension functions). The following entry would override the default > value and I prefer the latter wording suggestion as in other places like the Limits section, you use present tense, which I also think sounds better. s/would/will/ or s/would override/overrides/ s/disallow/disallows/ src/java.xml/share/conf/jaxp-strict.properties.template line 44: > 42: # Overriding the default parser: > 43: # > 44: # This property allows using a third party implementation to override the > default s/using a third party implementation/a third party implementation/ src/java.xml/share/conf/jaxp-strict.properties.template line 60: > 58: # strict -- indicates that the resolver should throw a > CatalogException > 59: # > 60: # The following setting would cause the resolve to throw a > CatalogException when s/would/will/ or s/would cause/causes/ s/resolve/CatalogResolver/ ? src/java.xml/share/conf/jaxp-strict.properties.template line 67: > 65: # Implementation Specific Properties - DTD > 66: # > 67: # This property instructs the parsers to: deny, ignore or allow DTD > processing. s/to:/to/ src/java.xml/share/conf/jaxp-strict.properties.template line 68: > 66: # > 67: # This property instructs the parsers to: deny, ignore or allow DTD > processing. > 68: # The following setting would cause the parser to reject DTD by throwing > an exception. s/would/will/ or s/would cause/causes/ s/DTD/DTDs/ src/java.xml/share/conf/jaxp.properties line 1: > 1: > Similar wording suggestions as in the `jaxp-strict.properties.template` file. - PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2083052633 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1617388223 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1617389854 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1617392917 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1617394277 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1617395992 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1617407662
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v9]
> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use > classfile API for reflection proxy-generation. Actual implementation of > `ProxyGenerator` is focused on performance, however it causes JDK bootstrap > regressions. `ProxyGenerator.TEMPLATE` class model is statically created and > each proxy class is transformed from the template. > > This patch is intended to examine plain proxy generation impact on > performance and JDK bootstrap (vs proxy transformation from template). > > The generated proxy is migrated from static initialization to CONDY bootstrap. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: removed obsolete entry - Changes: - all: https://git.openjdk.org/jdk/pull/19410/files - new: https://git.openjdk.org/jdk/pull/19410/files/fe596465..b3f6be89 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19410=08 - incr: https://webrevs.openjdk.org/?repo=jdk=19410=07-08 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19410.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19410/head:pull/19410 PR: https://git.openjdk.org/jdk/pull/19410
RFR: 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache'
Fix obvious typo in micro benchmark. - Commit messages: - 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache' Changes: https://git.openjdk.org/jdk/pull/19427/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19427=00 Issue: https://bugs.openjdk.org/browse/JDK-8332228 Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19427.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19427/head:pull/19427 PR: https://git.openjdk.org/jdk/pull/19427
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v8]
On Tue, 28 May 2024 11:55:30 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > MTD_ cleanup Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19410#pullrequestreview-2082875192
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v7]
On Tue, 28 May 2024 12:27:46 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 >> >> Thanks >> Erik > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Reduce clutter I think this looks okay for now and great to see the instrumentation going away. Follow-on work for the future will be to re-examine the issue with the plan read of the flag (which seems to have previously existed) and to see if we reduce this down to one test rather than two. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19129#pullrequestreview-2082828370
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 Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/16809#pullrequestreview-2082798275
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v8]
On Tue, 28 May 2024 11:55:30 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > MTD_ cleanup Nice work migrating method object initialization to condy. - PR Comment: https://git.openjdk.org/jdk/pull/19410#issuecomment-2135212751
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 Please review! This verifier extension is actually blocking new `javap` feature for 23, see: https://github.com/openjdk/jdk/pull/18629 Missing all of these class file verifications in `javap -verify` would be sad. Thank you! Adam - PR Comment: https://git.openjdk.org/jdk/pull/16809#issuecomment-2135209023
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Fri, 24 May 2024 15:45:07 GMT, Erik Gahlin wrote: >> Collapsing the extra layer of methods and combining the test into >> >> if (jfrTracing && FileReadEvent.enabled()) >> >> would indeed keep things a little neater. >> >> I'm still questioning the need for `jfrTracing` at all. There's the matter >> of how this boolean gets set and unset, and whether this is done in a way >> that comports with the memory model. Setting this aside, is the only benefit >> that it avoids loading an extra class at JVM startup time (without JFR)? In >> this case that would be the `FileReadEvent` class, which is the stub class >> in `jdk.internal.event`. Wouldn't this class be in the CDS archive, making >> it not worth the effort of bringing up this new `jfrTracing` mechanism >> simply to avoid loading it unnecessarily? >> >> I observe that in JDK 22 some (but not all) of the event classes in >> `jdk.internal.event` seem to be present in the CDS archive. These include >> various virtual thread events. > > I don't think the issue is so much the overhead of loading (one or more) > additional classes without JFR, even if it causes an extremely small > regression, but the added overhead to JFR when trying to fix the regression. > The cost of a JVMTI retransformation is perhaps 100 - 1000 times that of > loading a class in the CDS archive. > > I did an experiment with: > > `if (FlightRecorder.enabled && FileReadEvent.enabled())` > > and it passes JFR tests and tier1/tier2. I don't think > `FlightRecorder.enabled` needs to be used on every event class, but it would > be good to use on event classes loaded during startup, both for safety > reasons (ClassCircularityError) and to lower overhead of JFR startup. The > `jfrTracing` flag works as well, but it is perhaps harder to comprehend and > requires an additional static boolean in every class, which does clutter > things up. > > I can push Alan's suggestion of uniting the checks into one if-statement. It > may help to see how it looks. > > Virtual thread events are typically loaded in main, after JFR has started, > and not an issue unless `jcmd JFR.start` is used, which is not that common. Updated with Alan's suggestion to make the code more readable. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1617228065
Integrated: 8332064: Implementation of Structured Concurrency (Third Preview)
On Fri, 10 May 2024 10:58:42 GMT, Alan Bateman wrote: > There aren't any API or implementations changes in third preview but the JEP > number/title needs to be bumped for the javadoc page. This pull request has now been integrated. Changeset: e708d135 Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/e708d135e3af7e0652cdbb680388a0735582ba74 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8332064: Implementation of Structured Concurrency (Third Preview) Reviewed-by: jpai, bpb, mcimadamore - PR: https://git.openjdk.org/jdk/pull/19175
Re: RFR: 8332597: Remove redundant methods from j.l.classfile.ClassReader API [v2]
On Fri, 24 May 2024 16:41:33 GMT, Adam Sotona wrote: >> j.l.classfile.ClassReader instance is exposed in the Class-File API through >> j.l.classfile.AttributeMapper::readAttribute method only. >> ClassReader only purpose is to serve as a tool for reading content of a >> custom attribute in a user-provided AttribtueMapper. >> It contains useful set of low-level class reading methods for user to >> implement a custom attribute content parser. >> >> However methods ClassReader::thisClassPos and >> ClassReader::skipAttributeHolder are not necessary for a custom attribute >> content parsing and so redundant in the API. >> Class-File API implementation internally use these methods, however they >> should not be exposed in the API. >> >> This patch removes the methods from the API. >> >> 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 two commits: > > - Merge branch 'master' into JDK-8332597-classreader-redundancy > ># Conflicts: ># src/java.base/share/classes/jdk/internal/classfile/impl/ClassImpl.java > - 8332597: Remove redundant methods from j.l.classfile.ClassReader API Marked as reviewed by jlahoda (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19323#pullrequestreview-2082655142
Integrated: 8332505: JEP 457: ClassRemapper forgets to remap bootstrap method references
On Mon, 20 May 2024 08:03:28 GMT, Adam Sotona wrote: > Class-File API `ClassRemapper` component suppose to remap all classes > referenced in a class file. > Actual implementation missed remapping of bootstrap methods referenced from > `invokedynamic` instructions. > This patch fixes the remapping and adds relevant test. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: aa4c83a5 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/aa4c83a5bfe146714a46fb454aafc7393d2d8453 Stats: 9 lines in 2 files changed: 4 ins; 0 del; 5 mod 8332505: JEP 457: ClassRemapper forgets to remap bootstrap method references Reviewed-by: jlahoda - PR: https://git.openjdk.org/jdk/pull/19299
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v7]
> 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 > > Thanks > Erik Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Reduce clutter - Changes: - all: https://git.openjdk.org/jdk/pull/19129/files - new: https://git.openjdk.org/jdk/pull/19129/files/8a44b649..87647f9d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19129=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19129=05-06 Stats: 63 lines in 4 files changed: 0 ins; 47 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/19129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19129/head:pull/19129 PR: https://git.openjdk.org/jdk/pull/19129
Re: RFR: 8332505: JEP 457: ClassRemapper forgets to remap bootstrap method references
On Mon, 20 May 2024 08:03:28 GMT, Adam Sotona wrote: > Class-File API `ClassRemapper` component suppose to remap all classes > referenced in a class file. > Actual implementation missed remapping of bootstrap methods referenced from > `invokedynamic` instructions. > This patch fixes the remapping and adds relevant test. > > Please review. > > Thanks, > Adam Looks reasonable to me. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19299#pullrequestreview-2082624013
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v8]
> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use > classfile API for reflection proxy-generation. Actual implementation of > `ProxyGenerator` is focused on performance, however it causes JDK bootstrap > regressions. `ProxyGenerator.TEMPLATE` class model is statically created and > each proxy class is transformed from the template. > > This patch is intended to examine plain proxy generation impact on > performance and JDK bootstrap (vs proxy transformation from template). > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: MTD_ cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/19410/files - new: https://git.openjdk.org/jdk/pull/19410/files/22e87768..fe596465 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19410=07 - incr: https://webrevs.openjdk.org/?repo=jdk=19410=06-07 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19410.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19410/head:pull/19410 PR: https://git.openjdk.org/jdk/pull/19410
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v7]
> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use > classfile API for reflection proxy-generation. Actual implementation of > `ProxyGenerator` is focused on performance, however it causes JDK bootstrap > regressions. `ProxyGenerator.TEMPLATE` class model is statically created and > each proxy class is transformed from the template. > > This patch is intended to examine plain proxy generation impact on > performance and JDK bootstrap (vs proxy transformation from template). > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/19410/files - new: https://git.openjdk.org/jdk/pull/19410/files/c407e40e..22e87768 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19410=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19410=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19410.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19410/head:pull/19410 PR: https://git.openjdk.org/jdk/pull/19410
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v6]
> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use > classfile API for reflection proxy-generation. Actual implementation of > `ProxyGenerator` is focused on performance, however it causes JDK bootstrap > regressions. `ProxyGenerator.TEMPLATE` class model is statically created and > each proxy class is transformed from the template. > > This patch is intended to examine plain proxy generation impact on > performance and JDK bootstrap (vs proxy transformation from template). > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: CONDY implementation of ProxyGenerator - Changes: - all: https://git.openjdk.org/jdk/pull/19410/files - new: https://git.openjdk.org/jdk/pull/19410/files/3d975d28..c407e40e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19410=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19410=04-05 Stats: 69 lines in 1 file changed: 8 ins; 39 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19410.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19410/head:pull/19410 PR: https://git.openjdk.org/jdk/pull/19410
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
On Fri, 24 May 2024 17:57:28 GMT, Oussama Louati wrote: >> test/jdk/java/lang/invoke/indify/Indify.java line 660: >> >>> 658: * >>> 659: * @return true if any marks were changed, false otherwise. >>> 660: */ >> >> This method does incremental analysis of the constant pool. >> With Class-File API CP model it can be done in one pass, or even in no-pass >> and all requests to "marks" can be answered directly. > > Thank you for the feedback. I will look into the Class-File API CP model and > apply your suggestion to perform the analysis in one pass or handle requests > to "marks" directly. Done - PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1617070598
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v6]
> 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 > > Thanks > Erik Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Fix typos - Changes: - all: https://git.openjdk.org/jdk/pull/19129/files - new: https://git.openjdk.org/jdk/pull/19129/files/c4c64774..8a44b649 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19129=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19129=04-05 Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/19129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19129/head:pull/19129 PR: https://git.openjdk.org/jdk/pull/19129
Re: RFR: 8307818: Convert Indify tool to Classfile API [v10]
> An indify tool in j.l.i tests (also in vmTestBase) convert some source-code > private static methods with MT_ MH_, and INDY_ prefixes into MethodHandle, > MethodType, and CallSite constants. > It currently uses ad-hoc code to process class files and intends to migrate > to ASM; but since we have the Classfile API, we can migrate to Classfile API > instead. Oussama Louati has updated the pull request incrementally with one additional commit since the last revision: chore: changed pool marking initialization to be done in one pass - Changes: - all: https://git.openjdk.org/jdk/pull/18841/files - new: https://git.openjdk.org/jdk/pull/18841/files/2b8c94a7..0cac912d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18841=09 - incr: https://webrevs.openjdk.org/?repo=jdk=18841=08-09 Stats: 91 lines in 1 file changed: 9 ins; 53 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/18841.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18841/head:pull/18841 PR: https://git.openjdk.org/jdk/pull/18841
Integrated: 8292955: Collections.checkedMap Map.merge does not properly check key and value
On Wed, 6 Mar 2024 18:26:16 GMT, Lei Zhu wrote: > When the specified key did not associated with a value, should check the > `key` and `value` type. This pull request has now been integrated. Changeset: b5e1615c Author:Korov Committer: Per Minborg URL: https://git.openjdk.org/jdk/commit/b5e1615c0084538f2161fe9b56748d188983e972 Stats: 12 lines in 2 files changed: 9 ins; 0 del; 3 mod 8292955: Collections.checkedMap Map.merge does not properly check key and value Reviewed-by: gli, liach, pminborg - PR: https://git.openjdk.org/jdk/pull/18141
Re: RFR: 8292955: Collections.checkedMap Map.merge does not properly check key and value [v3]
On Thu, 7 Mar 2024 05:33:16 GMT, Lei Zhu wrote: >> When the specified key did not associated with a value, should check the >> `key` and `value` type. > > Lei Zhu has updated the pull request incrementally with one additional commit > since the last revision: > > Use testNG builtin functionalities and modify the test function name Adding the checks before remapping seems the right thing to do in order to uphold the checked maps guarantees on types and values. - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18141#pullrequestreview-2081881249
Re: RFR: 8292955: Collections.checkedMap Map.merge does not properly check key and value [v3]
On Mon, 27 May 2024 12:40:07 GMT, Chen Liang wrote: > @minborg Is it possible for you to review this collection bugfix? Will do! Thanks for the heads up. - PR Comment: https://git.openjdk.org/jdk/pull/18141#issuecomment-2134446236
