[jdk22] Integrated: Merge c7f1c97312f94b6dd6398a5e98dd0c8b63db4c9b
On Tue, 16 Jan 2024 16:31:32 GMT, Henry Jen wrote: > CPU24_01 fixes. This pull request has now been integrated. Changeset: b2cc1890 Author:Henry Jen URL: https://git.openjdk.org/jdk22/commit/b2cc1890ff4d2e5404e153ecba5e83f1bcdd6fa7 Stats: 736 lines in 16 files changed: 476 ins; 65 del; 195 mod Merge Reviewed-by: erikj - PR: https://git.openjdk.org/jdk22/pull/83
Re: [jdk22] RFR: Merge c7f1c97312f94b6dd6398a5e98dd0c8b63db4c9b [v2]
> CPU24_01 fixes. Henry Jen 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. - Changes: - all: https://git.openjdk.org/jdk22/pull/83/files - new: https://git.openjdk.org/jdk22/pull/83/files/c7f1c973..c7f1c973 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk22&pr=83&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk22&pr=83&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk22/pull/83.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/83/head:pull/83 PR: https://git.openjdk.org/jdk22/pull/83
Integrated: Merge bf7bd9a16c172bcb5ea6b24717a0429e12e2e3d1
On Tue, 16 Jan 2024 16:32:27 GMT, Henry Jen wrote: > CPU24_01 fixes. This pull request has now been integrated. Changeset: 2063bb8f Author:Henry Jen URL: https://git.openjdk.org/jdk/commit/2063bb8ffabd6096f547ec6da979cfcf68a56ba3 Stats: 736 lines in 16 files changed: 476 ins; 65 del; 195 mod Merge Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/17448
Re: [jdk22] RFR: Merge c7f1c97312f94b6dd6398a5e98dd0c8b63db4c9b
On Tue, 16 Jan 2024 16:31:32 GMT, Henry Jen wrote: > CPU24_01 fixes. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk22/pull/83#pullrequestreview-1826185706
Re: RFR: Merge bf7bd9a16c172bcb5ea6b24717a0429e12e2e3d1 [v2]
On Tue, 16 Jan 2024 19:05:44 GMT, Henry Jen wrote: >> CPU24_01 fixes. > > Henry Jen 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 eight additional commits since > the last revision: > > - Merge branch 'openjdk:master' into cpu2401 > - 8317547: Enhance TLS connection support > >Reviewed-by: ahgross, rhalade, weijun, valeriep > - 8314307: Improve loop handling > >Co-authored-by: Christian Hagedorn >Co-authored-by: Roland Westrelin >Co-authored-by: Emanuel Peter >Reviewed-by: mschoene, rhalade, thartmann, epeter > - 8318588: Windows build failure after JDK-8314468 due to ambiguous call > >Reviewed-by: epeter > - 8314468: Improve Compiler loops > >Co-authored-by: Dean Long >Reviewed-by: rhalade, mschoene, iveresov, kvn > - 8317331: Solaris build failed with "declaration can not follow a statement > (E_DECLARATION_IN_CODE)" > >Backport-of: 852276d1f833d49802693f2a5a82ba6eb2722de6 > - 8314295: Enhance verification of verifier > >Reviewed-by: mschoene, rhalade, dholmes, dlong > - 8308204: Enhanced certificate processing > >Reviewed-by: mschoene, rhalade, jnimeh Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17448#pullrequestreview-1826181933
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v4]
> There's an unused concept of a pluginConfig that is passed into the jlink > compress plugins, however we always pass null here and the code seems broken > (the pluginConfig wouldn't have been stored properly). This seem to be a > left-over from a more generic plugin design that never came to fruition. > > This PR cleans out this code from the plugins and decompressors, while > keeping the compressed header format intact for backwards compatibility (and > in case we want to revisit this in the future) Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Wrong order - Changes: - all: https://git.openjdk.org/jdk/pull/17443/files - new: https://git.openjdk.org/jdk/pull/17443/files/a65ffbf5..d1917182 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17443&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17443&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17443.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17443/head:pull/17443 PR: https://git.openjdk.org/jdk/pull/17443
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v7]
On Tue, 16 Jan 2024 23:51:15 GMT, Scott Gibbons wrote:
>> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 4111:
>>
>>> 4109: if ((UseAVX == 2) && EnableX86ECoreOpts &&
>>> VM_Version::supports_avx2()) {
>>> 4110: StubRoutines::_string_indexof = generate_string_indexof();
>>> 4111: }
>>
>> What motivation for this extensive new code only for avx2? 30% is nice (for
>> some cases) but it is enabled only for AVX2 and not for avx512 which all
>> modern x86 CPUs have so the code will not be used.
>>
>> Or it is typo?
>
> This is acceleration for AVX2, replacing the pcmpestri instruction which is
> microcoded on E-cores and causes significant performance impact. I am
> working on a pared-down implementation and should update this PR in a couple
> of days.
Thank you for explanation.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1454238988
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v7]
On Tue, 16 Jan 2024 09:11:01 GMT, Chris Hegarty wrote: >> Update LinkedTransferQueue add and put methods to not call overridable offer. > > Chris Hegarty 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 eight additional > commits since the last revision: > > - Merge branch 'master' into ltq_bug > - Merge branch 'master' into ltq_bug > - order of tags > - Merge branch 'master' into ltq_bug > - Update > src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java > >Co-authored-by: Andrey Turbanov > - timed offer > - add test > - Update LinkedTransferQueue add and put methods to not call overridable > offer I just integrated the fix into jdk 22, so we’re good there now. The final piece of the puzzle is jdk 21.0.2, which we’re too late to fix. We can add a release note, and fix it in 21.0.3. Any objections or alternative suggestions? - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1894710437
[jdk22] Integrated: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Tue, 16 Jan 2024 12:23:43 GMT, Chris Hegarty wrote: > Hi all, > > This pull request contains a backport of commit > [ee4d9aa4](https://github.com/openjdk/jdk/commit/ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Chris Hegarty on 16 Jan 2024 and > was reviewed by Alan Bateman. > > Thanks! This pull request has now been integrated. Changeset: c1ea6daa Author:Chris Hegarty URL: https://git.openjdk.org/jdk22/commit/c1ea6daa5bd3ecee4fd3f8acaf91dfa48ec02f1b Stats: 75 lines in 2 files changed: 72 ins; 0 del; 3 mod 8323659: LinkedTransferQueue add and put methods call overridable offer Reviewed-by: alanb Backport-of: ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7 - PR: https://git.openjdk.org/jdk22/pull/80
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v7]
On Tue, 16 Jan 2024 22:27:52 GMT, Vladimir Kozlov wrote:
>> Scott Gibbons has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 22 commits:
>>
>> - Merge branch 'openjdk:master' into indexof
>> - Merge branch 'openjdk:master' into indexof
>> - Addressing review comments.
>> - Fix for JDK-8321599
>> - Support UU IndexOf
>> - Only use optimization when EnableX86ECoreOpts is true
>> - Fix whitespace
>> - Merge branch 'openjdk:master' into indexof
>> - Comments; added exhaustive-ish test
>> - Subtracting 0x10 twice.
>> - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2
>
> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 4111:
>
>> 4109: if ((UseAVX == 2) && EnableX86ECoreOpts &&
>> VM_Version::supports_avx2()) {
>> 4110: StubRoutines::_string_indexof = generate_string_indexof();
>> 4111: }
>
> What motivation for this extensive new code only for avx2? 30% is nice (for
> some cases) but it is enabled only for AVX2 and not for avx512 which all
> modern x86 CPUs have so the code will not be used.
>
> Or it is typo?
This is acceleration for AVX2, replacing the pcmpestri instruction which is
microcoded on E-cores and causes significant performance impact. I am working
on a pared-down implementation and should update this PR in a couple of days.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1454217437
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v7]
On Thu, 11 Jan 2024 23:06: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 with a new target base due to a
> merge or a rebase. The pull request now contains 22 commits:
>
> - Merge branch 'openjdk:master' into indexof
> - Merge branch 'openjdk:master' into indexof
> - Addressing review comments.
> - Fix for JDK-8321599
> - Support UU IndexOf
> - Only use optimization when EnableX86ECoreOpts is true
> - Fix whitespace
> - Merge branch 'openjdk:master' into indexof
> - Comments; added exhaustive-ish test
> - Subtracting 0x10 twice.
> - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 4111:
> 4109: if ((UseAVX == 2) && EnableX86ECoreOpts &&
> VM_Version::supports_avx2()) {
> 4110: StubRoutines::_string_indexof = generate_string_indexof();
> 4111: }
What motivation for this extensive new code only for avx2? 30% is nice (for
some cases) but it is enabled only for AVX2 and not for avx512 which all modern
x86 CPUs have so the code will not be used.
Or it is typo?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1454139710
[jdk22] Integrated: 8322235: Split up and improve LocaleProvidersRun
On Fri, 12 Jan 2024 18:25:25 GMT, Justin Lu wrote: > Please review this PR which is a backport of commit > [4ea7b364](https://github.com/openjdk/jdk/commit/4ea7b36447ea96d62b1ca164c34e2b2b74a16579) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The original commit was a test-only change which optimized and split up the > LocaleProvidersRun.java test. This pull request has now been integrated. Changeset: b9a535b8 Author:Justin Lu URL: https://git.openjdk.org/jdk22/commit/b9a535b8ac2e7bd5c7c2e56c1b0a498fa9c94d2a Stats: 569 lines in 8 files changed: 425 ins; 85 del; 59 mod 8322235: Split up and improve LocaleProvidersRun Reviewed-by: naoto, iris Backport-of: 4ea7b36447ea96d62b1ca164c34e2b2b74a16579 - PR: https://git.openjdk.org/jdk22/pull/68
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern
On Sun, 14 Jan 2024 15:32:12 GMT, Archie Cobbs wrote:
> Please consider this fix to ensure that going from `MessageFormat` to pattern
> string via `toPattern()` and then back via `new MessageFormat()` results in a
> format that is equivalent to the original.
>
> The quoting and escaping rules for `MessageFormat` pattern strings are really
> tricky. I admit not completely understanding them. At a high level, they work
> like this: The normal way one would "nest" strings containing special
> characters is with straightforward recursive escaping like with the `bash`
> command line. For example, if you want to echo `a "quoted string" example`
> then you enter `echo "a "quoted string" example"`. With this scheme it's
> always the "outer" layer's job to (un)escape special characters as needed.
> That is, the echo command never sees the backslash characters.
>
> In contrast, with `MessageFormat` and friends, nested subformat pattern
> strings are always provided "pre-escaped". So to build an "outer" string
> (e.g., for `ChoiceFormat`) the "inner" subformat pattern strings are more or
> less just concatenated, and then only the `ChoiceFormat` option separator
> characters (e.g., `<`, `#`, `|`, etc.) are escaped.
>
> The "pre-escape" escaping algorithm escapes `{` characters, because `{`
> indicates the beginning of a format argument. However, it doesn't escape `}`
> characters. This is OK because the format string parser treats any "extra"
> closing braces (where "extra" means not matching an opening brace) as plain
> characters.
>
> So far, so good... at least, until a format string containing an extra
> closing brace is nested inside a larger format string, where the extra
> closing brace, which was previously "extra", can now suddenly match an
> opening brace in the outer pattern containing it, thus truncating it by
> "stealing" the match from some subsequent closing brace.
>
> An example is the `MessageFormat` string `"{0,choice,0.0#option A:
> {1}|1.0#option B: {1}'}'}"`. Note the second option format string has a
> trailing closing brace in plain text. If you create a `MessageFormat` with
> this string, you see a trailing `}` only with the second option.
>
> However, if you then invoke `toPattern()`, the result is
> `"{0,choice,0.0#option A: {1}|1.0#option B: {1}}}"`. Oops, now because the
> "extra" closing brace is no longer quoted, it matches the opening brace at
> the beginning of the string, and the following closing brace, which was the
> previous match, is now just plain text in the outer `MessageFormat` string.
>
> As a result, invoking `f.format(new Object{} { 0, 5 })` will retur...
Hi Archie, thanks for the proposed fix. I am still taking a look, but I wanted
to demonstrate a current issue,
(Jshell with your patch)
var pattIn = "Test: {0,number,foo'{'#.00}";
MessageFormat mFmt = new MessageFormat(pattIn);
var pattOut = mFmt.toPattern(); // returns "Test: {0,number,foo{#.00}";
var pattIn = "Test: {0,number,foo'}'#.00}";
MessageFormat mFmt = new MessageFormat(pattIn);
var pattOut = mFmt.toPattern(); // returns "Test: {0,number,foo'}'#.00}";
As it stands, it would be inconsistent to have the closing bracket quoted and
the opening bracket not quoted.
Also in response to your earlier question on core-libs-dev, ideally invoking
toPattern() can roundtrip, but there are known issues, such as a custom user
defined Format subclass, or one of the newer Format subclasses that do not
implement the toPattern() method. I am working on making this apparent in the
specification of the method in a separate issue.
-
PR Comment: https://git.openjdk.org/jdk/pull/17416#issuecomment-1894594743
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v3]
On Tue, 16 Jan 2024 18:42:59 GMT, Claes Redestad wrote: >> There's an unused concept of a pluginConfig that is passed into the jlink >> compress plugins, however we always pass null here and the code seems broken >> (the pluginConfig wouldn't have been stored properly). This seem to be a >> left-over from a more generic plugin design that never came to fruition. >> >> This PR cleans out this code from the plugins and decompressors, while >> keeping the compressed header format intact for backwards compatibility (and >> in case we want to revisit this in the future) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Copyrights, unused imports Marked as reviewed by mchung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17443#pullrequestreview-1824981642
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v3]
On Tue, 16 Jan 2024 18:47:43 GMT, Joe Darcy wrote: > We can and have run retroactive CSRs in cases like this before; I recommend > we do one now. Yes although the issue will be mute once JDK-8323659 is integrated into jdk22. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1894345385
Re: RFR: Merge bf7bd9a16c172bcb5ea6b24717a0429e12e2e3d1 [v2]
> CPU24_01 fixes. Henry Jen 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 eight additional commits since the last revision: - Merge branch 'openjdk:master' into cpu2401 - 8317547: Enhance TLS connection support Reviewed-by: ahgross, rhalade, weijun, valeriep - 8314307: Improve loop handling Co-authored-by: Christian Hagedorn Co-authored-by: Roland Westrelin Co-authored-by: Emanuel Peter Reviewed-by: mschoene, rhalade, thartmann, epeter - 8318588: Windows build failure after JDK-8314468 due to ambiguous call Reviewed-by: epeter - 8314468: Improve Compiler loops Co-authored-by: Dean Long Reviewed-by: rhalade, mschoene, iveresov, kvn - 8317331: Solaris build failed with "declaration can not follow a statement (E_DECLARATION_IN_CODE)" Backport-of: 852276d1f833d49802693f2a5a82ba6eb2722de6 - 8314295: Enhance verification of verifier Reviewed-by: mschoene, rhalade, dholmes, dlong - 8308204: Enhanced certificate processing Reviewed-by: mschoene, rhalade, jnimeh - Changes: - all: https://git.openjdk.org/jdk/pull/17448/files - new: https://git.openjdk.org/jdk/pull/17448/files/bf7bd9a1..e4e0d987 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17448&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17448&range=00-01 Stats: 484 lines in 21 files changed: 304 ins; 157 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/17448.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17448/head:pull/17448 PR: https://git.openjdk.org/jdk/pull/17448
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v3]
On Mon, 15 Jan 2024 09:49:53 GMT, Alan Bateman wrote: > > With my CSR hat on, JDK-8301341 should never have made the changes it did > > without going through a CSR request. We have been bitten by this kind of > > problem many times. Unless a public method is specified to utilise another > > public method, we should never implement one public method in terms of > > another (non-final one) due to the overriding problem. > > JDK-8301341 was a big update, a lot of refactoring to hollow out SQ and it > was just an oversight that we didn't spot the methods now use an overridable > method. It's something to always look out for in the collections area, just > missed here. We can and have run retroactive CSRs in cases like this before; I recommend we do one now. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1894323864
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v3]
> There's an unused concept of a pluginConfig that is passed into the jlink > compress plugins, however we always pass null here and the code seems broken > (the pluginConfig wouldn't have been stored properly). This seem to be a > left-over from a more generic plugin design that never came to fruition. > > This PR cleans out this code from the plugins and decompressors, while > keeping the compressed header format intact for backwards compatibility (and > in case we want to revisit this in the future) Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Copyrights, unused imports - Changes: - all: https://git.openjdk.org/jdk/pull/17443/files - new: https://git.openjdk.org/jdk/pull/17443/files/7df80e39..a65ffbf5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17443&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17443&range=01-02 Stats: 17 lines in 8 files changed: 0 ins; 8 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17443.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17443/head:pull/17443 PR: https://git.openjdk.org/jdk/pull/17443
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v4]
On Mon, 15 Jan 2024 10:26:53 GMT, Eirik Bjørsnøs wrote: >> Please consider this PR which makes `DeflaterOutputStream.close()` always >> close its wrapped output stream exactly once. >> >> Currently, closing of the wrapped output stream happens outside the finally >> block where `finish()` is called. If `finish()` throws, this means the >> wrapped stream will not be closed. This can potentially lead to leaking >> resources such as file descriptors or sockets. >> >> This fix is to move the closing of the wrapped stream inside the finally >> block. >> >> Additionally, the `closed = true;` statement is moved to the start of the >> close method. This makes sure we only ever close the wrapped stream once >> (this aligns with the overridden method `FilterOutputStream.close´) >> >> Specification: This change brings the implementation of >> `DeflaterOutputStream.close()` in line with its specification: *Writes >> remaining compressed data to the output stream and closes the underlying >> stream.* >> >> Risk: This is a behavioural change. There is a small risk that existing code >> depends on the close method not following its specification. >> >> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which >> simulates the failure condition and verifies that the wrapped stream was >> closed under failing and non-failing conditions. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/jdk/java/util/zip/ZipOutputStream/CloseWrappedStream.java > > Remove extra whitespace > > Co-authored-by: Andrey Turbanov The changes look good overall.See suggestion for comment improvement but not required, just makes it clearer test/jdk/java/util/zip/ZipOutputStream/CloseWrappedStream.java line 91: > 89: /** > 90: * Check that the exception handling is correct when the > 91: * wrapped stream throws while being closed This comment could use a bit of wordsmithing to indicate what "correct" means - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17209#pullrequestreview-1824541172 PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1453817997
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v2]
On Tue, 16 Jan 2024 18:03:34 GMT, Claes Redestad wrote: >> There's an unused concept of a pluginConfig that is passed into the jlink >> compress plugins, however we always pass null here and the code seems broken >> (the pluginConfig wouldn't have been stored properly). This seem to be a >> left-over from a more generic plugin design that never came to fruition. >> >> This PR cleans out this code from the plugins and decompressors, while >> keeping the compressed header format intact for backwards compatibility (and >> in case we want to revisit this in the future) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Named offsets Looks good. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17443#pullrequestreview-1824528708
[jdk22] RFR: Merge c7f1c97312f94b6dd6398a5e98dd0c8b63db4c9b
CPU24_01 fixes. - Commit messages: - 8317547: Enhance TLS connection support - 8314307: Improve loop handling - 8318588: Windows build failure after JDK-8314468 due to ambiguous call - 8314468: Improve Compiler loops - 8317331: Solaris build failed with "declaration can not follow a statement (E_DECLARATION_IN_CODE)" - 8314295: Enhance verification of verifier - 8308204: Enhanced certificate processing The merge commit only contains trivial merges, so no merge-specific webrevs have been generated. Changes: https://git.openjdk.org/jdk22/pull/83/files Stats: 736 lines in 16 files changed: 476 ins; 65 del; 195 mod Patch: https://git.openjdk.org/jdk22/pull/83.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/83/head:pull/83 PR: https://git.openjdk.org/jdk22/pull/83
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v2]
On Tue, 16 Jan 2024 18:03:34 GMT, Claes Redestad wrote: >> There's an unused concept of a pluginConfig that is passed into the jlink >> compress plugins, however we always pass null here and the code seems broken >> (the pluginConfig wouldn't have been stored properly). This seem to be a >> left-over from a more generic plugin design that never came to fruition. >> >> This PR cleans out this code from the plugins and decompressors, while >> keeping the compressed header format intact for backwards compatibility (and >> in case we want to revisit this in the future) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Named offsets LGTM - Marked as reviewed by jlaskey (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17443#pullrequestreview-1824485331
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v2]
> There's an unused concept of a pluginConfig that is passed into the jlink > compress plugins, however we always pass null here and the code seems broken > (the pluginConfig wouldn't have been stored properly). This seem to be a > left-over from a more generic plugin design that never came to fruition. > > This PR cleans out this code from the plugins and decompressors, while > keeping the compressed header format intact for backwards compatibility (and > in case we want to revisit this in the future) Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Named offsets - Changes: - all: https://git.openjdk.org/jdk/pull/17443/files - new: https://git.openjdk.org/jdk/pull/17443/files/591047b1..7df80e39 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17443&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17443&range=00-01 Stats: 14 lines in 1 file changed: 8 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/17443.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17443/head:pull/17443 PR: https://git.openjdk.org/jdk/pull/17443
RFR: Merge bf7bd9a16c172bcb5ea6b24717a0429e12e2e3d1
CPU24_01 fixes. - Commit messages: - 8317547: Enhance TLS connection support - 8314307: Improve loop handling - 8318588: Windows build failure after JDK-8314468 due to ambiguous call - 8314468: Improve Compiler loops - 8317331: Solaris build failed with "declaration can not follow a statement (E_DECLARATION_IN_CODE)" - 8314295: Enhance verification of verifier - 8308204: Enhanced certificate processing The merge commit only contains trivial merges, so no merge-specific webrevs have been generated. Changes: https://git.openjdk.org/jdk/pull/17448/files Stats: 736 lines in 16 files changed: 476 ins; 65 del; 195 mod Patch: https://git.openjdk.org/jdk/pull/17448.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17448/head:pull/17448 PR: https://git.openjdk.org/jdk/pull/17448
Re: [jdk22] RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate [v2]
On Tue, 16 Jan 2024 08:47:47 GMT, Per Minborg wrote: >> 8323159: Consider adding some text re. memory zeroing in Arena::allocate > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing comma in TestScope.java Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk22/pull/77#pullrequestreview-1824361691
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v5]
> `GZIPInputStream`, when looking for a concatenated stream, relies on what the > underlying `InputStream` says is how many bytes are `available()`. But this > is inappropriate because `InputStream.available()` is just an estimate and is > allowed (for example) to always return zero. > > The fix is to ignore what's `available()` and just proceed and see what > happens. If fewer bytes are available than required, the attempt to extend to > another stream is canceled just as it was before, e.g., when the next stream > header couldn't be read. Archie Cobbs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into JDK-7036144 - Address third round of review comments. - Address second round of review comments. - Address review comments. - Fix bug in GZIPInputStream when underlying available() returns short. - Changes: - all: https://git.openjdk.org/jdk/pull/17113/files - new: https://git.openjdk.org/jdk/pull/17113/files/cf457eff..4f1a0459 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17113&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17113&range=03-04 Stats: 35310 lines in 1143 files changed: 22955 ins; 7236 del; 5119 mod Patch: https://git.openjdk.org/jdk/pull/17113.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17113/head:pull/17113 PR: https://git.openjdk.org/jdk/pull/17113
Re: [jdk22] RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate [v2]
On Tue, 16 Jan 2024 08:47:47 GMT, Per Minborg wrote: >> 8323159: Consider adding some text re. memory zeroing in Arena::allocate > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing comma in TestScope.java Looks good - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk22/pull/77#pullrequestreview-1824307173
Re: RFR: 8310837: Use ByteArrayLittleEndian in java.util.zip [v7]
On Tue, 16 Jan 2024 16:38:34 GMT, Glavo wrote: >> Using `ByteArrayLittleEndian` is simpler and faster. >> >> `make test TEST="micro:java.util.zip.ZipFileOpen"`: >> >> >> Benchmark (size) Mode Cnt Score Error >> Units >> - ZipFileOpen.openCloseZipFile 512 avgt 15 39052.832 ± 107.496 >> ns/op >> + ZipFileOpen.openCloseZipFile 512 avgt 15 36275.539 ± 663.193 >> ns/op >> - ZipFileOpen.openCloseZipFile1024 avgt 15 77106.494 ± 4159.300 >> ns/op >> + ZipFileOpen.openCloseZipFile1024 avgt 15 71955.013 ± 2296.050 >> ns/op > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > Add tests I ran the tier1 tests with no failures. - PR Comment: https://git.openjdk.org/jdk/pull/14632#issuecomment-1894174227
Re: RFR: 8310837: Use ByteArrayLittleEndian in java.util.zip [v7]
> Using `ByteArrayLittleEndian` is simpler and faster. > > `make test TEST="micro:java.util.zip.ZipFileOpen"`: > > > Benchmark (size) Mode Cnt Score Error Units > - ZipFileOpen.openCloseZipFile 512 avgt 15 39052.832 ± 107.496 ns/op > + ZipFileOpen.openCloseZipFile 512 avgt 15 36275.539 ± 663.193 ns/op > - ZipFileOpen.openCloseZipFile1024 avgt 15 77106.494 ± 4159.300 ns/op > + ZipFileOpen.openCloseZipFile1024 avgt 15 71955.013 ± 2296.050 ns/op Glavo has updated the pull request incrementally with one additional commit since the last revision: Add tests - Changes: - all: https://git.openjdk.org/jdk/pull/14632/files - new: https://git.openjdk.org/jdk/pull/14632/files/72175ea1..a9b6b78e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14632&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14632&range=05-06 Stats: 440 lines in 4 files changed: 339 ins; 19 del; 82 mod Patch: https://git.openjdk.org/jdk/pull/14632.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14632/head:pull/14632 PR: https://git.openjdk.org/jdk/pull/14632
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration
On Tue, 16 Jan 2024 10:55:07 GMT, Claes Redestad wrote: > There's an unused concept of a pluginConfig that is passed into the jlink > compress plugins, however we always pass null here and the code seems broken > (the pluginConfig wouldn't have been stored properly). This seem to be a > left-over from a more generic plugin design that never came to fruition. > > This PR cleans out this code from the plugins and decompressors, while > keeping the compressed header format intact for backwards compatibility (and > in case we want to revisit this in the future) Changes requested by jlaskey (Reviewer). src/java.base/share/classes/jdk/internal/jimage/decompressor/CompressedResourceHeader.java line 104: > 102: ByteBuffer buffer = ByteBuffer.wrap(resource, 0, SIZE); > 103: buffer.order(order); > 104: int magic = buffer.getInt(0); Named constant for offsets? src/java.base/share/classes/jdk/internal/jimage/decompressor/CompressedResourceHeader.java line 108: > 106: return null; > 107: } > 108: long size = buffer.getLong(4); Named constant for offsets? - PR Review: https://git.openjdk.org/jdk/pull/17443#pullrequestreview-1823928256 PR Review Comment: https://git.openjdk.org/jdk/pull/17443#discussion_r1453640996 PR Review Comment: https://git.openjdk.org/jdk/pull/17443#discussion_r1453641242
Re: RFR: 8310837: Use ByteArrayLittleEndian in java.util.zip [v6]
On Mon, 20 Nov 2023 16:24:31 GMT, Glavo wrote: >> Using `ByteArrayLittleEndian` is simpler and faster. >> >> `make test TEST="micro:java.util.zip.ZipFileOpen"`: >> >> >> Benchmark (size) Mode Cnt Score Error >> Units >> - ZipFileOpen.openCloseZipFile 512 avgt 15 39052.832 ± 107.496 >> ns/op >> + ZipFileOpen.openCloseZipFile 512 avgt 15 36275.539 ± 663.193 >> ns/op >> - ZipFileOpen.openCloseZipFile1024 avgt 15 77106.494 ± 4159.300 >> ns/op >> + ZipFileOpen.openCloseZipFile1024 avgt 15 71955.013 ± 2296.050 >> ns/op > > Glavo 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: > > - Merge branch 'openjdk:master' into zip-utils > - Merge branch 'openjdk:master' into zip-utils > - Merge branch 'openjdk:master' into zip-utils > - Merge branch 'openjdk:master' into zip-utils > - Merge branch 'openjdk:master' into zip-utils > - use ByteArrayLittleEndian in ZipUtils > Hello Glavo, I see that you are interested in pursuing this change further. > Would you mind getting the latest micro benchmark numbers which this proposed > change? I see that your PR description has a run from some time back, getting > a latest one would be useful. > > Additionally, I see that #14636 where you had proposed a test case for the > `ByteArrayLittleEndian` class (in addition to other things) got closed > without being integrated. Would you mind adding a new test case for that > class as part of this current PR since you have a few more new methods being > added to that class? I've moved those changes into this PR and am running tests. I'll push these changes once the tests are finished running. - PR Comment: https://git.openjdk.org/jdk/pull/14632#issuecomment-1894037304
Re: RFR: 8310837: Use ByteArrayLittleEndian in java.util.zip [v6]
On Mon, 20 Nov 2023 16:24:31 GMT, Glavo wrote: >> Using `ByteArrayLittleEndian` is simpler and faster. >> >> `make test TEST="micro:java.util.zip.ZipFileOpen"`: >> >> >> Benchmark (size) Mode Cnt Score Error >> Units >> - ZipFileOpen.openCloseZipFile 512 avgt 15 39052.832 ± 107.496 >> ns/op >> + ZipFileOpen.openCloseZipFile 512 avgt 15 36275.539 ± 663.193 >> ns/op >> - ZipFileOpen.openCloseZipFile1024 avgt 15 77106.494 ± 4159.300 >> ns/op >> + ZipFileOpen.openCloseZipFile1024 avgt 15 71955.013 ± 2296.050 >> ns/op > > Glavo 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: > > - Merge branch 'openjdk:master' into zip-utils > - Merge branch 'openjdk:master' into zip-utils > - Merge branch 'openjdk:master' into zip-utils > - Merge branch 'openjdk:master' into zip-utils > - Merge branch 'openjdk:master' into zip-utils > - use ByteArrayLittleEndian in ZipUtils Hello Glavo, I see that you are interested in pursuing this change further. Would you mind getting the latest micro benchmark numbers which this proposed change? I see that your PR description has a run from some time back, getting a latest one would be useful. Additionally, I see that https://github.com/openjdk/jdk/pull/14636 where you had proposed a test case for the `ByteArrayLittleEndian` class (in addition to other things) got closed without being integrated. Would you mind adding a new test case for that class as part of this current PR since you have a few more new methods being added to that class? - PR Comment: https://git.openjdk.org/jdk/pull/14632#issuecomment-1893953682
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v4]
On Mon, 15 Jan 2024 10:26:53 GMT, Eirik Bjørsnøs wrote: >> Please consider this PR which makes `DeflaterOutputStream.close()` always >> close its wrapped output stream exactly once. >> >> Currently, closing of the wrapped output stream happens outside the finally >> block where `finish()` is called. If `finish()` throws, this means the >> wrapped stream will not be closed. This can potentially lead to leaking >> resources such as file descriptors or sockets. >> >> This fix is to move the closing of the wrapped stream inside the finally >> block. >> >> Additionally, the `closed = true;` statement is moved to the start of the >> close method. This makes sure we only ever close the wrapped stream once >> (this aligns with the overridden method `FilterOutputStream.close´) >> >> Specification: This change brings the implementation of >> `DeflaterOutputStream.close()` in line with its specification: *Writes >> remaining compressed data to the output stream and closes the underlying >> stream.* >> >> Risk: This is a behavioural change. There is a small risk that existing code >> depends on the close method not following its specification. >> >> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which >> simulates the failure condition and verifies that the wrapped stream was >> closed under failing and non-failing conditions. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/jdk/java/util/zip/ZipOutputStream/CloseWrappedStream.java > > Remove extra whitespace > > Co-authored-by: Andrey Turbanov The source changes as well as the test looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17209#pullrequestreview-1823702967
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]
On Tue, 16 Jan 2024 14:41:06 GMT, Eirik Bjørsnøs wrote: > The spec isn't terribly helpful in spelling out what should happen in the > case where an entry combines the uses of data descriptors (mandating that > CRC, size and compressed size must be zero) with Zip64 (mandating that size > and compressed size must be 0x) I see what you mean. However, given that the data descriptor general bit field is set to indicate that the data descriptor should be honoured, plus the spec stating that the original/compressed sizes in the zip64 extra block aren't mandatory, I think not relying on the zip64 extra block for parsing decisions about a data descriptor content might be better. I think the only role that a zip64 block should play when we are deciding how to parse a data descriptor is whether or not the entry has zip64 extra field set (the header id value of the extra field). Does that sound reasonable? - PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1453546995
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v14]
> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the > number of compressed or uncompressed bytes read from the inflater is larger > than the Zip64 magic value. > > While the ZIP format mandates that the data descriptor `SHOULD be stored in > ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it > also states that `ZIP64 format MAY be used regardless of the size of a file`. > For such small entries, the above assumption does not hold. > > This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the > ZipEntry includes a Zip64 extra information field AND the 'compressed size' > and 'uncompressed size' have the expected Zip64 "magic" value 0x. > This brings ZipInputStream into alignment with the APPNOTE format spec: > > > When extracting, if the zip64 extended information extra > field is present for the file the compressed and > uncompressed sizes will be 8 byte values. > > > While small Zip64 files with 8-byte data descriptors are not commonly found > in the wild, it is possible to create one using the Info-ZIP command line > `-fd` flag: > > `echo hello | zip -fd > hello.zip` > > The PR also adds a test verifying that such a small Zip64 file can be parsed > by ZipInputStream. Eirik Bjørsnøs has updated the pull request incrementally with one additional commit since the last revision: Remove extra whitespace Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/12524/files - new: https://git.openjdk.org/jdk/pull/12524/files/1aedf3e5..cfd53910 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12524&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12524&range=12-13 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12524.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12524/head:pull/12524 PR: https://git.openjdk.org/jdk/pull/12524
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]
On Tue, 16 Jan 2024 13:54:06 GMT, Jaikiran Pai wrote:
>> Eirik Bjørsnøs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Remove trailing whitespace
>> - Remove trailing whitespace
>
> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 664:
>
>> 662:
>> 663: // The LOC's 'compressed size' and 'uncompressed size' must
>> both be marked for Zip64
>> 664: if (csize != ZIP64_MAGICVAL || size != ZIP64_MAGICVAL) {
>
> The spec for this says different. It says:
>
>>
>> 4.4.4 general purpose bit flag:
>> ...
>>Bit 3: If this bit is set, the fields crc-32, compressed size and
>> uncompressed size are set to zero in the local header. The correct values
>> are put in the data descriptor immediately following the compressed data.
>
> So it expects the value zero for the compressed/uncompressed sizes in the LOC
> when the data descriptor bit is set.
The spec isn't terribly helpful in spelling out what should happen in the case
where an entry combines the uses of data descriptors (mandating that CRC, size
and compressed size must be zero) with Zip64 (mandating that size and
compressed size must be 0x)
My interpretation (based in the InfoZIP implementation) is that in such cases,
CRC should be zero, while size and compressed size should be 0x, with
their counterparts in the Zip64 extra field should set to zero.
Does this make sense?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1453526069
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]
On Tue, 16 Jan 2024 14:34:29 GMT, Eirik Bjørsnøs wrote: >(These reads are from a temp buffer, not the stream) Ah! you are right indeed. I didn't correctly read that part of that code. It's reading from a temp buffer which has been fully initialized with a LOC and these reads happens with specific offsets within that buffer. So yes, you are correct that the order won't matter here. Thank you for that detail. - PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1453524828
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v13]
On Tue, 16 Jan 2024 14:32:51 GMT, Eirik Bjørsnøs wrote: >> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the >> number of compressed or uncompressed bytes read from the inflater is larger >> than the Zip64 magic value. >> >> While the ZIP format mandates that the data descriptor `SHOULD be stored in >> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it >> also states that `ZIP64 format MAY be used regardless of the size of a >> file`. For such small entries, the above assumption does not hold. >> >> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the >> ZipEntry includes a Zip64 extra information field AND the 'compressed size' >> and 'uncompressed size' have the expected Zip64 "magic" value 0x. >> This brings ZipInputStream into alignment with the APPNOTE format spec: >> >> >> When extracting, if the zip64 extended information extra >> field is present for the file the compressed and >> uncompressed sizes will be 8 byte values. >> >> >> While small Zip64 files with 8-byte data descriptors are not commonly found >> in the wild, it is possible to create one using the Info-ZIP command line >> `-fd` flag: >> >> `echo hello | zip -fd > hello.zip` >> >> The PR also adds a test verifying that such a small Zip64 file can be parsed >> by ZipInputStream. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove extra whitespace > > Co-authored-by: Andrey Turbanov On a general note - thank you for updating this PR from the previous proposed approach. The current proposed approach of solely relying on the data that comes from within the stream to decide whether or not to use 8 bytes for a data descriptor compressed/uncompressed fields, looks right to me. That prevents issues related to basing this decision on some application controlled/manipulated data which may not match the stream content. - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1893873146
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]
On Tue, 16 Jan 2024 13:42:30 GMT, Jaikiran Pai wrote: >> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 534: >> >>> 532: >>> 533: long csize = get32(tmpbuf, LOCSIZ); >>> 534: long size = get32(tmpbuf, LOCLEN); >> >> Hello Eirik, I suspect this part of the change has an issue. Before reading >> the `tmpbuf` for compressed and uncompressed sizes, there will be 32 bits of >> CRC, which should be read first. This now skips those 32 CRC bits and reads >> them (in the else block) after reading these sizes and that can cause >> incorrect LOC data. > > The Github actions job which runs tier1 is all successful with this proposed > change. So I'm a bit surprised that the tests didn't catch any issues, which > makes me wonder if we have enough test coverage that covers this change. > Hello Eirik, I suspect this part of the change has an issue. Before reading > the `tmpbuf` for compressed and uncompressed sizes, there will be 32 bits of > CRC, which should be read first. This now skips those 32 CRC bits and reads > them (in the else block) after reading these sizes and that can cause > incorrect LOC data. How does the order of the reads from the byte array matter? Outside cache efficiency I would presume they could be read in any order? (These reads are from a temp buffer, not the stream) Could you elaborate? I'm not sure I'm following :-) - PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1453516319
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v13]
> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the > number of compressed or uncompressed bytes read from the inflater is larger > than the Zip64 magic value. > > While the ZIP format mandates that the data descriptor `SHOULD be stored in > ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it > also states that `ZIP64 format MAY be used regardless of the size of a file`. > For such small entries, the above assumption does not hold. > > This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the > ZipEntry includes a Zip64 extra information field AND the 'compressed size' > and 'uncompressed size' have the expected Zip64 "magic" value 0x. > This brings ZipInputStream into alignment with the APPNOTE format spec: > > > When extracting, if the zip64 extended information extra > field is present for the file the compressed and > uncompressed sizes will be 8 byte values. > > > While small Zip64 files with 8-byte data descriptors are not commonly found > in the wild, it is possible to create one using the Info-ZIP command line > `-fd` flag: > > `echo hello | zip -fd > hello.zip` > > The PR also adds a test verifying that such a small Zip64 file can be parsed > by ZipInputStream. Eirik Bjørsnøs has updated the pull request incrementally with one additional commit since the last revision: Remove extra whitespace Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/12524/files - new: https://git.openjdk.org/jdk/pull/12524/files/91fbcce5..1aedf3e5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12524&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12524&range=11-12 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12524.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12524/head:pull/12524 PR: https://git.openjdk.org/jdk/pull/12524
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]
On Wed, 10 Jan 2024 13:39:52 GMT, Eirik Bjørsnøs wrote:
>> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the
>> number of compressed or uncompressed bytes read from the inflater is larger
>> than the Zip64 magic value.
>>
>> While the ZIP format mandates that the data descriptor `SHOULD be stored in
>> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it
>> also states that `ZIP64 format MAY be used regardless of the size of a
>> file`. For such small entries, the above assumption does not hold.
>>
>> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the
>> ZipEntry includes a Zip64 extra information field AND the 'compressed size'
>> and 'uncompressed size' have the expected Zip64 "magic" value 0x.
>> This brings ZipInputStream into alignment with the APPNOTE format spec:
>>
>>
>> When extracting, if the zip64 extended information extra
>> field is present for the file the compressed and
>> uncompressed sizes will be 8 byte values.
>>
>>
>> While small Zip64 files with 8-byte data descriptors are not commonly found
>> in the wild, it is possible to create one using the Info-ZIP command line
>> `-fd` flag:
>>
>> `echo hello | zip -fd > hello.zip`
>>
>> The PR also adds a test verifying that such a small Zip64 file can be parsed
>> by ZipInputStream.
>
> Eirik Bjørsnøs has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Remove trailing whitespace
> - Remove trailing whitespace
src/java.base/share/classes/java/util/zip/ZipInputStream.java line 706:
> 704: * @return true if the extra field is a Zip64 extra field compatible
> with data descriptors
> 705: */
> 706: private static boolean isZip64DataDescriptorField(int headerId,
> byte[] extra, int blockStart, int blockSize) {
I understand the goals of this method - what it's trying to do is, assure the
caller that the extra field/block actually is a zip64 extra block. That
assurance is then used to access the data descriptor content as 8 byte fields.
However, I think in this proposed implementation of this method we are perhaps
doing a bit too much. Specifically, I don't think we should check what values
have been stamped for "Original size" and "Compressed size" fields of this
zip64 block. I think, those values (presence or absence) shouldn't play a role
in deciding whether we have to read a data descriptor size fields as 8 bytes.
Doing these checks for these zip64 original/compressed size fields, I think
will open up more permutations about which zip entries get processed as 8 byte
data descriptors.
Given the context in which this method is used, I think the only checks that we
should do in this method is to verify that the header id is `ZIP64_EXTID`.
Perhaps then this `isZip64DataDescriptorField(...)` won't be needed and we can
just inline that `headerid == ZIP64_EXTID` check inline in the implementation
of `expect64BitDataDescriptor(...)` method
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1453508949
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]
On Wed, 10 Jan 2024 13:39:52 GMT, Eirik Bjørsnøs wrote:
>> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the
>> number of compressed or uncompressed bytes read from the inflater is larger
>> than the Zip64 magic value.
>>
>> While the ZIP format mandates that the data descriptor `SHOULD be stored in
>> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it
>> also states that `ZIP64 format MAY be used regardless of the size of a
>> file`. For such small entries, the above assumption does not hold.
>>
>> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the
>> ZipEntry includes a Zip64 extra information field AND the 'compressed size'
>> and 'uncompressed size' have the expected Zip64 "magic" value 0x.
>> This brings ZipInputStream into alignment with the APPNOTE format spec:
>>
>>
>> When extracting, if the zip64 extended information extra
>> field is present for the file the compressed and
>> uncompressed sizes will be 8 byte values.
>>
>>
>> While small Zip64 files with 8-byte data descriptors are not commonly found
>> in the wild, it is possible to create one using the Info-ZIP command line
>> `-fd` flag:
>>
>> `echo hello | zip -fd > hello.zip`
>>
>> The PR also adds a test verifying that such a small Zip64 file can be parsed
>> by ZipInputStream.
>
> Eirik Bjørsnøs has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Remove trailing whitespace
> - Remove trailing whitespace
src/java.base/share/classes/java/util/zip/ZipInputStream.java line 664:
> 662:
> 663: // The LOC's 'compressed size' and 'uncompressed size' must both
> be marked for Zip64
> 664: if (csize != ZIP64_MAGICVAL || size != ZIP64_MAGICVAL) {
The spec for this says different. It says:
>
> 4.4.4 general purpose bit flag:
> ...
>Bit 3: If this bit is set, the fields crc-32, compressed size and
> uncompressed size are set to zero in the local header. The correct values
> are put in the data descriptor immediately following the compressed data.
So it expects the value zero for the compressed/uncompressed sizes in the LOC
when the data descriptor bit is set.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1453460177
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]
On Wed, 10 Jan 2024 13:39:52 GMT, Eirik Bjørsnøs wrote: >> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the >> number of compressed or uncompressed bytes read from the inflater is larger >> than the Zip64 magic value. >> >> While the ZIP format mandates that the data descriptor `SHOULD be stored in >> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it >> also states that `ZIP64 format MAY be used regardless of the size of a >> file`. For such small entries, the above assumption does not hold. >> >> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the >> ZipEntry includes a Zip64 extra information field AND the 'compressed size' >> and 'uncompressed size' have the expected Zip64 "magic" value 0x. >> This brings ZipInputStream into alignment with the APPNOTE format spec: >> >> >> When extracting, if the zip64 extended information extra >> field is present for the file the compressed and >> uncompressed sizes will be 8 byte values. >> >> >> While small Zip64 files with 8-byte data descriptors are not commonly found >> in the wild, it is possible to create one using the Info-ZIP command line >> `-fd` flag: >> >> `echo hello | zip -fd > hello.zip` >> >> The PR also adds a test verifying that such a small Zip64 file can be parsed >> by ZipInputStream. > > Eirik Bjørsnøs has updated the pull request incrementally with two additional > commits since the last revision: > > - Remove trailing whitespace > - Remove trailing whitespace src/java.base/share/classes/java/util/zip/ZipInputStream.java line 534: > 532: > 533: long csize = get32(tmpbuf, LOCSIZ); > 534: long size = get32(tmpbuf, LOCLEN); Hello Eirik, I suspect this part of the change has an issue. Before reading the `tmpbuf` for compressed and uncompressed sizes, there will be 32 bits of CRC, which should be read first. This now skips those 32 CRC bits and reads them (in the else block) after reading these sizes and that can cause incorrect LOC data. - PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1453442814
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]
On Tue, 16 Jan 2024 13:40:18 GMT, Jaikiran Pai wrote: >> Eirik Bjørsnøs has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Remove trailing whitespace >> - Remove trailing whitespace > > src/java.base/share/classes/java/util/zip/ZipInputStream.java line 534: > >> 532: >> 533: long csize = get32(tmpbuf, LOCSIZ); >> 534: long size = get32(tmpbuf, LOCLEN); > > Hello Eirik, I suspect this part of the change has an issue. Before reading > the `tmpbuf` for compressed and uncompressed sizes, there will be 32 bits of > CRC, which should be read first. This now skips those 32 CRC bits and reads > them (in the else block) after reading these sizes and that can cause > incorrect LOC data. The Github actions job which runs tier1 is all successful with this proposed change. So I'm a bit surprised that the tests didn't catch any issues, which makes me wonder if we have enough test coverage that covers this change. - PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1453445673
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v7]
On Thu, 11 Jan 2024 23:06: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 with a new target base due to a > merge or a rebase. The pull request now contains 22 commits: > > - Merge branch 'openjdk:master' into indexof > - Merge branch 'openjdk:master' into indexof > - Addressing review comments. > - Fix for JDK-8321599 > - Support UU IndexOf > - Only use optimization when EnableX86ECoreOpts is true > - Fix whitespace > - Merge branch 'openjdk:master' into indexof > - Comments; added exhaustive-ish test > - Subtracting 0x10 twice. > - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2 src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 470: > 468: __ jne(L_top_loop_1); > 469: __ jmp(L_0x406019); > 470: For 16 bytes we can directly use [V]PTEST instruction to save multiple loads and compares. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1453429803
Re: RFR: JDK-8323760 putIfAbsent documentation conflicts with itself
On Tue, 16 Jan 2024 11:03:28 GMT, John Hendrikx wrote:
>> src/java.base/share/classes/java/util/Map.java line 820:
>>
>>> 818: * @param key key with which the specified value is to be
>>> associated
>>> 819: * @param value value to be associated with the specified key
>>> 820: * @return {@code null} if the specified key was considered
>>> absent, else returns
>>
>> "Considered" feels out of place. No other method in Map uses it. Try to
>> rephrase that sentence or, if it helps, the complete `@return` tag.
>> (@stuart-marks might have more substantial feedback.)
>
> Yeah, I wasn't sure about that, I can make it more specific, I used
> `considered` here because both unmapped keys and keys mapped to `null` are
> considered to be absent.
I think `absent or {@code null}` is no less concise yet it is way more accurate
than `considered absent`. So something like `@return {@code null} if the
mapping for the specified key is absent or has a {@code null} value`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17438#discussion_r1453427452
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v7]
On Thu, 11 Jan 2024 23:06: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 with a new target base due to a > merge or a rebase. The pull request now contains 22 commits: > > - Merge branch 'openjdk:master' into indexof > - Merge branch 'openjdk:master' into indexof > - Addressing review comments. > - Fix for JDK-8321599 > - Support UU IndexOf > - Only use optimization when EnableX86ECoreOpts is true > - Fix whitespace > - Merge branch 'openjdk:master' into indexof > - Comments; added exhaustive-ish test > - Subtracting 0x10 twice. > - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2 src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 417: > 415: __ cmpl(Address(rbx, r15, Address::times_1, -0x14), rax); > 416: __ jne(L_top_loop_1); > 417: __ jmp(L_0x406019); For cases which are multiple of 4 bytes we can use VMASKMOVPS (conditional moves) and VPTEST. - PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1453425855
Re: [jdk22] RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Tue, 16 Jan 2024 12:23:43 GMT, Chris Hegarty wrote: > Hi all, > > This pull request contains a backport of commit > [ee4d9aa4](https://github.com/openjdk/jdk/commit/ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Chris Hegarty on 16 Jan 2024 and > was reviewed by Alan Bateman. > > Thanks! Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk22/pull/80#pullrequestreview-1823225500
[jdk22] RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer
Hi all, This pull request contains a backport of commit [ee4d9aa4](https://github.com/openjdk/jdk/commit/ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Chris Hegarty on 16 Jan 2024 and was reviewed by Alan Bateman. Thanks! - Commit messages: - Backport ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7 Changes: https://git.openjdk.org/jdk22/pull/80/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=80&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323659 Stats: 75 lines in 2 files changed: 72 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk22/pull/80.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/80/head:pull/80 PR: https://git.openjdk.org/jdk22/pull/80
Integrated: 8323659: LinkedTransferQueue add and put methods call overridable offer
On Fri, 12 Jan 2024 10:38:40 GMT, Chris Hegarty wrote: > Update LinkedTransferQueue add and put methods to not call overridable offer. This pull request has now been integrated. Changeset: ee4d9aa4 Author:Chris Hegarty URL: https://git.openjdk.org/jdk/commit/ee4d9aa4c11c47e7cf15f2742919ac20311f9ea7 Stats: 75 lines in 2 files changed: 72 ins; 0 del; 3 mod 8323659: LinkedTransferQueue add and put methods call overridable offer Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v7]
On Thu, 11 Jan 2024 23:06: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 with a new target base due to a
> merge or a rebase. The pull request now contains 22 commits:
>
> - Merge branch 'openjdk:master' into indexof
> - Merge branch 'openjdk:master' into indexof
> - Addressing review comments.
> - Fix for JDK-8321599
> - Support UU IndexOf
> - Only use optimization when EnableX86ECoreOpts is true
> - Fix whitespace
> - Merge branch 'openjdk:master' into indexof
> - Comments; added exhaustive-ish test
> - Subtracting 0x10 twice.
> - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 197:
> 195: __ bind(L_small_string);
> 196: __ cmpq(r15, 0x20);
> 197: __ ja(L_small_string2);
ja should replaced by jg.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1526:
> 1524: __ movq(rdx, r8);
> 1525: __ movq(rcx, r9);
> 1526: #endif
Can we spill them into XXMs, to save costly stack operations.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1545:
> 1543: // return 0;
> 1544: // }
> 1545: __ movq(r12, rcx);
Kindly use meaningful variable and label names. It will ease the review process
and maintenance.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1551:
> 1549: __ movq(r15, rsi);
> 1550: __ movq(r11, rdi);
> 1551: __ cmpq(rsi, 0x20);
All comparisons are with 32 bit int value , cmpq -> cmpl, may save emitting REX
encoding prefix (no need for setting REX.W).
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1552:
> 1550: __ movq(r11, rdi);
> 1551: __ cmpq(rsi, 0x20);
> 1552: __ jb(L_small_string);
All the comparisons against needle length are signed integer comparisons, so jb
should be replaced by jl
src/hotspot/share/opto/library_call.cpp line 1206:
> 1204:
> 1205: Node* result = nullptr;
> 1206: bool do_intrinsic =
Name change suggestion: do_intrinsic -> call_opt_stub
src/hotspot/share/opto/library_call.cpp line 1229:
> 1227: } else {
> 1228: result = make_indexOf_node(src_start, src_count, tgt_start,
> tgt_count,
> 1229:result_rgn, result_phi, ae);
Existing routines emits IR to handle following special cases.
tgt_cnt > src_cnt return -1
tgt_cnt == 0 return 0.
Should we not be preserving those check before calling stub ?
As of now these checks are part of stub and doing them in JIT code will save
call overhead.
src/hotspot/share/opto/runtime.cpp line 1347:
> 1345: fields[argp++] = TypeInt::INT;// needle length
> 1346: fields[argp++] = TypePtr::NOTNULL;// haystack array
> 1347: fields[argp++] = TypeInt::INT;// haystack length
Do we need to swap the comments? first two arguments corresponds to value
(haystack) as per java side intrinsic signature.
https://github.com/openjdk/jdk/blob/master/src/
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v7]
On Thu, 11 Jan 2024 23:06: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 with a new target base due to a > merge or a rebase. The pull request now contains 22 commits: > > - Merge branch 'openjdk:master' into indexof > - Merge branch 'openjdk:master' into indexof > - Addressing review comments. > - Fix for JDK-8321599 > - Support UU IndexOf > - Only use optimization when EnableX86ECoreOpts is true > - Fix whitespace > - Merge branch 'openjdk:master' into indexof > - Comments; added exhaustive-ish test > - Subtracting 0x10 twice. > - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2 label /add hotspot-compiler-dev - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-1893605792
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v7]
On Thu, 11 Jan 2024 23:06: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 with a new target base due to a
> merge or a rebase. The pull request now contains 22 commits:
>
> - Merge branch 'openjdk:master' into indexof
> - Merge branch 'openjdk:master' into indexof
> - Addressing review comments.
> - Fix for JDK-8321599
> - Support UU IndexOf
> - Only use optimization when EnableX86ECoreOpts is true
> - Fix whitespace
> - Merge branch 'openjdk:master' into indexof
> - Comments; added exhaustive-ish test
> - Subtracting 0x10 twice.
> - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1528:
> 1526: #endif
> 1527:
> 1528: __ subptr(rsp, 0xf0);
Can we spill them into XXMs, to save costly stack operations.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1544:
> 1542: // if (k == 0) {
> 1543: // return 0;
> 1544: // }
Kindly use meaningful variable and label names. It will ease the review process
and maintenance.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1545:
> 1543: // return 0;
> 1544: // }
> 1545: __ movq(r12, rcx);
Check for K == 0 should use rsi.
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1551:
> 1549: __ movq(r15, rsi);
> 1550: __ movq(r11, rdi);
> 1551: __ cmpq(rsi, 0x20);
All comparisons are with 32 bit int value , cmpq -> cmpl, may save emitting
REX encoding prefix (no need for setting REX.W).
src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1552:
> 1550: __ movq(r11, rdi);
> 1551: __ cmpq(rsi, 0x20);
> 1552: __ jb(L_small_string);
All the comparisons against needled / haystack lengths are signed integer
comparisons, so jb should be replaced by jl
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1453226797
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1453227987
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1453245805
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1453250207
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1453294109
Re: RFR: 8291027: Some of TimeZone methods marked 'synchronized' unnecessarily
On Tue, 16 Jan 2024 10:19:44 GMT, Andrey Turbanov wrote:
> 8291027: Some of TimeZone methods marked 'synchronized' unnecessarily
src/java.base/share/classes/java/util/TimeZone.java line 629:
> 627: */
> 628: public static String[] getAvailableIDs(int rawOffset) {
> 629: return ZoneInfo.getAvailableIDs(rawOffset);
BTW can we call `ZoneInfoFile.getZoneIds` here directly?
`ZoneInfo.getAvailableIDs` bridge seems unnecessary.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17441#discussion_r1453280995
Re: RFR: JDK-8323760 putIfAbsent documentation conflicts with itself
On Tue, 16 Jan 2024 10:41:04 GMT, Pavel Rappo wrote:
>> Update the documentation for `@return` tag of `putIfAbsent` to match the
>> main description. `putIfAbsent` uses the same wording as `put` for its
>> `@return` tag, but that is incorrect. `putIfAbsent` never returns the
>> **previous** value, as the whole point of the method is not the replace the
>> value if it was present. As such, if it returns a value, it is the
>> **current** value, and in all other cases it will return `null`.
>
> src/java.base/share/classes/java/util/Map.java line 820:
>
>> 818: * @param key key with which the specified value is to be associated
>> 819: * @param value value to be associated with the specified key
>> 820: * @return {@code null} if the specified key was considered absent,
>> else returns
>
> "Considered" feels out of place. No other method in Map uses it. Try to
> rephrase that sentence or, if it helps, the complete `@return` tag.
> (@stuart-marks might have more substantial feedback.)
Yeah, I wasn't sure about that, I can make it more specific, I used
`considered` here because both unmapped and key maps to `null` is considered to
be absent.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17438#discussion_r1453271276
RFR: 8323794: Remove unused jimage compressor plugin configuration
There's an unused concept of a pluginConfig that is passed into the jlink compress plugins, however we always pass null here and the code seems broken (the pluginConfig wouldn't have been stored properly). This seem to be a left-over from a more generic plugin design that never came to fruition. This PR cleans out this code from the plugins and decompressors, while keeping the compressed header format intact for backwards compatibility (and in case we want to revisit this in the future) - Commit messages: - 8323794: Remove unused jimage compressor plugin configuration Changes: https://git.openjdk.org/jdk/pull/17443/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17443&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323794 Stats: 67 lines in 12 files changed: 2 ins; 39 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/17443.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17443/head:pull/17443 PR: https://git.openjdk.org/jdk/pull/17443
Re: RFR: JDK-8323760 putIfAbsent documentation conflicts with itself
On Tue, 16 Jan 2024 07:40:44 GMT, John Hendrikx wrote:
> Update the documentation for `@return` tag of `putIfAbsent` to match the main
> description. `putIfAbsent` uses the same wording as `put` for its `@return`
> tag, but that is incorrect. `putIfAbsent` never returns the **previous**
> value, as the whole point of the method is not the replace the value if it
> was present. As such, if it returns a value, it is the **current** value,
> and in all other cases it will return `null`.
src/java.base/share/classes/java/util/Map.java line 820:
> 818: * @param key key with which the specified value is to be associated
> 819: * @param value value to be associated with the specified key
> 820: * @return {@code null} if the specified key was considered absent,
> else returns
"Considered" feels out of place. No other method in Map uses it. Try to
rephrase that sentence or, if it helps, the complete `@return` tag.
(@stuart-marks might have more substantial feedback.)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17438#discussion_r1453244818
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v7]
On Thu, 11 Jan 2024 23:06: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 with a new target base due to a
> merge or a rebase. The pull request now contains 22 commits:
>
> - Merge branch 'openjdk:master' into indexof
> - Merge branch 'openjdk:master' into indexof
> - Addressing review comments.
> - Fix for JDK-8321599
> - Support UU IndexOf
> - Only use optimization when EnableX86ECoreOpts is true
> - Fix whitespace
> - Merge branch 'openjdk:master' into indexof
> - Comments; added exhaustive-ish test
> - Subtracting 0x10 twice.
> - ... and 12 more: https://git.openjdk.org/jdk/compare/8e12053e...3e58d0c2
src/hotspot/share/opto/library_call.cpp line 1228:
> 1226: result = _gvn.transform(new ProjNode(call, TypeFunc::Parms));
> 1227: } else {
> 1228: result = make_indexOf_node(src_start, src_count, tgt_start,
> tgt_count,
Existing routines emits IR to handle following special cases.
tgt_cnt > src_cnt return -1
tgt_cnt == 0 return 0.
Should we not be preserving those check before calling stub ?
As of now these checks are part of stub and doing them in JIT code will save
call overhead.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1453223658
RFR: 8291027: Some of TimeZone methods marked 'synchronized' unnecessarily
8291027: Some of TimeZone methods marked 'synchronized' unnecessarily - Commit messages: - 8291027: Some of TimeZone methods marked 'synchronized' unnecessarily Changes: https://git.openjdk.org/jdk/pull/17441/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17441&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8291027 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17441.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17441/head:pull/17441 PR: https://git.openjdk.org/jdk/pull/17441
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:all 6731 670229 0
<<
jtreg:test/jdk:all 9962 995111 0
<<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
Marked as reviewed by alanb (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/17422#pullrequestreview-1822902993
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v7]
> Update LinkedTransferQueue add and put methods to not call overridable offer. Chris Hegarty 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 eight additional commits since the last revision: - Merge branch 'master' into ltq_bug - Merge branch 'master' into ltq_bug - order of tags - Merge branch 'master' into ltq_bug - Update src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java Co-authored-by: Andrey Turbanov - timed offer - add test - Update LinkedTransferQueue add and put methods to not call overridable offer - Changes: - all: https://git.openjdk.org/jdk/pull/17393/files - new: https://git.openjdk.org/jdk/pull/17393/files/3aa026fa..ddaab989 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17393&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17393&range=05-06 Stats: 125 lines in 18 files changed: 86 ins; 30 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17393.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17393/head:pull/17393 PR: https://git.openjdk.org/jdk/pull/17393
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString [v2]
On Mon, 15 Jan 2024 17:10:47 GMT, Jorn Vernee wrote: >> Per Minborg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Rework PathElement:toString > > src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 958: > >> 956: return new >> LayoutPath.PathElementImpl(PathKind.DEREF_ELEMENT, >> 957: LayoutPath::derefElement, >> 958: "*"); > > It seems that this would result in paths like `a.b*` rather than the `*a.b`, > which is correct C syntax. Correct. Additional logic is needed to form a correct C syntax. It would be possible to provide a method that does this. - PR Review Comment: https://git.openjdk.org/jdk/pull/17417#discussion_r1453127266
Re: [jdk22] RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate [v2]
On Mon, 15 Jan 2024 16:23:34 GMT, Alan Bateman wrote: >> Per Minborg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add missing comma in TestScope.java > > test/jdk/java/foreign/TestScope.java line 2: > >> 1: /* >> 2: * Copyright (c) 2023, 2024 Oracle and/or its affiliates. All rights >> reserved. > > Can you hold off on this until TestScope.java's copyright header is fixed in > main line. This has been fixed now. - PR Review Comment: https://git.openjdk.org/jdk22/pull/77#discussion_r1453122625
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 08:52:03 GMT, Alan Bateman wrote: >> Tried not to introduce new `*_all` groups here. `jdk_all` would be the same >> as `jdk:all`, TBH. But we still can do it for symmetry reasons, see new >> commit. > > "all" looks okay but the comment "Catch-all" suggests something else, > shouldn't be "All tests"? Yeah, we can do "All tests" instead. See new commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/17422#discussion_r1453113607
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
> Since recent work to improve `tier4` performance, we actually test
> `tier{1,2,3,4}` often, which includes all the tests in current tree. It would
> be more convenient to just have the `all` test group/alias, so that we can do
> `make test TEST=all`. This also gives a parallelism / run time benefit, as we
> do not wait for tests in each tier to complete before moving to next tier.
>
> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
> environments one also needs to supply a few keywords like `!printer` to skip
> tests that cannot complete without failure due to misconfiguration. I left
> the keywords as is to show how would a failing run look. There is also an
> existing shortcut in build system that allows to run this with `make
> test-all`.
>
>
> % make test TEST=all
>
> Test selection 'all', will run:
> * jtreg:test/hotspot/jtreg:all
> * jtreg:test/jdk:all
> * jtreg:test/langtools:all
> * jtreg:test/jaxp:all
> * jtreg:test/lib-test:all
>
> (...about 6 hours later...)
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>>> jtreg:test/hotspot/jtreg:all 6731 670229 0 <<
>>> jtreg:test/jdk:all 9962 995111 0 <<
>jtreg:test/langtools:all 4469 4469 0 0
>
>jtreg:test/jaxp:all 513 513 0 0
>
>jtreg:test/lib-test:all 3232 0 0
>
> ==
> TEST FAILURE
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
Catch-all -> All tests
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17422/files
- new: https://git.openjdk.org/jdk/pull/17422/files/78f5f9bd..def2f39b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17422&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17422&range=01-02
Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/17422.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17422/head:pull/17422
PR: https://git.openjdk.org/jdk/pull/17422
Re: RFR: 8323515: Create test alias "all" for all test roots [v2]
On Tue, 16 Jan 2024 08:47:38 GMT, Aleksey Shipilev wrote: >> test/jdk/TEST.groups line 28: >> >>> 26: # >>> 27: >>> 28: all = \ >> >> Why no `jdk_all` definition in this case? > > Tried not to introduce new `*_all` groups here. `jdk_all` would be the same > as `jdk:all`, TBH. But we still can do it for symmetry reasons, see new > commit. "all" looks okay but the comment "Catch-all" suggests something else, shouldn't be "All tests"? - PR Review Comment: https://git.openjdk.org/jdk/pull/17422#discussion_r1453103766
Re: RFR: 8323515: Create test alias "all" for all test roots [v2]
On Mon, 15 Jan 2024 22:37:36 GMT, David Holmes wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> jdk_all and lib_test_all groups > > test/jdk/TEST.groups line 28: > >> 26: # >> 27: >> 28: all = \ > > Why no `jdk_all` definition in this case? Tried not to introduce new `*_all` groups here. `jdk_all` would be the same as `jdk:all`, TBH. But we still can do it for symmetry reasons, see new commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/17422#discussion_r1453098855
Re: RFR: 8323515: Create test alias "all" for all test roots [v2]
> Since recent work to improve `tier4` performance, we actually test
> `tier{1,2,3,4}` often, which includes all the tests in current tree. It would
> be more convenient to just have the `all` test group/alias, so that we can do
> `make test TEST=all`. This also gives a parallelism / run time benefit, as we
> do not wait for tests in each tier to complete before moving to next tier.
>
> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
> environments one also needs to supply a few keywords like `!printer` to skip
> tests that cannot complete without failure due to misconfiguration. I left
> the keywords as is to show how would a failing run look. There is also an
> existing shortcut in build system that allows to run this with `make
> test-all`.
>
>
> % make test TEST=all
>
> Test selection 'all', will run:
> * jtreg:test/hotspot/jtreg:all
> * jtreg:test/jdk:all
> * jtreg:test/langtools:all
> * jtreg:test/jaxp:all
> * jtreg:test/lib-test:all
>
> (...about 6 hours later...)
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>>> jtreg:test/hotspot/jtreg:all 6731 670229 0 <<
>>> jtreg:test/jdk:all 9962 995111 0 <<
>jtreg:test/langtools:all 4469 4469 0 0
>
>jtreg:test/jaxp:all 513 513 0 0
>
>jtreg:test/lib-test:all 3232 0 0
>
> ==
> TEST FAILURE
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
jdk_all and lib_test_all groups
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17422/files
- new: https://git.openjdk.org/jdk/pull/17422/files/7f6797b6..78f5f9bd
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17422&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17422&range=00-01
Stats: 8 lines in 2 files changed: 8 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/17422.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17422/head:pull/17422
PR: https://git.openjdk.org/jdk/pull/17422
Re: [jdk22] RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate [v2]
> 8323159: Consider adding some text re. memory zeroing in Arena::allocate Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Add missing comma in TestScope.java - Changes: - all: https://git.openjdk.org/jdk22/pull/77/files - new: https://git.openjdk.org/jdk22/pull/77/files/45c100bb..9fa7cf85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk22&pr=77&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk22&pr=77&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk22/pull/77.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/77/head:pull/77 PR: https://git.openjdk.org/jdk22/pull/77
[jdk22] Integrated: 8322846: Running with -Djdk.tracePinnedThreads set can hang
On Sat, 13 Jan 2024 18:06:11 GMT, Alan Bateman wrote: > Clean backport of P3 issue JDK-8322846. This pull request has now been integrated. Changeset: 30172819 Author:Alan Bateman URL: https://git.openjdk.org/jdk22/commit/3017281956f3c8b50f064a75444c74a18d59e96d Stats: 128 lines in 3 files changed: 100 ins; 12 del; 16 mod 8322846: Running with -Djdk.tracePinnedThreads set can hang Reviewed-by: jpai Backport-of: faa9c6909dda635eb008b9dada6e06fca47c17d6 - PR: https://git.openjdk.org/jdk22/pull/71
[jdk22] Integrated: 8322818: Thread::getStackTrace can fail with InternalError if virtual thread is timed-parked when pinned
On Sat, 13 Jan 2024 18:08:03 GMT, Alan Bateman wrote: > Clean backport of P3 issue JDK-8322818. The test was initially problematic so > I have to include the test-only fixes JDK-8323002 and JDK-8323296 (doing > these as follow-on or dependent PRs wouldn't guarantee that would integrate > at the same time). This pull request has now been integrated. Changeset: 628e31b8 Author:Alan Bateman URL: https://git.openjdk.org/jdk22/commit/628e31b8c1ab425fa61219439c7f7f05fe6ea883 Stats: 126 lines in 2 files changed: 123 ins; 0 del; 3 mod 8322818: Thread::getStackTrace can fail with InternalError if virtual thread is timed-parked when pinned 8323002: test/jdk/java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java times out on macosx-x64 8323296: java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java#id1 timed out Reviewed-by: jpai Backport-of: 4db7a1c3bb6b56cc7416aa27350406da27fe04a8 - PR: https://git.openjdk.org/jdk22/pull/72
