Re: RFR: 8318971 : Better Error Handling for Jar Tool When Processing Non-existent Files [v4]
On Wed, 20 Dec 2023 16:29:59 GMT, Weibing Xiao wrote: >> Better Error Handling for Jar Tool Processing of "@File" >> >> This is a new PR for this PR since the original developer left the team. See >> all of the review history at https://github.com/openjdk/jdk/pull/16423. >> >> Thank you. > > Weibing Xiao has updated the pull request incrementally with one additional > commit since the last revision: > > update the test cases Marked as reviewed by jpai (Reviewer). The current state in `ff349833` looks good to me. Thank you for these updates. Please run tier1, tier2, tier3 before integrating. - PR Review: https://git.openjdk.org/jdk/pull/17088#pullrequestreview-1792326348 PR Comment: https://git.openjdk.org/jdk/pull/17088#issuecomment-1865680335
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v3]
On Thu, 21 Dec 2023 01:12:07 GMT, Adam Sotona wrote: >> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy >> classes. >> >> This patch converts it to use Classfile API. >> >> It is continuation of https://github.com/openjdk/jdk/pull/10991 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with two additional > commits since the last revision: > > - performance improvements > - SplitConstantPool performance fix Here are actual numbers (_22 is before this patch): Benchmark Mode Cnt Score Error Units ProxyPerf.genIntf_1avgt5 18354.110 ? 696.706 ns/op ProxyPerf.genIntf_1_22 avgt5 15744.305 ? 1032.092 ns/op ProxyPerf.genStringsIntf_3 avgt5 26388.493 ? 2614.877 ns/op ProxyPerf.genStringsIntf_3_22 avgt5 21431.752 ? 398.627 ns/op ProxyPerf.genZeroParamsavgt5 17627.978 ? 456.046 ns/op ProxyPerf.genZeroParams_22 avgt5 15411.519 ? 6426.321 ns/op ProxyPerf.getPrimsIntf_2 avgt5 23862.592 ? 2274.386 ns/op ProxyPerf.getPrimsIntf_2_22avgt5 19148.768 ? 648.304 ns/op Finished running test 'micro:.+ProxyPerf.+' - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1865370450
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v4]
On Wed, 20 Dec 2023 20:39:55 GMT, Mandy Chung wrote: > Can you include the performance comparison before and after this change and > what performance benchmarks you run? I've run two batches on Aurora with mixed results (linked to [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960)): `org.openjdk.bech.java.lang.invoke.+` and startup benchmarks I can attach some of them here if needed. - PR Comment: https://git.openjdk.org/jdk/pull/17108#issuecomment-1865359565
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v3]
On Thu, 21 Dec 2023 01:12:07 GMT, Adam Sotona wrote: >> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy >> classes. >> >> This patch converts it to use Classfile API. >> >> It is continuation of https://github.com/openjdk/jdk/pull/10991 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with two additional > commits since the last revision: > > - performance improvements > - SplitConstantPool performance fix Profiling of the benchmarks revealed several slowdowns: - many expensive conversions from `Class` to `ClassDesc` to `ClassEntry`, or even more expensive `MethodTypeDesc` - building proxy class from scratch from symbols also involves a lot of `String` concatenations, hashing, encoding and comparisons - computation of stack maps is also still expensive - `SplitConstantPool` was ineffective in some specific cases Proposed performance improvements use pre-generated template class and each proxy is then transformed with share constant pool. Frequently used constant pool entries are materialized in the template class constant pool to avoid expensive conversions, comparisons and hashing. Stack maps are generated manually. Performance fixes related to ClassFile API `SplitConstantPool` and `StackCounter` may be separated into another PR. For now I keep them together to measure synergic effect. Full benchmark numbers will be ready soon. - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1865353203
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v3]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - performance improvements - SplitConstantPool performance fix - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/a21fb264..75e31e3a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=01-02 Stats: 76 lines in 2 files changed: 21 ins; 10 del; 45 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 22:46:32 GMT, Markus KARG wrote: > So I did not do something wrong but finally I learned Github forensics now. If no crime was committed, I would call it archaeology. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1865249242
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 18:49:39 GMT, Brian Burkhalter wrote: > > this icon only says that Alan made change requests > > I think it's if the "changes requested" button is selected in the "Submit > Review" popup under "Files Changed." Typically yes. But not in the current case: Alan pressed "approve" first *without* giving a comment leading to this entry:  ...then...  ...and **after that** Alan pressed `Request changes` button in the review dialog (note the heading "Alan Bateman suggested changes last week"):  So the comment "I think jai is right" was the trigger to set that icon, but that icon was definitively **not** set before. So I did not do something wrong but finally I learned Github forensics now. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1865242504
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v2]
On Wed, 29 Nov 2023 15:01:32 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:
>
> Only use optimization when EnableX86ECoreOpts is true
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 42:
> 40: // 2. Broadcast the last byte of the needle to a different ymm register
> 41: // 3. Compare the first-byte ymm register to the first 32 bytes of the
> haystack
> 42: // 4. Compare the last-byte register to the 32 bytes of the haystack at
> the (k-1)st position
It would be good to mention that k is the length of the needle.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 117:
> 115:
> /**/
> 116:
> 117: void StubGenerator::loop_helper(int size, Label& bailout, Label&
> loop_top) {
Let us name this method as string_indexof_loop_helper() instead of just
loop_helper().
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1409626570
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1409623608
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v4]
On Tue, 19 Dec 2023 18:42:19 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 for JDK-8321599
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2543:
> 2541:
> 2542: // Strip pad characters, if any, and adjust length and mask
> 2543: __ addq(length, start_offset);
This change is unrelated to this PR.
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2548:
> 2546:
> 2547: __ BIND(L_donePadding);
> 2548: __ subq(length, start_offset);
This change is unrelated to this PR.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 3:
> 1: /*
> 2: * Copyright (c) 2023, Intel Corporation. All rights reserved.
> 3: * Intel Math Library (LIBM) Source Code
Please remove the line "Intel Math Library ..." as this is not from there.
test/jdk/java/lang/StringBuffer/IndexOf.java line 45:
> 43: System.err.println(gg);
> 44:
> 45: } else {
Some changes in this test file looks like a leftover from debugging.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1433245292
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1433245471
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1433245843
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1433247486
Re: RFR: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called [v2]
On Wed, 20 Dec 2023 21:52:40 GMT, Roger Riggs wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Clear sooner
>
> src/java.base/share/classes/java/lang/StringBuffer.java line 719:
>
>> 717: public synchronized StringBuffer repeat(int codePoint, int count) {
>> 718: super.repeat(codePoint, count);
>> 719: toStringCache = null;
>
> The other cases in StringBuffer clear toStringCache *before* performing the
> modification.
> For consistency, I'd suggest doing the same.
Changed
> src/java.base/share/classes/java/lang/StringBuffer.java line 731:
>
>> 729: public synchronized StringBuffer repeat(CharSequence cs, int count)
>> {
>> 730: super.repeat(cs, count);
>> 731: toStringCache = null;
>
> Ditto, clear toStringCache before the modification.
Changed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17172#discussion_r1433234794
PR Review Comment: https://git.openjdk.org/jdk/pull/17172#discussion_r1433234928
Re: RFR: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called [v2]
> The new repeat methods were not clearing the toStringCache. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Clear sooner - Changes: - all: https://git.openjdk.org/jdk/pull/17172/files - new: https://git.openjdk.org/jdk/pull/17172/files/82b7e342..66eb7f71 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17172=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17172=00-01 Stats: 4 lines in 1 file changed: 2 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17172.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17172/head:pull/17172 PR: https://git.openjdk.org/jdk/pull/17172
Re: RFR: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called [v2]
On Wed, 20 Dec 2023 22:15:04 GMT, Jim Laskey wrote: >> The new repeat methods were not clearing the toStringCache. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Clear sooner LGTM. - Marked as reviewed by mk...@github.com (no known OpenJDK username). PR Review: https://git.openjdk.org/jdk/pull/17172#pullrequestreview-1791741679
Re: RFR: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called [v2]
On Wed, 20 Dec 2023 22:15:04 GMT, Jim Laskey wrote: >> The new repeat methods were not clearing the toStringCache. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Clear sooner LGTM - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17172#pullrequestreview-1791741828
Integrated: 8322078: ZipSourceCache.testKeySourceMapping() test fails with The process cannot access the file because it is being used by another process
On Wed, 20 Dec 2023 15:37:37 GMT, Sean Coffey wrote: > trivial test edit to use FileUtils.deleteFileIfExistsWithRetry test library > API > > tested on CI system with test-repeat = 20 - no issues seen. This pull request has now been integrated. Changeset: f6fe39ff Author:Sean Coffey URL: https://git.openjdk.org/jdk/commit/f6fe39ff1168d27f4d0ea3e4c7f3f17ecae9e1ab Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod 8322078: ZipSourceCache.testKeySourceMapping() test fails with The process cannot access the file because it is being used by another process Reviewed-by: lancea - PR: https://git.openjdk.org/jdk/pull/17169
Re: RFR: 8322078: ZipSourceCache.testKeySourceMapping() test fails with The process cannot access the file because it is being used by another process
On Wed, 20 Dec 2023 15:37:37 GMT, Sean Coffey wrote: > trivial test edit to use FileUtils.deleteFileIfExistsWithRetry test library > API > > tested on CI system with test-repeat = 20 - no issues seen. Thanks for the review Lance. I didn't use try-with-resources during test creation in those locations since the timing of the close() operation is key to the test. Being able to control when the files are closed allows extra sanity checks to be performed. - PR Comment: https://git.openjdk.org/jdk/pull/17169#issuecomment-1865200537
Re: RFR: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called
On Wed, 20 Dec 2023 20:25:07 GMT, Jim Laskey wrote:
> The new repeat methods were not clearing the toStringCache.
src/java.base/share/classes/java/lang/StringBuffer.java line 719:
> 717: public synchronized StringBuffer repeat(int codePoint, int count) {
> 718: super.repeat(codePoint, count);
> 719: toStringCache = null;
The other cases in StringBuffer clear toStringCache *before* performing the
modification.
For consistency, I'd suggest doing the same.
src/java.base/share/classes/java/lang/StringBuffer.java line 731:
> 729: public synchronized StringBuffer repeat(CharSequence cs, int count) {
> 730: super.repeat(cs, count);
> 731: toStringCache = null;
Ditto, clear toStringCache before the modification.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17172#discussion_r1433218351
PR Review Comment: https://git.openjdk.org/jdk/pull/17172#discussion_r1433218688
[jdk22] Integrated: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 20 Dec 2023 19:00:50 GMT, Alisen Chung wrote: > Backport of JDK-8322041 This pull request has now been integrated. Changeset: a05f3d10 Author:Alisen Chung URL: https://git.openjdk.org/jdk22/commit/a05f3d10cc374297fd9b0fa27305c9b26f8081d2 Stats: 290 lines in 36 files changed: 166 ins; 15 del; 109 mod 8322041: JDK 22 RDP1 L10n resource files update Reviewed-by: kcr, naoto Backport-of: b061b6678fde891974d5b58cec963b3481099a8d - PR: https://git.openjdk.org/jdk22/pull/22
[jdk22] RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable
Hi all, This pull request contains a backport of commit [0f8e4e0a](https://github.com/openjdk/jdk/commit/0f8e4e0a81257c678e948c341a241dc0b810494f) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Serguei Spitsyn on 19 Dec 2023 and was reviewed by Leonid Mesnik and Alan Bateman. Thanks! - Commit messages: - Backport 0f8e4e0a81257c678e948c341a241dc0b810494f Changes: https://git.openjdk.org/jdk22/pull/23/files Webrev: https://webrevs.openjdk.org/?repo=jdk22=23=00 Issue: https://bugs.openjdk.org/browse/JDK-8311218 Stats: 229 lines in 15 files changed: 196 ins; 0 del; 33 mod Patch: https://git.openjdk.org/jdk22/pull/23.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/23/head:pull/23 PR: https://git.openjdk.org/jdk22/pull/23
Re: RFR: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile [v8]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip`, > `ZipFile/input.jar` and `ZipFile/crash.jar` > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired or moved into other test files as separate > methods. See comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. > After discussion, we decided to merge this test into `ReadZip.java`. I added > some checks to verify that reading from the stream reduces the number of > available bytes accordingly, also checking the behavior when the stream is > closed. > > `CopyJar.java` > The concern of copying entries seems to now have better coverage in the test > `zip/CopyZipFile`. We decided to retire this test rather than convert it and > instead convert `CopyZipFile` to JUnit. > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > We decided to merge this test into `ReadZip.readDirectoryEntries` rather than > keeping it as a separate test. > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was using TestNG so was converted to JUnit for consistency. Added > some comments to help understanding. > > `ReadAfterClose.java` > This uses the binary test vector `crash.jar`. The test is converted to JUnit > and moved to `ReadZip.readAfterClose´. The test is expanded to exercise more > read methods. Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 14 additional commits since the last revision: - Merge branch 'master' into readzip-junit - Merge the ReadAfterClose test into ReadZip, converting it to JUnit. - Improve comment explaining what happens when ZipEntry.setCompressedSize is calledExplicitly - Convert CopyZipFile.java to JUnit - Remove trailing whitespace - Merge GetDirEntry.java into ReadZip.readDirectoryEntries() - Retire redundant test ZipFile/CopyJar - Merge Available.java into ReadZip.java - Add @bug reference 4401122 on the Available.java test - Merge branch 'master' into readzip-junit - ... and 4 more: https://git.openjdk.org/jdk/compare/82804792...85500cd8 - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/27fe1131..85500cd8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17038=07 - incr: https://webrevs.openjdk.org/?repo=jdk=17038=06-07 Stats: 3896 lines in 272 files changed: 2360 ins; 631 del; 905 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
RFR: 8322565 (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits
This PR suggests that `Files.setPosixPermissions`as implemented by `ZipFileSystem` should preserve the leading seven bits of the 'external file attributes' field. These bits contain the 'file type', 'setuid', 'setgid', and 'sticky' bits. These are unrelated to permissions and should not be modified by this operation. The fix is to update `Entry.readCEN` to read all 16 bits and to update `ZipFileSystem.setPermissions` to preserve the leading 7 bits when updating the trailing 9 permission-related bits of the `Entry.posixPerms` field. The PR adds a new test `TestPosix.preserveRemainingBits()` which verifies that the leading 7 bits are not affected by `Files.setPosixPermissions`. Note that this PR does not aim to preserve the leading seven bits for the case when `Files.setPosixPermissions` is called with a `null` permission set. (The implementation currently interprets this as a signal that the 'external file attributes' should not be populated and the 'version made by' OS will be MSDOS instead of Unix) - Commit messages: - Preserve non-permission 'external file attributes' bits when setting posix permissions Changes: https://git.openjdk.org/jdk/pull/17170/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17170=00 Issue: https://bugs.openjdk.org/browse/JDK-8322565 Stats: 83 lines in 2 files changed: 75 ins; 4 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17170.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17170/head:pull/17170 PR: https://git.openjdk.org/jdk/pull/17170
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 14:15:48 GMT, Alan Bateman wrote: > Update: ignore this I mis-read that it updates the current thread's suspend > value, not the thread's suspend value. Thanks, Alan. I've also got confused with this and even filed a follow up bug. :) Yes, the initial design was the `_is_disable_suspend` is set/modified/accessed on the current thread only. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1433182764
Re: [jdk22] RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 20 Dec 2023 19:00:50 GMT, Alisen Chung wrote: > Backport of JDK-8322041 Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk22/pull/22#pullrequestreview-1791633961
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v4]
On Mon, 18 Dec 2023 11:03:10 GMT, Adam Sotona wrote: >> java.base java.lang.invoke package heavily uses ASM to generate lambdas and >> method handles. >> >> This patch converts ASM calls to Classfile API. >> >> This PR is continuation of https://github.com/openjdk/jdk/pull/12945 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Apply suggestions from code review > > Co-authored-by: liach <7806504+li...@users.noreply.github.com> Can you include the performance comparison before and after this change and what performance benchmarks you run? - PR Comment: https://git.openjdk.org/jdk/pull/17108#issuecomment-1865109693
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v2]
On Wed, 20 Dec 2023 20:08:08 GMT, Adam Sotona wrote: >> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy >> classes. >> >> This patch converts it to use Classfile API. >> >> It is continuation of https://github.com/openjdk/jdk/pull/10991 >> >> Any comments and suggestions are welcome. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with three additional > commits since the last revision: > > - performance improvements > - StackCounter performance boost > - performance improvements Can you explain the changes to resolve the performance regressions? - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1865107527
RFR: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called
The new repeat methods were not clearing the toStringCache. - Commit messages: - Clear toStringCache when calling StringBuffer::repeat Changes: https://git.openjdk.org/jdk/pull/17172/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17172=00 Issue: https://bugs.openjdk.org/browse/JDK-8322512 Stats: 16 lines in 2 files changed: 15 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17172.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17172/head:pull/17172 PR: https://git.openjdk.org/jdk/pull/17172
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes
On Tue, 19 Dec 2023 20:45:58 GMT, Mandy Chung wrote: > Can you include a summary of the performance comparison before and after the > patch per the > [comment](https://github.com/openjdk/jdk/pull/10991#pullrequestreview-1333426016) > in #10991. I've found significant performance regressions when running these recommended benchmark. Performance improvements are now in progress. Thanks for the reminder. - PR Comment: https://git.openjdk.org/jdk/pull/17121#issuecomment-1865062212
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v2]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes. > > This patch converts it to use Classfile API. > > It is continuation of https://github.com/openjdk/jdk/pull/10991 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with three additional commits since the last revision: - performance improvements - StackCounter performance boost - performance improvements - Changes: - all: https://git.openjdk.org/jdk/pull/17121/files - new: https://git.openjdk.org/jdk/pull/17121/files/0267d657..a21fb264 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17121=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17121=00-01 Stats: 338 lines in 2 files changed: 166 ins; 33 del; 139 mod Patch: https://git.openjdk.org/jdk/pull/17121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17121/head:pull/17121 PR: https://git.openjdk.org/jdk/pull/17121
Re: [jdk22] RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 20 Dec 2023 19:00:50 GMT, Alisen Chung wrote: > Backport of JDK-8322041 The patch looks good. Additionally, I confirmed that the files in question are identical to those in jdk mainline after this patch is applied. It will need a "R"eviewer to approve. - Marked as reviewed by kcr (Author). PR Review: https://git.openjdk.org/jdk22/pull/22#pullrequestreview-1791515826
[jdk22] RFR: 8322041: JDK 22 RDP1 L10n resource files update
Backport of JDK-8322041 - Commit messages: - Backport b061b6678fde891974d5b58cec963b3481099a8d Changes: https://git.openjdk.org/jdk22/pull/22/files Webrev: https://webrevs.openjdk.org/?repo=jdk22=22=00 Issue: https://bugs.openjdk.org/browse/JDK-8322041 Stats: 290 lines in 36 files changed: 166 ins; 15 del; 109 mod Patch: https://git.openjdk.org/jdk22/pull/22.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/22/head:pull/22 PR: https://git.openjdk.org/jdk22/pull/22
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 18:28:27 GMT, Markus KARG wrote: > this icon only says that Alan made change requests I think it's if the "changes requested" button is selected in the "Submit Review" popup under "Files Changed." - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864976239
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 17:57:57 GMT, Brian Burkhalter wrote: > > But where do you see a change request from Alan _before_ I posted > > `/integrate`? > > Mouse-over the icon to the right of his name at the top right under > "Reviewers." I might be wrong, but IMHO this icon only says that Alan made change requests, but not *when*. This means, the icon pops up even when requesting the changes *after* approving. But I again, I might be wrong. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864945556
Re: RFR: 8275338: Add JFR events for notable serialization situations [v8]
> Adds serialization misdeclaration events to JFR. Raffaello Giulietti 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 11 additional commits since the last revision: - Merge branch 'master' into 8275338 - Renamed an event field. - Minor changes. - Removed event kind. serialVersionUID must have type long. Test now base on keyword search in event message. Commented test classes about misdeclarations. - Changes according to reviewer's comments. - Better name for a label, corrected name of removed field. - Event enabled on profile.jfc but disabled on default.jfc. - Restrict accessibility of nested classes. - Make use of EventNames. - Merge branch 'master' into 8275338 - ... and 1 more: https://git.openjdk.org/jdk/compare/6ea0f368...94fc01ba - Changes: - all: https://git.openjdk.org/jdk/pull/17129/files - new: https://git.openjdk.org/jdk/pull/17129/files/0cab1c1b..94fc01ba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17129=07 - incr: https://webrevs.openjdk.org/?repo=jdk=17129=06-07 Stats: 2512 lines in 172 files changed: 1722 ins; 321 del; 469 mod Patch: https://git.openjdk.org/jdk/pull/17129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17129/head:pull/17129 PR: https://git.openjdk.org/jdk/pull/17129
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Sat, 16 Dec 2023 14:07:52 GMT, Markus KARG wrote: >> Fixes JDK-8322141 >> >> As suggested by @vlsi and documented by @bplb this PR does not return, but >> only sets the maximum result value. > > Markus KARG has updated the pull request incrementally with one additional > commit since the last revision: > > Test was using Integer but must use Long > But where do you see a change request from Alan _before_ I posted > `/integrate`? Mouse-over the icon to the right of his name at the top right under "Reviewers." > BTW, here is the "Ready" label you did not see: [#17119 > (comment)](https://github.com/openjdk/jdk/pull/17119#event-11256913866) Ack. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864901444
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 16:52:38 GMT, Brian Burkhalter wrote: > > Alan actually did approve them on December 15. > > I know, with requested changes. I didn't see the request transition to > "Ready" which is where it needs to be to `/integrate`. Looking at the Github history I need to say that I cannot see that Alan approved *with change requests*. Maybe I am missing something. I see his approval on Dec 15 directly before my `/integrate` on the same day, but no change request *there*, and I can see his change request *after* my `/integrate` where he says "I think jai is right". But where do you see a change request from Alan *before* I posted `/integrate`? I am pretty sure you are right, but where actually do you see this in the Github history? Thanks. :-) BTW, here is the "Ready" label you did not see: https://github.com/openjdk/jdk/pull/17119#event-11256913866 - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864893934
Re: RFR: 8322078: ZipSourceCache.testKeySourceMapping() test fails with The process cannot access the file because it is being used by another process
On Wed, 20 Dec 2023 15:37:37 GMT, Sean Coffey wrote: > trivial test edit to use FileUtils.deleteFileIfExistsWithRetry test library > API > > tested on CI system with test-repeat = 20 - no issues seen. Hi Sean, Thank you for tackling this. Would it be beneficial to use a try-with-resources for the zip file opened on lines 88 and 98 just to rule out any other weirdness on windows? - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17169#pullrequestreview-1791382251
Re: RFR: 8275338: Add JFR events for notable serialization situations [v7]
> Adds serialization misdeclaration events to JFR. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Minor changes. - Changes: - all: https://git.openjdk.org/jdk/pull/17129/files - new: https://git.openjdk.org/jdk/pull/17129/files/a6f80250..0cab1c1b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17129=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17129=05-06 Stats: 10 lines in 2 files changed: 0 ins; 8 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17129/head:pull/17129 PR: https://git.openjdk.org/jdk/pull/17129
Integrated: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred
On Fri, 15 Dec 2023 08:23:58 GMT, Markus KARG wrote: > Fixes JDK-8322141 > > As suggested by @vlsi and documented by @bplb this PR does not return, but > only sets the maximum result value. This pull request has now been integrated. Changeset: 2d609557 Author:Markus KARG Committer: Brian Burkhalter URL: https://git.openjdk.org/jdk/commit/2d609557ffe8e15ab81fb51dce90e40780598371 Stats: 50 lines in 2 files changed: 48 ins; 0 del; 2 mod 8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred Reviewed-by: vsitnikov, bpb, jpai - PR: https://git.openjdk.org/jdk/pull/17119
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 00:56:16 GMT, Jaikiran Pai wrote: > The updated source change looks fine to me. @jaikiran Thanks for the corroboration. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864827299
Re: RFR: 8275338: Add JFR events for notable serialization situations [v6]
> Adds serialization misdeclaration events to JFR. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Removed event kind. serialVersionUID must have type long. Test now base on keyword search in event message. Commented test classes about misdeclarations. - Changes: - all: https://git.openjdk.org/jdk/pull/17129/files - new: https://git.openjdk.org/jdk/pull/17129/files/fa04bf48..a6f80250 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17129=05 - incr: https://webrevs.openjdk.org/?repo=jdk=17129=04-05 Stats: 285 lines in 4 files changed: 87 ins; 96 del; 102 mod Patch: https://git.openjdk.org/jdk/pull/17129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17129/head:pull/17129 PR: https://git.openjdk.org/jdk/pull/17129
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 16:52:38 GMT, Brian Burkhalter wrote: >>> Approved. >>> >>> Please note in future that it is better not to `/integrate` until the >>> request has actually been approved by a Reviewer. >> >> Alan actually did approve them on December 15. > >> Alan actually did approve them on December 15. > > I know, with requested changes. I didn't see the request transition to > "Ready" which is where it needs to be to `/integrate`. > @bplb Apparently Skara wants you to repeat your sponsoring again. Yes, because the PR was updated since the previous `/integrate`. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864817465
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Wed, 20 Dec 2023 09:47:05 GMT, Markus KARG wrote: > Alan actually did approve them on December 15. I know, with requested changes. I didn't see the request transition to "Ready" which is where it needs to be to `/integrate`. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864815013
Re: RFR: 8318971 : Better Error Handling for Jar Tool When Processing Non-existent Files [v4]
> Better Error Handling for Jar Tool Processing of "@File" > > This is a new PR for this PR since the original developer left the team. See > all of the review history at https://github.com/openjdk/jdk/pull/16423. > > Thank you. Weibing Xiao has updated the pull request incrementally with one additional commit since the last revision: update the test cases - Changes: - all: https://git.openjdk.org/jdk/pull/17088/files - new: https://git.openjdk.org/jdk/pull/17088/files/ba9335e0..ff349833 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17088=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17088=02-03 Stats: 39 lines in 1 file changed: 35 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17088.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17088/head:pull/17088 PR: https://git.openjdk.org/jdk/pull/17088
RFR: 8322078: ZipSourceCache.testKeySourceMapping() test fails with The process cannot access the file because it is being used by another process
trivial test edit to use FileUtils.deleteFileIfExistsWithRetry test library API tested on CI system with test-repeat = 20 - no issues seen. - Commit messages: - 8322078 Changes: https://git.openjdk.org/jdk/pull/17169/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17169=00 Issue: https://bugs.openjdk.org/browse/JDK-8322078 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17169.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17169/head:pull/17169 PR: https://git.openjdk.org/jdk/pull/17169
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Wed, 20 Dec 2023 14:28:39 GMT, Roger Riggs wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Changes according to reviewer's comments. > > It would also be useful/interesting to include a test that checks every > Serializable class (by Invoking `ObjectStreamClass.lookup(clazz)`) in the > Java runtime and reports any jfr events. > Fixing them would be a separate task. The compiler warnings from last year > should have fixed most/many non-conforming classes. > @RogerRiggs Do you mean a permanent test in the codebase, or just a throwaway > run? Anyway, interesting suggestion. A separate PR to add permanent test would advise about the current state and prevent new cases. If there are cases that can't be fixed (because of some kind of backward compatibility issue), there might need to be an exclusion list. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1864639166
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Wed, 20 Dec 2023 15:01:02 GMT, Raffaello Giulietti
wrote:
>> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
>> line 113:
>>
>>> 111: if (longFromStatic(f) == null) {
>>> 112: commitEvent(SUID_CONVERTIBLE_TO_LONG,
>>> 113: SUID_NAME + " must be convertible to long via
>>> widening to be effective");
>>
>> The serialization spec only shows using long. If any recommendation is made
>> it should be to declare the field as a `long`
>
> There's a check on the type at L.104 which is about the "should"
> recommendation, since serialization does not care about the type of the field
> being `long`.
>
> This check is about the value at runtime, which is a "must" because
> serialization expects it to be convertible to long by widening.
The implementation should not show through. I don't remember if the particular
implementation was intentional or just a shortcut to get the value. But any
suggestion that implies a fix should be to recommend the best practice.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432834402
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Wed, 20 Dec 2023 14:17:19 GMT, Roger Riggs wrote: >> Same remark here about finality as >> https://github.com/openjdk/jdk/pull/17129#discussion_r1432400888. public >> statics should be final. > > I'd also remove the error codes, the string messages will be pretty stable > and the extra surface area of the constants is just maintenance overhead. Working on that. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432828516
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Wed, 20 Dec 2023 14:28:39 GMT, Roger Riggs wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Changes according to reviewer's comments.
>
> It would also be useful/interesting to include a test that checks every
> Serializable class (by Invoking `ObjectStreamClass.lookup(clazz)`) in the
> Java runtime and reports any jfr events.
> Fixing them would be a separate task. The compiler warnings from last year
> should have fixed most/many non-conforming classes.
@RogerRiggs Do you mean a permanent test in the codebase, or just a throwaway
run? Anyway, interesting suggestion.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 113:
>
>> 111: if (longFromStatic(f) == null) {
>> 112: commitEvent(SUID_CONVERTIBLE_TO_LONG,
>> 113: SUID_NAME + " must be convertible to long via
>> widening to be effective");
>
> The serialization spec only shows using long. If any recommendation is made
> it should be to declare the field as a `long`
There's a check on the type at L.104 which is about the "should"
recommendation, since serialization does not care about the type of the field
being `long`.
This check is about the value at runtime, which is a "must" because
serialization expects it to be convertible to long by widening.
-
PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1864628102
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432827176
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 16:45:04 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Changes according to reviewer's comments. It would also be useful/interesting to include a test that checks every Serializable class (by Invoking `ObjectStreamClass.lookup(clazz)`) in the Java runtime and reports any jfr events. Fixing them would be a separate task. The compiler warnings from last year should have fixed most/many non-conforming classes. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1864569540
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 16:45:04 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Changes according to reviewer's comments.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 113:
> 111: if (longFromStatic(f) == null) {
> 112: commitEvent(SUID_CONVERTIBLE_TO_LONG,
> 113: SUID_NAME + " must be convertible to long via
> widening to be effective");
The serialization spec only shows using long. If any recommendation is made it
should be to declare the field as a `long`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432778556
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Wed, 20 Dec 2023 08:29:19 GMT, Daniel Fuchs wrote: >> You could define them with an Enum but use the ordinal as the value for JFR. > > Same remark here about finality as > https://github.com/openjdk/jdk/pull/17129#discussion_r1432400888. public > statics should be final. I'd also remove the error codes, the string messages will be pretty stable and the extra surface area of the constants is just maintenance overhead. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432772028
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Mon, 18 Dec 2023 17:09:59 GMT, Serguei Spitsyn wrote:
>> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1
>> time frame.
>> It is fixing a deadlock issue between `VirtualThread` class critical
>> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`,
>> `getAndClearInterrupt()`, `threadState()`, `toString()`),
>> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms.
>> The deadlocking scenario is well described by Patricio in a bug report
>> comment.
>> In simple words, a virtual thread should not be suspended during
>> 'interruptLock' critical sections.
>>
>> The fix is to record that a virtual thread is in a critical section
>> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about
>> begin/end of critical section.
>> This bit is used in `HandshakeState::get_op_for_self()` to filter out any
>> `HandshakeOperation` if a target `JavaThread` is in a critical section.
>>
>> Some of new notifications with `notifyJvmtiSync()` method is on a
>> performance critical path. It is why this method has been intrincified.
>>
>> New test was developed by Patricio:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> The test is very nice as it reliably in 100% reproduces the deadlock without
>> the fix.
>> The test is never failing with this fix.
>>
>> Testing:
>> - tested with newly added test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 10 commits:
>
> - Merge
> - review: improve an assert message
> - review: moved a couple of comments out of try blocks
> - review: moved notifyJvmtiDisableSuspend(true) out of try-block
> - review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks
> - review: (1) rename notifyJvmti method; (2) add try-final statements to
> VirtualThread methods
> - Resolved merge conflict in VirtualThread.java
> - added @summary to new test SuspendWithInterruptLock.java
> - add new test SuspendWithInterruptLock.java
> - 8311218: fatal error: stuck in
> JvmtiVTMSTransitionDisabler::VTMS_transition_disable
src/hotspot/share/runtime/javaThread.hpp line 652:
> 650:
> 651: bool is_disable_suspend() const{ return
> _is_disable_suspend; }
> 652: void toggle_is_disable_suspend() { _is_disable_suspend =
> !_is_disable_suspend; };
Looking at this again then I don't think it can be a bit that is toggled on and
off will work. Consider the case where several threads attempt to poll the
state of a virtual Thread with Thread::getState at the same time. This can't
work without an atomic counter and further coordination. So I think further
work is required on this issue.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432770204
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 19:19:35 GMT, Roger Riggs wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Better name for a label, corrected name of removed field.
>
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 108:
>
>> 106: SUID_NAME + " should be declared of type long");
>> 107: }
>> 108: if (!isStatic(f)) {
>
> The two calls to isStatic could be reordered closer together to be a single
> if (isstatic()) { ... } else {... }.
I guess you are looking to an older commit?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432581987
Re: RFR: 8321786: SegmentAllocator:allocateFrom(ValueLayout, MemorySegment, ValueLayout, long, long) spec mismatch in exception scenario [v7]
On Tue, 12 Dec 2023 15:45:47 GMT, Per Minborg wrote:
>> This PR proposes to change the specification for some methods that take
>> `long` offsets so that they will throw an `IllegalArgumentException` rather
>> than an `IndexOutOfBoundsException` for negative values.
>>
>> The PR also proposes to fix a bug where the allocation size would overflow
>> and the specified exception was not thrown.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move @throws tag
test/jdk/java/foreign/TestSegmentCopy.java line 183:
> 181: byte[] dst = new byte[] {1, 2, 3, 4};
> 182: assertThrows(IndexOutOfBoundsException.class, () ->
> 183: MemorySegment.copy(src, JAVA_BYTE, -1, dst,0, 4)
Suggestion:
MemorySegment.copy(src, JAVA_BYTE, -1, dst, 0, 4)
test/jdk/java/foreign/TestSegmentCopy.java line 186:
> 184: );
> 185: assertThrows(IndexOutOfBoundsException.class, () ->
> 186: MemorySegment.copy(src, JAVA_BYTE, 0, dst,-1, 4)
Suggestion:
MemorySegment.copy(src, JAVA_BYTE, 0, dst, -1, 4)
test/jdk/java/foreign/TestSegmentCopy.java line 189:
> 187: );
> 188: assertThrows(IndexOutOfBoundsException.class, () ->
> 189: MemorySegment.copy(src, JAVA_BYTE, 0, dst,0, -1)
Suggestion:
MemorySegment.copy(src, JAVA_BYTE, 0, dst, 0, -1)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1432545779
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1432546057
PR Review Comment: https://git.openjdk.org/jdk/pull/17079#discussion_r1432546260
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 08:02:14 GMT, Alan Bateman wrote:
>> src/hotspot/share/prims/jvm.cpp line 4024:
>>
>>> 4022: #else
>>> 4023: fatal("Should only be called with JVMTI enabled");
>>> 4024: #endif
>>
>> You can't do this! The Java code knows nothing about JVM TI being
>> enabled/disabled and will call this function unconditionally.
>
>> You can't do this! The Java code knows nothing about JVM TI being
>> enabled/disabled and will call this function unconditionally.
>
> Indeed. I wonder if anyone is testing minimal builds to catch issues like
> this.
Good catch, David!
Filed a cleanup bug: https://bugs.openjdk.org/browse/JDK-8322538
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432548911
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Tue, 19 Dec 2023 18:31:19 GMT, Brian Burkhalter wrote: >> Markus KARG has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Test was using Integer but must use Long > > I verified that the test now fails without the source change but passes with > it. @bplb Apparently Skara wants you to repeat your sponsoring again. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864199779
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v4]
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
Sergey Tsypanov has updated the pull request incrementally with one additional
commit since the last revision:
8322292: Tiny improvement
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17143/files
- new: https://git.openjdk.org/jdk/pull/17143/files/dded977f..9c8ae4fb
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17143=03
- incr: https://webrevs.openjdk.org/?repo=jdk=17143=02-03
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/17143.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17143/head:pull/17143
PR: https://git.openjdk.org/jdk/pull/17143
Re: RFR: JDK-8322141: SequenceInputStream.transferTo should not return as soon as Long.MAX_VALUE bytes have been transferred [v4]
On Tue, 19 Dec 2023 18:20:05 GMT, Brian Burkhalter wrote: > Approved. > > Please note in future that it is better not to `/integrate` until the request > has actually been approved by a Reviewer. Alan actually did approve them on December 15. - PR Comment: https://git.openjdk.org/jdk/pull/17119#issuecomment-1864164861
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 19:39:22 GMT, Roger Riggs wrote: >> What about fixed `String`s rather than `int`s for the kind of error? >> Something like `"SUID_INEFFECTIVE_ON_ENUM"`, and so on? >> It would be nice to be able to use enums, but AFAIK that's not supported in >> JFR. > > You could define them with an Enum but use the ordinal as the value for JFR. Same remark here about finality as https://github.com/openjdk/jdk/pull/17129#discussion_r1432400888. public statics should be final. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432402527
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 16:00:09 GMT, Raffaello Giulietti wrote: >> src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java >> line 94: >> >>> 92: >>> 93: /* >>> 94: * These constants are not final on purpose. >> >> Just curious - what's the purpose? I don't see them being changed/updated >> anywhere (not even in tests). > > Declaring a `public static int` field `final` means that the value is then > stored in the client class. If a value is changed, then the client needs to > be recompiled as well, but this is not enforced by the language, so it might > lead to inconsistencies. > > The clean solution would be to use an enum class, but that's not supported by > JFR. Not having them final is a quite smelly. I would suggest to make them final with a comment that new values can be added but that existing values must not be changed. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1432400888
Re: RFR: 8322417: Console read line with zero out should zero out when throwing exception
On Tue, 19 Dec 2023 12:47:53 GMT, Goetz Lindenmaier wrote:
> …g exception
>
> After leaving the method by throwing an exception the data can not be cleaned
> any more.
src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java line 122:
> 120: ioe.addSuppressed(x);
> 121: }
> 122: if (ioe != null) {
I assume it's only important to zero the buffer when restoring the echo setting
fails. If I read the changes correctly then it's been zero's even if readLine
fails. It's okay, just probably unnecessary for that case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17156#discussion_r1432381996
Integrated: 8322417: Console read line with zero out should zero out when throwing exception
On Tue, 19 Dec 2023 12:47:53 GMT, Goetz Lindenmaier wrote: > …g exception > > After leaving the method by throwing an exception the data can not be cleaned > any more. This pull request has now been integrated. Changeset: 2f917bff Author:Goetz Lindenmaier URL: https://git.openjdk.org/jdk/commit/2f917bff5cbb71dccd70960f563ca1a05d109fda Stats: 10 lines in 1 file changed: 9 ins; 0 del; 1 mod 8322417: Console read line with zero out should zero out when throwing exception Reviewed-by: mbaesken, stuefe, naoto - PR: https://git.openjdk.org/jdk/pull/17156
Re: RFR: 8322417: Console read line with zero out should zero out when throwing exception
On Tue, 19 Dec 2023 12:47:53 GMT, Goetz Lindenmaier wrote: > …g exception > > After leaving the method by throwing an exception the data can not be cleaned > any more. Thanks for the reviews! SAPs nigthly testing passed for this change. Sorry, I saw the java.uti. comment too late :( - PR Comment: https://git.openjdk.org/jdk/pull/17156#issuecomment-1864024998 PR Comment: https://git.openjdk.org/jdk/pull/17156#issuecomment-1864029388
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 04:44:35 GMT, David Holmes wrote: > You can't do this! The Java code knows nothing about JVM TI being > enabled/disabled and will call this function unconditionally. Indeed. I wonder if anyone is testing minimal builds to catch issues like this. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432377494
