RFR: JDK-8323760 putIfAbsent documentation conflicts with itself
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`. - Commit messages: - Improve putIfAbsent documentation Changes: https://git.openjdk.org/jdk/pull/17438/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17438&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323760 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17438.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17438/head:pull/17438 PR: https://git.openjdk.org/jdk/pull/17438
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Tue, 16 Jan 2024 06:08:28 GMT, Jatin Bhateja wrote: >> Or would that require too many registers? > >> Can the `offset` not be added to `idx_base` before the loop? > > Offset needs to be added to each index element, please refer to API > specification for details. > https://docs.oracle.com/en/java/javase/21/docs/api/jdk.incubator.vector/jdk/incubator/vector/ShortVector.html#fromArray(jdk.incubator.vector.VectorSpecies,short[],int,int[],int) Ah great, thanks for the link 😊 Can you put such equations in the code, using the register names? - PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1453021973
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Tue, 16 Jan 2024 06:08:35 GMT, Jatin Bhateja wrote:
>> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1634:
>>
>>> 1632: Register offset,
>>> XMMRegister offset_vec, XMMRegister idx_vec,
>>> 1633: XMMRegister
>>> xtmp1, XMMRegister xtmp2, XMMRegister xtmp3, KRegister mask,
>>> 1634: KRegister gmask,
>>> int vlen_enc, int vlen) {
>>
>> Would you mind giving a quick summary of what the input registers are and
>> what exactly this method does?
>> Why do we need to call `vgather_subword_avx3` so many times
>> (`lane_count_subwords`)?
>
> Method gathers sub-words from gather indices using integral gather
> instructions, because of the lane size mismatch b/w int and sub-words
> algorithm makes multiple calls to vgather_subword_avx3.
As a reviewer, I feel like I have to reverse engineer this now. I would really
appreciate if there was a proper comment at the beginning, that tells me what
is happening here. Maybe use some equation at the beginning, of what we want to
acheive in the abstract, then explain why that does not work directly, and why
you have to break it down into a loop, and then state the equation again in the
loop form.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1453020617
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Tue, 16 Jan 2024 06:08:40 GMT, Jatin Bhateja wrote: >> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1627: >> >>> 1625: vpsrlvd(dst, dst, xtmp, vlen_enc); >>> 1626: // Pack double word vector into byte vector. >>> 1627: vpackI2X(T_BYTE, dst, ones, xtmp, vlen_enc); >> >> I would prefer if there was less code duplication here. I think there are >> just a few values which you could set to variables, and then apply for both >> versions. > > Meaty part of the algorithm accept different operands, line #1593, #1599 and > #1601, keep two flows for SHORT and BYTE separate will be better maintainable. All I see is replacing a 3 with a 4, etc. It just makes this very long to review, and spot the differences. - PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1453017659
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Tue, 16 Jan 2024 06:08:31 GMT, Jatin Bhateja wrote:
>> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1757:
>>
>>> 1755: for (int i = 0; i < 4; i++) {
>>> 1756: movl(rtmp, Address(idx_base, i * 4));
>>> 1757: pinsrw(dst, Address(base, rtmp, Address::times_2), i);
>>
>> Do I understand this right that you are basically doing this?
>> `dst[i*4 .. i*4 + 3] = load_8bytes(base + (idx_base + i * 4) * 2)`
>> But this does not look like a gather, rather like 4 adjacent loads that pack
>> the data together into a single 8*4 byte vector.
>>
>> Why can this not be done by a simple `32bit` load?
>
> Loop scans over integral index array and pick the work from computed address,
> indexes could be non-contiguous.
Maybe you could have comment lines that state this, similar like in the
documentation?
`dst[i] = load(base + 2 * load(idx_base + i * 4))`
Or maybe:
`dst[i] = base[idx_base[i * 4] * 2]`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1453013821
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Tue, 16 Jan 2024 06:17:43 GMT, Jatin Bhateja wrote:
>> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1900:
>>
>>> 1898: vgather8b(elem_ty, xtmp3, base, idx_base, rtmp, vlen_enc);
>>> 1899: } else {
>>> 1900: LP64_ONLY(vgather8b_masked(elem_ty, xtmp3, base, idx_base,
>>> mask, midx, rtmp, vlen_enc));
>>
>> What happens if if not `LP64_ONLY`?
>
> 32bit skip over check is part of match_rule_supported_vector,
> https://github.com/openjdk/jdk/pull/16354/files#diff-d6a3624f0f0af65a98a47378a5c146eed5016ca09b4de1acd0a3acc823242e82R1921
An assert might still be nice. Or just call the method anyway but then hit an
assert there?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1453005169
Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v5]
On Tue, 16 Jan 2024 06:13:43 GMT, Jatin Bhateja wrote: >> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 5309: >> >>> 5307: assert(bt == T_LONG || bt == T_DOUBLE, ""); >>> 5308: vmovmskpd(rtmp, mask, vec_enc); >>> 5309: shlq(rtmp, 5); // for 64 bit rows (4 longs) >> >> Suggestion: >> >> shlq(rtmp, 5); // for 32 bit rows (4 longs) > > Each long/double permute lane holds 64 bit value. @jatin-bhateja so why do you shift by 5? I thought 4 longs are 32 bit? - PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1453003935
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 657:
> 655: * @param size the value of the 'uncompressed size' field in the LOC
> 656: */
> 657: private boolean expect64BitDataDescriptor(byte[] extra, int flag,
> long csize, long size) {
Suggestion:
private boolean expect64BitDataDescriptor(byte[] extra, int flag, long
csize, long size) {
test/jdk/java/util/zip/ZipInputStream/Zip64DataDescriptor.java line 113:
> 111: // This ZIP has the regular 4-bit data descriptor
> 112:
> 113: byte[] extra = new byte[Long.BYTES + Long.BYTES + Short.BYTES *
> 2]; // Size of a regular Zip64 extra field
Suggestion:
byte[] extra = new byte[Long.BYTES + Long.BYTES + Short.BYTES * 2]; //
Size of a regular Zip64 extra field
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1452996737
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1452996515
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Mon, 15 Jan 2024 14:27:43 GMT, Emanuel Peter wrote:
>> Jatin Bhateja has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 12 commits:
>>
>> - Accelerating masked sub-word gathers for AVX2 targets, this gives
>> additional 1.5-4x speedups over existing implementation.
>> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8318650
>> - Removing JDK-8321648 related changes.
>> - Refined AVX3 implementation with integral gather.
>> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8318650
>> - Fix incorrect comment
>> - Review comments resolutions.
>> - Review comments resolutions.
>> - Review comments resolutions.
>> - Restricting masked sub-word gather to AVX512 target to align with
>> integral gather support.
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/518ec971...de47076e
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1900:
>
>> 1898: vgather8b(elem_ty, xtmp3, base, idx_base, rtmp, vlen_enc);
>> 1899: } else {
>> 1900: LP64_ONLY(vgather8b_masked(elem_ty, xtmp3, base, idx_base, mask,
>> midx, rtmp, vlen_enc));
>
> What happens if if not `LP64_ONLY`?
32bit skip over check is part of match_rule_supported_vector,
https://github.com/openjdk/jdk/pull/16354/files#diff-d6a3624f0f0af65a98a47378a5c146eed5016ca09b4de1acd0a3acc823242e82R1921
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1452969364
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-1893133426
Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v5]
On Mon, 15 Jan 2024 09:10:38 GMT, Emanuel Peter wrote: >> Jatin Bhateja has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Using emulated variable blend E-Core optimized instruction. > > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 5309: > >> 5307: assert(bt == T_LONG || bt == T_DOUBLE, ""); >> 5308: vmovmskpd(rtmp, mask, vec_enc); >> 5309: shlq(rtmp, 5); // for 64 bit rows (4 longs) > > Suggestion: > > shlq(rtmp, 5); // for 32 bit rows (4 longs) Each long/double permute lane holds 64 bit value. - PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1452967063
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Mon, 15 Jan 2024 14:36:38 GMT, Emanuel Peter wrote:
>> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1776:
>>
>>> 1774: for (int i = 0; i < 4; i++) {
>>> 1775: movl(rtmp, Address(idx_base, i * 4));
>>> 1776: addl(rtmp, offset);
>>
>> Can the `offset` not be added to `idx_base` before the loop?
>
> Or would that require too many registers?
> Can the `offset` not be added to `idx_base` before the loop?
Offset needs to be added to each index element, please refer to API
specification for details.
https://docs.oracle.com/en/java/javase/21/docs/api/jdk.incubator.vector/jdk/incubator/vector/ShortVector.html#fromArray(jdk.incubator.vector.VectorSpecies,short[],int,int[],int)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1452964002
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Mon, 15 Jan 2024 13:49:06 GMT, Emanuel Peter wrote:
>> Jatin Bhateja has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 12 commits:
>>
>> - Accelerating masked sub-word gathers for AVX2 targets, this gives
>> additional 1.5-4x speedups over existing implementation.
>> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8318650
>> - Removing JDK-8321648 related changes.
>> - Refined AVX3 implementation with integral gather.
>> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8318650
>> - Fix incorrect comment
>> - Review comments resolutions.
>> - Review comments resolutions.
>> - Review comments resolutions.
>> - Restricting masked sub-word gather to AVX512 target to align with
>> integral gather support.
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/518ec971...de47076e
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1627:
>
>> 1625: vpsrlvd(dst, dst, xtmp, vlen_enc);
>> 1626: // Pack double word vector into byte vector.
>> 1627: vpackI2X(T_BYTE, dst, ones, xtmp, vlen_enc);
>
> I would prefer if there was less code duplication here. I think there are
> just a few values which you could set to variables, and then apply for both
> versions.
Meaty part of the algorithm accept different operands, line #1593, #1599 and
#1601, keep two flows for SHORT and BYTE separate will be better maintainable.
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1634:
>
>> 1632: Register offset,
>> XMMRegister offset_vec, XMMRegister idx_vec,
>> 1633: XMMRegister xtmp1,
>> XMMRegister xtmp2, XMMRegister xtmp3, KRegister mask,
>> 1634: KRegister gmask,
>> int vlen_enc, int vlen) {
>
> Would you mind giving a quick summary of what the input registers are and
> what exactly this method does?
> Why do we need to call `vgather_subword_avx3` so many times
> (`lane_count_subwords`)?
Method gathers sub-words from gather indices using integral gather
instructions, because of the lane size mismatch b/w int and sub-words algorithm
makes multiple calls to vgather_subword_avx3.
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1757:
>
>> 1755: for (int i = 0; i < 4; i++) {
>> 1756: movl(rtmp, Address(idx_base, i * 4));
>> 1757: pinsrw(dst, Address(base, rtmp, Address::times_2), i);
>
> Do I understand this right that you are basically doing this?
> `dst[i*4 .. i*4 + 3] = load_8bytes(base + (idx_base + i * 4) * 2)`
> But this does not look like a gather, rather like 4 adjacent loads that pack
> the data together into a single 8*4 byte vector.
>
> Why can this not be done by a simple `32bit` load?
Loop scans over integral index array and pick the work from computed address,
indexes could be non-contiguous.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1452964120
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1452964077
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1452964030
Re: [jdk22] RFR: 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). Looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk22/pull/72#pullrequestreview-1822389191
Re: [jdk22] RFR: 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. Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk22/pull/71#pullrequestreview-1822387922
Withdrawn: 8320400: Make fields final in 'jdk.internal.util.xml.impl' package
On Tue, 12 Sep 2023 19:59:57 GMT, Andrey Turbanov wrote: > A few classes in `jdk.internal.util.xml.impl` package have non-final fields > which could easily be marked `final`. > > Also fixed a few typos and incorrect javadoc links. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/15691
Re: RFR: 8321620: Optimize JImage decompressors
On Fri, 12 Jan 2024 18:45:39 GMT, Glavo wrote:
> I generated runtime images using `jlink --compress 2 --add-modules
> java.se,jdk.unsupported,jdk.management` and then ran the following JMH
> benchmark:
>
>
> @Warmup(iterations = 10, time = 2)
> @Measurement(iterations = 5, time = 3)
> @Fork(value = 1, jvmArgsAppend = {"-XX:+UseG1GC", "-Xms8g", "-Xmx8g",
> "--add-exports=java.base/jdk.internal.jimage=ALL-UNNAMED"})
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @State(Scope.Benchmark)
> public class Decompress {
>
> private static final ImageReader READER =
> ImageReaderFactory.getImageReader();
> private static final ImageLocation LOC = READER.findLocation("java.base",
> "java/lang/String.class");
>
> @Benchmark
> public ByteBuffer test() {
> return READER.getResourceBuffer(LOC);
> }
>
> }
>
>
>
> This is the result:
>
>
> Zip
>
> Benchmark Mode Cnt Score Error Units
>Score Error Units
> Decompress.test avgt5 194237.534 ± 1026.180 ns/op
> 152855.728 ± 16058.780 ns/op (-21.30%)
> Decompress.test:gc.alloc.rate avgt51197.700 ± 6.306 MB/sec
> 464.278 ±47.465 MB/sec
> Decompress.test:gc.alloc.rate.norm avgt5 243953.338 ± 5.810B/op
>74376.291 ± 2.175B/op (-69.51%)
> Decompress.test:gc.countavgt5 2.000 counts
>1.000 counts
> Decompress.test:gc.time avgt5 4.000 ms
>3.000 ms
>
>
> The results show that memory allocation is reduced by 70% while decompression
> speed is significantly improved.
It'd be very interesting to see zstd compression applied in this context to see
how it'd stack up w.r.t archive size, speed of decompression and so on.
In this specific case we'd probably have to keep the 4-byte magic marker intact
(it might be possible to rid us of this), then compress the remainder (25
bytes) and store the size (a byte might be enough). For reference with regular
zip on representative data I can squeeze headers down then from 29 to 22-24
bytes. In #17433 I'm down to 12 bytes (including marker) with little to no
overhead. I'm not going to pursue getting #17433 integrated (for now), but it
might be a nice little piece of a larger puzzle.
-
PR Comment: https://git.openjdk.org/jdk/pull/17405#issuecomment-1892883726
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. Backporting such a >> change to 21u is then potentially very damaging. > >> 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. Thanks for the reviews @AlanBateman and @DougLea - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1892838717
Re: RFR: 8321620: Optimize JImage decompressors
On Mon, 15 Jan 2024 22:16:13 GMT, Glavo wrote: > In the early development stage of JApp, I used 32-bit integers to store > metadata such as resource size. But after using zstd to compress the > metadata, I was surprised to find that storing them as 64-bit integers can > actually reduce the file size for some test cases. Sorry, I re-looked at the test data and found that I was wrong. Switching 32-bit fields to 64-bit still slightly increased the file size, although I still feel this is inconsequential. - PR Comment: https://git.openjdk.org/jdk/pull/17405#issuecomment-1892838446
Re: RFR: 8323515: Create test alias "all" for all test roots
On Mon, 15 Jan 2024 11:05:09 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
Okay - change is harmless with no ongoing maintenance cost.
test/jdk/TEST.groups line 28:
> 26: #
> 27:
> 28: all = \
Why no `jdk_all` definition in this case?
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17422#pullrequestreview-1822313872
PR Review Comment: https://git.openjdk.org/jdk/pull/17422#discussion_r1452781088
Re: RFR: 8321620: Optimize JImage decompressors
On Mon, 15 Jan 2024 16:08:35 GMT, Claes Redestad wrote: > I'd very much welcome support for zstd, both for resource content and > metadata. A larger JEP-sized project, that. > > I have run a quick experiment with adding a more compact header format > (16-bit sizes for the four fields) and on a `jlink --compress 2 --add-modules > java.se,jdk.unsupported,jdk.management` it reduces the jimage size by about > 0.9% (33279195 vs 32983412 bytes). Throughput might take a small hit. I'm not > sure I have time right now to get tested and PR'd but I'll post the draft > after sponsoring this. >From my experience on the JApp project, if compressed metadata is implemented, >the size of these fields may be irrelevant. In the early development stage of JApp, I used 32-bit integers to store metadata such as resource size. But after using zstd to compress the metadata, I was surprised to find that storing them as 64-bit integers can actually reduce the file size for some test cases. The compression algorithm does the work for me, so I no longer need to care about these details. - PR Comment: https://git.openjdk.org/jdk/pull/17405#issuecomment-1892821541
Re: RFR: 8321620: Optimize JImage decompressors
On Fri, 12 Jan 2024 18:45:39 GMT, Glavo wrote:
> I generated runtime images using `jlink --compress 2 --add-modules
> java.se,jdk.unsupported,jdk.management` and then ran the following JMH
> benchmark:
>
>
> @Warmup(iterations = 10, time = 2)
> @Measurement(iterations = 5, time = 3)
> @Fork(value = 1, jvmArgsAppend = {"-XX:+UseG1GC", "-Xms8g", "-Xmx8g",
> "--add-exports=java.base/jdk.internal.jimage=ALL-UNNAMED"})
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @State(Scope.Benchmark)
> public class Decompress {
>
> private static final ImageReader READER =
> ImageReaderFactory.getImageReader();
> private static final ImageLocation LOC = READER.findLocation("java.base",
> "java/lang/String.class");
>
> @Benchmark
> public ByteBuffer test() {
> return READER.getResourceBuffer(LOC);
> }
>
> }
>
>
>
> This is the result:
>
>
> Zip
>
> Benchmark Mode Cnt Score Error Units
>Score Error Units
> Decompress.test avgt5 194237.534 ± 1026.180 ns/op
> 152855.728 ± 16058.780 ns/op (-21.30%)
> Decompress.test:gc.alloc.rate avgt51197.700 ± 6.306 MB/sec
> 464.278 ±47.465 MB/sec
> Decompress.test:gc.alloc.rate.norm avgt5 243953.338 ± 5.810B/op
>74376.291 ± 2.175B/op (-69.51%)
> Decompress.test:gc.countavgt5 2.000 counts
>1.000 counts
> Decompress.test:gc.time avgt5 4.000 ms
>3.000 ms
>
>
> The results show that memory allocation is reduced by 70% while decompression
> speed is significantly improved.
For reference here's my draft idea on a backwards-compatible compressed
compressed header (heh): https://github.com/openjdk/jdk/pull/17433
No significant difference on micros but a modest saving on the archive size.
-
PR Comment: https://git.openjdk.org/jdk/pull/17405#issuecomment-1892678919
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v6]
On Mon, 15 Jan 2024 17:59:38 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 seven additional > commits since the last revision: > > - 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 Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17393#pullrequestreview-1822167659
Re: New method on java.util.function.Function -- ternary method
Hello Magnus,
Thank you for closing my bug! Terribly sorry to ask another favor, but
could you link the following link for traceability in the JBS submission?
https://mail.openjdk.org/pipermail/amber-dev/2024-January/008488.html
Thank you again for the time and help!
David Alayachew
On Mon, Jan 1, 2024 at 1:26 AM David Alayachew
wrote:
> Hello all,
>
> After reading through Brian Goetz's "Effect cases on Switch"
> (specifically, the "Other switch tricks" section), I have decided to not
> implement this feature after all. The fact is, switch expressions,
> especially with "when-clauses", does a better job of clarifying intent than
> this feature does. Sure, there is slightly more verbosity, but the intent
> of this feature was never to minimize the character count, it was to
> increase clarity. And as is, switch expressions with when clauses
> accomplish that better.
>
> Here is a link to the subsection of the abovementioned article --
> https://inside.java/2023/12/15/switch-case-effect/#other-switch-tricks
>
> Could someone help me close this on JBS? Or should it just not be closed
> at all?
>
> Here is a link to the JBS -- https://bugs.openjdk.org/browse/JDK-8319802
>
> Thank you for your time and help!
> David Alayachew
>
> On Thu, Nov 9, 2023 at 1:19 AM David Alayachew
> wrote:
>
>> Oh, I will also add a specialized version of this method to
>> java.util.function.UnaryOperator. That is because UnaryOperator is a
>> subinterface of Function. Function goes T -> R, but UnaryOperator just goes
>> T -> T. We do not want UnaryOperator to have the T -> R static function
>> included due to inheritance, so it will get its own version that has the
>> same method name and parameters, but the type parameters will be different.
>>
>> Here is another mockup - this time for UnaryOperator2's version of the
>> same method.
>>
>> > interface UnaryOperator2 extends Function2, UnaryOperator
>> > {
>> >
>> > public static UnaryOperator2 ternaryApply
>> > (
>> > Predicate test,
>> > Function trueFunction,
>> > Function falseFunction
>> > )
>> > {
>> >
>> > return
>> > (T input) ->
>> > test.test(input)
>> > ? trueFunction.apply(input)
>> > : falseFunction.apply(input)
>> > ;
>> >
>> > }
>> >
>> > }
>>
>> On Thu, Nov 9, 2023 at 12:12 AM David Alayachew
>> wrote:
>>
>>> It has been a month since I sent this proposal. Since no one has told me
>>> that this is a terrible idea, I will submit this as an enhancement to JBS,
>>> and once the ticket is made, start work on creating a pull request.
>>>
>>> On Tue, Oct 3, 2023 at 3:13 AM David Alayachew
>>> wrote:
>>>
Whoops, bad import.
Please replace the following line with the one after it.
> import java.util.function.Function;
> import java.util.function.*;
On Tue, Oct 3, 2023 at 3:09 AM David Alayachew <
davidalayac...@gmail.com> wrote:
> Hello all,
>
> I have an idea that I want to run by you all -- a new method on
> java.util.function.Function to capture ternary operations.
>
> Here is a mockup of what I wanted to do.
>
> >
> > import java.util.function.Function;
> >
> > @FunctionalInterface
> > public interface Function2 extends Function
> > {
> >
> >public static Function ternary
> >(
> >Predicate test,
> >Function trueOutput,
> >Function falseOutput
> >)
> >{
> >
> > return
> > (I input) ->
> > test.test(input)
> > ? trueOutput.apply(input)
> > : falseOutput.apply(input)
> > ;
> >
> >}
> >
> > }
> >
>
> I think this is useful for a few reasons.
>
> * This composes, just like the ternary operator itself.
>
> * It pairs well with Function.identity() and method references to
> clearly (but concisely) communicate intent.
>
> * Ternary operations are common, so this will find great use by
> developers of all sorts.
>
> There is at least one part I don't quite like about this design - what
> if one (or both!) of your outputs is not a functional transformation of
> the
> input?
>
> For example, String username = id.isBlank() ? "UNKNOWN" : lookup(id);
>
> Obviously, this is easy to work around - simply ignore the input of
> the function. But you lose clarity and simplicity that way. I would put a
> note in the javadoc that says that this method should only be used in
> instances where both outputs are a functional transformation of the input.
> That way, intent is clarified. But if we go that route, maybe this
> function
> should get a better name to capture that? testThenApply? ternaryTransform?
> ternaryApply?
>
> Thank yo
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v6]
On Mon, 15 Jan 2024 17:59:38 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 seven additional > commits since the last revision: > > - 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 Yes, the change looks right to me -- changing the only two calls that could matter here. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1892605207
Re: RFR: 8294977: Convert test/jdk/java tests from ASM library to Classfile API [v9]
On Thu, 21 Sep 2023 11:46:19 GMT, Adam Sotona wrote: >> Chen Liang has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 14 commits: >> >> - Classfile object update >> - Merge branch 'master' into invoke-test-classfile >> - Merge branch 'master' into invoke-test-classfile >> - Switch to ConstantDescs for and void constants >> - Merge AnnotationsTest, remove ModuleTargetAttribute call >> - Merge branch 'invoke-test-classfile' of >> https://github.com/liachmodded/jdk into invoke-test-classfile >> - Update test/jdk/java/lang/invoke/8022701/MHIllegalAccess.java >> >>Co-authored-by: Andrey Turbanov >> - Merge branch 'master' into invoke-test-classfile >> - Fix LambdaStackTrace after running >> - formatting >> - ... and 4 more: https://git.openjdk.org/jdk/compare/526dba1a...d0b6c2ae > > I would integrate conversions that are ready, otherwise the work spend to fix > and reflect changes would double in the preview branch. Keep alive, @asotona can you review this again as it's up-to-date against current master's Classfile API? (i.e. without the instruction renames) - PR Comment: https://git.openjdk.org/jdk/pull/13009#issuecomment-1892590106
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v6]
> 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 seven additional commits since the last revision: - 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/72ad71fb..3aa026fa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17393&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17393&range=04-05 Stats: 350 lines in 17 files changed: 297 ins; 37 del; 16 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
[jdk22] RFR: 8322846: Running with -Djdk.tracePinnedThreads set can hang
Clean backport of P3 issue JDK-8322846. - Commit messages: - Backport faa9c6909dda635eb008b9dada6e06fca47c17d6 Changes: https://git.openjdk.org/jdk22/pull/71/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=71&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322846 Stats: 128 lines in 3 files changed: 100 ins; 12 del; 16 mod Patch: https://git.openjdk.org/jdk22/pull/71.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/71/head:pull/71 PR: https://git.openjdk.org/jdk22/pull/71
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v5]
On Mon, 15 Jan 2024 12:09:48 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 five additional > commits since the last revision: > > - 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 Marked as reviewed by alanb (Reviewer). test/jdk/java/util/concurrent/LinkedTransferQueue/SubclassTest.java line 28: > 26: * @run testng SubclassTest > 27: * @bug 8323659 > 28: * @summary Ensures that the implementation of LTQ add and put methods > does [ORDER OF TAGS](https://openjdk.org/jtreg/tag-spec.html#ORDER) - PR Review: https://git.openjdk.org/jdk/pull/17393#pullrequestreview-1822096706 PR Review Comment: https://git.openjdk.org/jdk/pull/17393#discussion_r1452638878
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v5]
On Mon, 15 Jan 2024 12:34:57 GMT, Doug Lea wrote: > Sorry for needlessly calling overridable versions in these two cases. I > should have caught that. No problem, easy to overlook. I assume you are ok with the changes? If so, could I please ask you to add your review. Otherwise, is there another way that we should proceed? - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1892569945
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString [v2]
On Mon, 15 Jan 2024 16:01:32 GMT, Per Minborg wrote: >> This PR proposes to add an improved Improve >> `LayoutPath.PathElement::toString` method simplifying debugging. >> >> Opinions and suggestions for `static PathElement sequenceElement(long start, >> long step)` are welcome. > > 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/17417#discussion_r1452611709
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 I was initially concerned about this change but I think it's okay. As you say, it expands set the set of ZIP file that can be read by ZipInputStream. I've added myself as reviewer to the CSR. - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1892510671
Integrated: 8323745: Missing comma in copyright header in TestScope
On Mon, 15 Jan 2024 16:25:41 GMT, Per Minborg wrote: > This PR proposed to fix a missing comma in the header of TestScope This pull request has now been integrated. Changeset: edc0ebb7 Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/edc0ebb7803982311e96a8710e73fa920f321992 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8323745: Missing comma in copyright header in TestScope Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/17431
Integrated: 8323745: Missing comma in copyright header in TestScope
This PR proposed to fix a missing comma in the header of TestScope - Commit messages: - Add missing comma in copyright header Changes: https://git.openjdk.org/jdk/pull/17431/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17431&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323745 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17431.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17431/head:pull/17431 PR: https://git.openjdk.org/jdk/pull/17431
Re: Integrated: 8323745: Missing comma in copyright header in TestScope
On Mon, 15 Jan 2024 16:25:41 GMT, Per Minborg wrote: > This PR proposed to fix a missing comma in the header of TestScope Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17431#pullrequestreview-1821998292
Re: RFR: 8275338: Add JFR events for notable serialization situations [v15]
On Mon, 15 Jan 2024 14:17:40 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Removed useless event settings in test. Marked as reviewed by egahlin (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17129#pullrequestreview-1821998747
Re: [jdk22] RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate
On Mon, 15 Jan 2024 16:14:15 GMT, Per Minborg wrote: > 8323159: Consider adding some text re. memory zeroing in Arena::allocate 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. - PR Review Comment: https://git.openjdk.org/jdk22/pull/77#discussion_r1452570361
[jdk22] RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate
8323159: Consider adding some text re. memory zeroing in Arena::allocate - Commit messages: - Backport f5b757ced6b672010ea10575d644d3f9d1728923 Changes: https://git.openjdk.org/jdk22/pull/77/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=77&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323159 Stats: 51 lines in 2 files changed: 49 ins; 0 del; 2 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
Integrated: 8321620: Optimize JImage decompressors
On Fri, 12 Jan 2024 18:45:39 GMT, Glavo wrote:
> I generated runtime images using `jlink --compress 2 --add-modules
> java.se,jdk.unsupported,jdk.management` and then ran the following JMH
> benchmark:
>
>
> @Warmup(iterations = 10, time = 2)
> @Measurement(iterations = 5, time = 3)
> @Fork(value = 1, jvmArgsAppend = {"-XX:+UseG1GC", "-Xms8g", "-Xmx8g",
> "--add-exports=java.base/jdk.internal.jimage=ALL-UNNAMED"})
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @State(Scope.Benchmark)
> public class Decompress {
>
> private static final ImageReader READER =
> ImageReaderFactory.getImageReader();
> private static final ImageLocation LOC = READER.findLocation("java.base",
> "java/lang/String.class");
>
> @Benchmark
> public ByteBuffer test() {
> return READER.getResourceBuffer(LOC);
> }
>
> }
>
>
>
> This is the result:
>
>
> Zip
>
> Benchmark Mode Cnt Score Error Units
>Score Error Units
> Decompress.test avgt5 194237.534 ± 1026.180 ns/op
> 152855.728 ± 16058.780 ns/op (-21.30%)
> Decompress.test:gc.alloc.rate avgt51197.700 ± 6.306 MB/sec
> 464.278 ±47.465 MB/sec
> Decompress.test:gc.alloc.rate.norm avgt5 243953.338 ± 5.810B/op
>74376.291 ± 2.175B/op (-69.51%)
> Decompress.test:gc.countavgt5 2.000 counts
>1.000 counts
> Decompress.test:gc.time avgt5 4.000 ms
>3.000 ms
>
>
> The results show that memory allocation is reduced by 70% while decompression
> speed is significantly improved.
This pull request has now been integrated.
Changeset: a03eb6d3
Author:Glavo
Committer: Claes Redestad
URL:
https://git.openjdk.org/jdk/commit/a03eb6d3f69301616faf13d68be8571a037e5999
Stats: 21 lines in 1 file changed: 10 ins; 5 del; 6 mod
8321620: Optimize JImage decompressors
Reviewed-by: mchung, redestad
-
PR: https://git.openjdk.org/jdk/pull/17405
RFR: 8323515: Create test alias "all" for all test roots
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
-
Commit messages:
- Fix
Changes: https://git.openjdk.org/jdk/pull/17422/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17422&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8323515
Stats: 41 lines in 5 files changed: 34 ins; 5 del; 2 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: 8321620: Optimize JImage decompressors
On Fri, 12 Jan 2024 18:45:39 GMT, Glavo wrote:
> I generated runtime images using `jlink --compress 2 --add-modules
> java.se,jdk.unsupported,jdk.management` and then ran the following JMH
> benchmark:
>
>
> @Warmup(iterations = 10, time = 2)
> @Measurement(iterations = 5, time = 3)
> @Fork(value = 1, jvmArgsAppend = {"-XX:+UseG1GC", "-Xms8g", "-Xmx8g",
> "--add-exports=java.base/jdk.internal.jimage=ALL-UNNAMED"})
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @State(Scope.Benchmark)
> public class Decompress {
>
> private static final ImageReader READER =
> ImageReaderFactory.getImageReader();
> private static final ImageLocation LOC = READER.findLocation("java.base",
> "java/lang/String.class");
>
> @Benchmark
> public ByteBuffer test() {
> return READER.getResourceBuffer(LOC);
> }
>
> }
>
>
>
> This is the result:
>
>
> Zip
>
> Benchmark Mode Cnt Score Error Units
>Score Error Units
> Decompress.test avgt5 194237.534 ± 1026.180 ns/op
> 152855.728 ± 16058.780 ns/op (-21.30%)
> Decompress.test:gc.alloc.rate avgt51197.700 ± 6.306 MB/sec
> 464.278 ±47.465 MB/sec
> Decompress.test:gc.alloc.rate.norm avgt5 243953.338 ± 5.810B/op
>74376.291 ± 2.175B/op (-69.51%)
> Decompress.test:gc.countavgt5 2.000 counts
>1.000 counts
> Decompress.test:gc.time avgt5 4.000 ms
>3.000 ms
>
>
> The results show that memory allocation is reduced by 70% while decompression
> speed is significantly improved.
I'd very much welcome support for zstd, both for resource content and metadata.
A larger JEP-sized project, that.
I have run a quick experiment with adding a more compact header format (16-bit
sizes for the four fields) and on a `jlink --compress 2 --add-modules
java.se,jdk.unsupported,jdk.management` it reduces the jimage size by about
0.9% (33279195 vs 32983412 bytes). Throughput might take a small hit. I'm not
sure I have time right now to get tested and PR'd but I'll post the draft after
sponsoring this.
-
PR Comment: https://git.openjdk.org/jdk/pull/17405#issuecomment-1892445590
Integrated: 8323159: Consider adding some text re. memory zeroing in Arena::allocate
On Mon, 8 Jan 2024 16:16:50 GMT, Per Minborg wrote: > This PR proposes to add a clarification that an `Arena` always returns > zeroed-out segments for `Arena::allocate` methods. > > Note that other overloaded methods refer to the abstract `Arena::allocate` > method via implementation notes. This pull request has now been integrated. Changeset: f5b757ce Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/f5b757ced6b672010ea10575d644d3f9d1728923 Stats: 51 lines in 2 files changed: 49 ins; 0 del; 2 mod 8323159: Consider adding some text re. memory zeroing in Arena::allocate Reviewed-by: mcimadamore, jvernee - PR: https://git.openjdk.org/jdk/pull/17308
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v3]
On Tue, 2 Jan 2024 22:06:36 GMT, Eirik Bjørsnøs wrote: > A CSR for this change has been proposed and is ready for review: > [JDK-8322871](https://bugs.openjdk.org/browse/JDK-8322871) I've reviewed the CSR so you can finalize. The implementation change looks fine. - PR Comment: https://git.openjdk.org/jdk/pull/17209#issuecomment-1892440727
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString [v2]
> This PR proposes to add an improved Improve > `LayoutPath.PathElement::toString` method simplifying debugging. > > Opinions and suggestions for `static PathElement sequenceElement(long start, > long step)` are welcome. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Rework PathElement:toString - Changes: - all: https://git.openjdk.org/jdk/pull/17417/files - new: https://git.openjdk.org/jdk/pull/17417/files/a9c78ea8..83cf10c5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17417&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17417&range=00-01 Stats: 25 lines in 3 files changed: 2 ins; 8 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/17417.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17417/head:pull/17417 PR: https://git.openjdk.org/jdk/pull/17417
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Mon, 15 Jan 2024 14:25:28 GMT, Emanuel Peter wrote:
>> Jatin Bhateja has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 12 commits:
>>
>> - Accelerating masked sub-word gathers for AVX2 targets, this gives
>> additional 1.5-4x speedups over existing implementation.
>> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8318650
>> - Removing JDK-8321648 related changes.
>> - Refined AVX3 implementation with integral gather.
>> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8318650
>> - Fix incorrect comment
>> - Review comments resolutions.
>> - Review comments resolutions.
>> - Review comments resolutions.
>> - Restricting masked sub-word gather to AVX512 target to align with
>> integral gather support.
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/518ec971...de47076e
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1776:
>
>> 1774: for (int i = 0; i < 4; i++) {
>> 1775: movl(rtmp, Address(idx_base, i * 4));
>> 1776: addl(rtmp, offset);
>
> Can the `offset` not be added to `idx_base` before the loop?
Or would that require too many registers?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1452453827
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v10]
On Mon, 1 Jan 2024 14:36:06 GMT, Jatin Bhateja wrote:
>> Hi All,
>>
>> This patch optimizes sub-word gather operation for x86 targets with AVX2 and
>> AVX512 features.
>>
>> Following is the summary of changes:-
>>
>> 1) Intrinsify sub-word gather using hybrid algorithm which initially
>> partially unrolls scalar loop to accumulates values from gather indices into
>> a quadword(64bit) slice followed by vector permutation to place the slice
>> into appropriate vector lanes, it prevents code bloating and generates
>> compact JIT sequence. This coupled with savings from expansive array
>> allocation in existing java implementation translates into significant
>> performance of 1.5-10x gains with included micro on Intel Atom family CPUs
>> and with JVM option UseAVX=2.
>>
>> 
>>
>>
>> 2) For AVX512 targets algorithm uses integral gather instructions to load
>> values from normalized indices which are multiple of integer size, followed
>> by shuffling and packing exact sub-word values from integral lanes.
>>
>> 3) Patch was also compared against modified java fallback implementation by
>> replacing temporary array allocation with zero initialized vector and a
>> scalar loops which inserts gathered values into vector. But, vector insert
>> operation in higher vector lanes is a three step process which first
>> extracts the upper vector 128 bit lane, updates it with gather subword value
>> and then inserts the lane back to its original position. This makes inserts
>> into higher order lanes costly w.r.t to proposed solution. In addition
>> generated JIT code for modified fallback implementation was very bulky. This
>> may impact in-lining decisions into caller contexts.
>>
>> Kindly review and share your feedback.
>>
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 12 commits:
>
> - Accelerating masked sub-word gathers for AVX2 targets, this gives
> additional 1.5-4x speedups over existing implementation.
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8318650
> - Removing JDK-8321648 related changes.
> - Refined AVX3 implementation with integral gather.
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8318650
> - Fix incorrect comment
> - Review comments resolutions.
> - Review comments resolutions.
> - Review comments resolutions.
> - Restricting masked sub-word gather to AVX512 target to align with integral
> gather support.
> - ... and 2 more: https://git.openjdk.org/jdk/compare/518ec971...de47076e
Just had a quick look at this. Is there any support for gather with different
indices for each element in the vector?
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1627:
> 1625: vpsrlvd(dst, dst, xtmp, vlen_enc);
> 1626: // Pack double word vector into byte vector.
> 1627: vpackI2X(T_BYTE, dst, ones, xtmp, vlen_enc);
I would prefer if there was less code duplication here. I think there are just
a few values which you could set to variables, and then apply for both versions.
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1634:
> 1632: Register offset,
> XMMRegister offset_vec, XMMRegister idx_vec,
> 1633: XMMRegister xtmp1,
> XMMRegister xtmp2, XMMRegister xtmp3, KRegister mask,
> 1634: KRegister gmask,
> int vlen_enc, int vlen) {
Would you mind giving a quick summary of what the input registers are and what
exactly this method does?
Why do we need to call `vgather_subword_avx3` so many times
(`lane_count_subwords`)?
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1757:
> 1755: for (int i = 0; i < 4; i++) {
> 1756: movl(rtmp, Address(idx_base, i * 4));
> 1757: pinsrw(dst, Address(base, rtmp, Address::times_2), i);
Do I understand this right that you are basically doing this?
`dst[i*4 .. i*4 + 3] = load_8bytes(base + (idx_base + i * 4) * 2)`
But this does not look like a gather, rather like 4 adjacent loads that pack
the data together into a single 8*4 byte vector.
If so, maybe you should either leave a comment, or even rename the method.
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1776:
> 1774: for (int i = 0; i < 4; i++) {
> 1775: movl(rtmp, Address(idx_base, i * 4));
> 1776: addl(rtmp, offset);
Can the `offset` not be added to `idx_base` before the loop?
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1900:
> 1898: vgather8b(elem_ty, xtmp3, base, idx_base, rtmp, vlen_enc);
> 1899: } else {
> 1900: LP64_ONLY(vgather8b_masked(elem_ty, xtmp3, base, idx_base, mask,
> midx, rtmp, vlen_enc));
What happens if if not `LP64_ONLY`?
-
Changes requested by epeter (Reviewer).
Re: RFR: 8275338: Add JFR events for notable serialization situations [v15]
> Adds serialization misdeclaration events to JFR. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Removed useless event settings in test. - Changes: - all: https://git.openjdk.org/jdk/pull/17129/files - new: https://git.openjdk.org/jdk/pull/17129/files/b76285d9..d0f16b8d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=13-14 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17129/head:pull/17129 PR: https://git.openjdk.org/jdk/pull/17129
Re: RFR: JDK-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
test/jdk/java/lang/StringBuffer/IndexOf.java line 220:
> 218:
> 219: for (int x = 0; x < 100; x++) {
> 220: if(make_new) {
Suggestion:
if (make_new) {
test/jdk/java/lang/StringBuffer/IndexOf.java line 262:
> 260: }
> 261:
> 262: if(make_new)
Suggestion:
if (make_new)
test/jdk/java/lang/StringBuffer/IndexOf.java line 295:
> 293: }
> 294:
> 295: if(make_new) testIndex = getRandomIndex(-100, 100);
Suggestion:
if (make_new) testIndex = getRandomIndex(-100, 100);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1452380458
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1452380633
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1452380799
Re: RFR: 8323621: JDK build should exclude snippet class in java.lang.foreign
On Sun, 14 Jan 2024 04:07:29 GMT, Chen Liang wrote: >> make/modules/java.base/Java.gmk line 41: >> >>> 39: java/lang/classfile/snippet-files \ >>> 40: java/lang/classfile/components/snippet-files \ >>> 41: jdk/lang/foreign/snippet-files >> >> I can't find this directory in the source tree, but there is a >> `src/java.base/share/classes/java/lang/foreign/snippet-files`. Is that what >> you want to exclude? > > If possible, we should simply exclude all files in directories that have `-` > (minus sign) in their name; this is the intentional design to prevent javac > from compiling those classes as package names cannot include `-`. I agree that this piecemeal approach is not good. I think there is a JBS enhancement request to filter all "snippet-files" and "javadoc-files" everywhere. But maybe we can make it broader? Filtering on just `-` makes me a bit nervous though; it seems like it could unintentionally break at some point. But maybe filter out all `-files`? - PR Review Comment: https://git.openjdk.org/jdk/pull/17403#discussion_r1452377059
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 17:53:49 GMT, Erik Gahlin wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Changes according to reviewer's comments. > >> You mean, in the @description annotation? > > Yes. > @egahlin Are you OK with the last commit from the perspective of hotspot-jfr? The JFR product code looks fine. I think the test could be simplified. There is no need to disable threshold or stack trace. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1892171443
Re: RFR: JDK-8319662 ForkJoinPool trims worker threads too slowly [v8]
On Mon, 4 Dec 2023 13:56:55 GMT, Doug Lea wrote: >> This update cascades timeouts to trim subsequent workers after the first >> keepAlive inactive period. > > Doug Lea has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 10 additional commits since > the last revision: > > - Merge branch 'openjdk:master' into JDK-8319662 > - Remove unnecessary re-interrupt > - Merge branch 'openjdk:master' into JDK-8319662 > - Reduce oversignalling and contention; add test > - Revert 2 lines in method scan > - Merge branch 'openjdk:master' into JDK-8319662 > - Split up method awaitWork; other associated changes. > - Merge branch 'openjdk:master' into JDK-8319662 > - tweak cascades; reinstate an @Contended; resolve JDK-8319498 > - Support cascading idle timeouts Thanks I will report back once I tested it. - PR Comment: https://git.openjdk.org/jdk/pull/16725#issuecomment-1892134621
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v5]
On Mon, 15 Jan 2024 12:09:48 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 five additional > commits since the last revision: > > - 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 Sorry for needlessly calling overridable versions in these two cases. I should have caught that. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1892095964
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v5]
> 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 five additional commits since the last revision: - 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/22f2857c..72ad71fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17393&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17393&range=03-04 Stats: 11818 lines in 198 files changed: 5468 ins; 5671 del; 679 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: 8323659: LinkedTransferQueue add and put methods call overridable offer [v4]
On Mon, 15 Jan 2024 09:49:39 GMT, Chris Hegarty wrote: >> Update LinkedTransferQueue add and put methods to not call overridable offer. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java > > Co-authored-by: Andrey Turbanov It's unfortunate that we're only discovering this now, since 21.0.2 is scheduled to release tomorrow, Jan 16th, and we've not yet gotten this reviewed and merged into _master_ or _jdk22_ yet. We can decided how to proceed with the backports once this PR has agreed the approach and is reviewed. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1892039902
Re: RFR: 8323183: ClassFile API performance improvements [v7]
> ClassFile API performance related improvements have been separated from > #17121 into this PR. > > These improvements are important to minimize performance regression of > 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the > Classfile API to generate proxy classes #17121 > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/classfile/impl/StackMapDecoder.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/17306/files - new: https://git.openjdk.org/jdk/pull/17306/files/5f67f8c1..5a04ec59 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17306&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17306&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17306.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17306/head:pull/17306 PR: https://git.openjdk.org/jdk/pull/17306
Re: RFR: JDK-8319662 ForkJoinPool trims worker threads too slowly [v8]
On Sat, 13 Jan 2024 19:46:05 GMT, He-Pin(kerr) wrote: >> Cool, I just encounter this problem, when I migrate to Virtual threads and >> set `jdk.virtualThreadScheduler.maxPoolSize` to 2500. >> >> Will this change backport to j21u, thanks. > >> @He-Pin I tried to backport this patch for you just now [1]. But it can't be >> automatically backported because JDK-8288899 [2][3] also revised the file >> `src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java`. Do you >> want to backport these two patches? Or only this one? >> >> [1] >> https://github.com/openjdk/jdk/commit/cc25d8b12bbab9dde9ade7762927dcb8d27e23c5#commitcomment-136982984 >> [2] https://bugs.openjdk.org/browse/JDK-8288899 >> [3] https://git.openjdk.org/jdk/pull/14301 > > I think the related issue should be backported too. @He-Pin Hi He-Pin, I think @AlanBateman is right, first check if most recent 22-EA build addresses the issue you are seeing. - PR Comment: https://git.openjdk.org/jdk/pull/16725#issuecomment-1891996543
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString
On Mon, 15 Jan 2024 07:57:00 GMT, Per Minborg wrote:
>> This PR proposes to add an improved Improve
>> `LayoutPath.PathElement::toString` method simplifying debugging.
>>
>> Opinions and suggestions for `static PathElement sequenceElement(long start,
>> long step)` are welcome.
>
> test/jdk/java/foreign/TestLayoutPaths.java line 341:
>
>> 339: public void testSequenceElementRangeToString() {
>> 340: PathElement e = PathElement.sequenceElement(2, 4);
>> 341: assertEquals(e.toString(), "sequence range 2 + N * 4, N >= 0");
>
> Not sure this is the best way to express the toString for the range version.
I think there's some choices here. One way possible way to think about paths is
to imagine the consequences of chaining the strings. In the proposed
implementation, this would yield very good/hard to parse results, I think.
Another way would be to borrow some C syntax, to make it look more like an
access expression, so that we we can see something like:
a.b[23].c[].d->e
That said, while the above looks nice to read, it's probably hard to generate,
because when printing out a single element we don't know whether e.g. to emit
`.` or `->` (as a separator).
But, we could still take some inspiration from that, and have the following
string representations:
* `a`
* `b[23]`
* `c[]`
* `d`
* `*e`
Then if a client chains everything, they can use e.g. `/` (or `.`) as a
delimiter and it will print out something sensible and compact.
Back to your question, for the sequence range expression we can use a
Numpy-style syntax:
` : `
So, in this case:
`[2:4]`
Which is, again, relatively compact and "composable".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17417#discussion_r1452210277
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v4]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/17209/files - new: https://git.openjdk.org/jdk/pull/17209/files/33e7756e..96deca07 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17209&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17209&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17209.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17209/head:pull/17209 PR: https://git.openjdk.org/jdk/pull/17209
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 17:53:49 GMT, Erik Gahlin wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Changes according to reviewer's comments. > >> You mean, in the @description annotation? > > Yes. @egahlin Are you OK with the last commit from the perspective of hotspot-jfr? - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1891803118
Re: RFR: 8275338: Add JFR events for notable serialization situations [v14]
On Wed, 10 Jan 2024 18:56:45 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Small space optimization. Anybody from security wants to chime in? - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1891808292
Re: RFR: JDK-8319662 ForkJoinPool trims worker threads too slowly [v8]
On Sat, 13 Jan 2024 19:46:05 GMT, He-Pin(kerr) wrote: > I think the related issue should be backported too. That update is a huge update to FJP that includes API and behavior changes, the CSR has details. Maybe the starting out here is to first see if running with the JDK 22 EA builds address the issue you are seeing. - PR Comment: https://git.openjdk.org/jdk/pull/16725#issuecomment-1891762543
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v3]
On Mon, 15 Jan 2024 01:55:12 GMT, David Holmes 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. - PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1891752374
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v3]
On Mon, 15 Jan 2024 07:31:13 GMT, Andrey Turbanov wrote: >> Chris Hegarty has updated the pull request incrementally with one additional >> commit since the last revision: >> >> timed offer > > src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java > line 1162: > >> 1160: Objects.requireNonNull(e); >> 1161: xfer(e, -1L); >> 1162: return true;} > > Suggestion: > > return true; > } Thank you @turbanoff - accepted and committed. - PR Review Comment: https://git.openjdk.org/jdk/pull/17393#discussion_r1452141389
Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v4]
> Update LinkedTransferQueue add and put methods to not call overridable offer. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/17393/files - new: https://git.openjdk.org/jdk/pull/17393/files/1097a6bc..22f2857c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17393&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17393&range=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 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
[jdk22] RFR: 8322818: Thread::getStackTrace can fail with InternalError if virtual thread is timed-parked when pinned
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). - Commit messages: - JDK-8323002 and JDK-8323296 - Backport 4db7a1c3bb6b56cc7416aa27350406da27fe04a8 Changes: https://git.openjdk.org/jdk22/pull/72/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=72&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322818 Stats: 126 lines in 2 files changed: 123 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk22/pull/72.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/72/head:pull/72 PR: https://git.openjdk.org/jdk22/pull/72
Re: RFR: 8320575: generic type information lost on mandated parameters [v10]
On Thu, 14 Dec 2023 04:00:58 GMT, Vicente Romero wrote:
>> Reflection is not retrieving generic type information for mandated
>> parameters. This is a known issue which has been explicitly stated in the
>> API of some reflection methods. Fix for
>> [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the
>> parameters of compact constructors of record classes `mandated` as specified
>> in the spec. But this implied that users that previous to this change could
>> retrieve the generic type of parameters of compact constructors now they
>> can't anymore. The proposed fix is to try to retrieve generic type
>> information for mandated parameters if available plus changing the spec of
>> the related reflection methods.
>>
>> TIA
>
> Vicente Romero has updated the pull request incrementally with one additional
> commit since the last revision:
>
> adding comment to jcod file
test/jdk/java/lang/reflect/records/RecordReflectionTest.java line 175:
> 173: var constructor = recordClass.getDeclaredConstructors()[0];
> 174: i = 0;
> 175: for(var p: constructor.getParameters()) {
Suggestion:
for (var p: constructor.getParameters()) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17070#discussion_r1452103551
Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v5]
On Tue, 9 Jan 2024 16:48:56 GMT, Jatin Bhateja wrote: >> Hi, >> >> Patch optimizes non-subword vector compress and expand APIs for x86 AVX2 >> only targets. >> Upcoming E-core Xeons (Sierra Forest) and Hybrid CPUs only support AVX2 >> instruction set. >> These are very frequently used APIs in columnar database filter operation. >> >> Implementation uses a lookup table to record permute indices. Table index is >> computed using >> mask argument of compress/expand operation. >> >> Following are the performance number of JMH micro included with the patch. >> >> >> System : Intel(R) Xeon(R) Platinum 8480+ (Sapphire Rapids) >> >> Baseline: >> Benchmark (size) Mode CntScore >> Error Units >> ColumnFilterBenchmark.filterDoubleColumn1024 thrpt2 142.767 >> ops/ms >> ColumnFilterBenchmark.filterDoubleColumn2047 thrpt2 71.436 >> ops/ms >> ColumnFilterBenchmark.filterDoubleColumn4096 thrpt2 35.992 >> ops/ms >> ColumnFilterBenchmark.filterFloatColumn 1024 thrpt2 182.151 >> ops/ms >> ColumnFilterBenchmark.filterFloatColumn 2047 thrpt2 91.096 >> ops/ms >> ColumnFilterBenchmark.filterFloatColumn 4096 thrpt2 44.757 >> ops/ms >> ColumnFilterBenchmark.filterIntColumn 1024 thrpt2 184.099 >> ops/ms >> ColumnFilterBenchmark.filterIntColumn 2047 thrpt2 91.981 >> ops/ms >> ColumnFilterBenchmark.filterIntColumn 4096 thrpt2 45.170 >> ops/ms >> ColumnFilterBenchmark.filterLongColumn 1024 thrpt2 148.017 >> ops/ms >> ColumnFilterBenchmark.filterLongColumn 2047 thrpt2 73.516 >> ops/ms >> ColumnFilterBenchmark.filterLongColumn 4096 thrpt2 36.844 >> ops/ms >> >> Withopt: >> Benchmark (size) Mode Cnt Score >> Error Units >> ColumnFilterBenchmark.filterDoubleColumn1024 thrpt2 2051.707 >>ops/ms >> ColumnFilterBenchmark.filterDoubleColumn2047 thrpt2 914.072 >>ops/ms >> ColumnFilterBenchmark.filterDoubleColumn4096 thrpt2 489.898 >>ops/ms >> ColumnFilterBenchmark.filterFloatColumn 1024 thrpt2 5324.195 >>ops/ms >> ColumnFilterBenchmark.filterFloatColumn 2047 thrpt2 2587.229 >>ops/ms >> ColumnFilterBenchmark.filterFloatColumn 4096 thrpt2 1278.665 >>ops/ms >> ColumnFilterBenchmark.filterIntColumn 1024 thrpt2 4149.384 >>ops/ms >> ColumnFilterBenchmark.filterIntColumn 2047 thrpt ... > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > Using emulated variable blend E-Core optimized instruction. src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 5309: > 5307: assert(bt == T_LONG || bt == T_DOUBLE, ""); > 5308: vmovmskpd(rtmp, mask, vec_enc); > 5309: shlq(rtmp, 5); // for 64 bit rows (4 longs) Suggestion: shlq(rtmp, 5); // for 32 bit rows (4 longs) - PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1452098849
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v4]
> Tests using ProcessTools.executeProcess gets the following output written to > stdout: > [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 > [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 > [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process > 2517117 > > This might be good for some use cases and debugging, but some tests spawns a > large number of processes and for those this output fills up the log files. > > I propose that we add a way to turn of this output for tests where we find > this output to be too noisy. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Update copyright years - Changes: - all: https://git.openjdk.org/jdk/pull/16807/files - new: https://git.openjdk.org/jdk/pull/16807/files/9a43b2c9..4fb2fc21 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16807&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16807&range=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16807.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16807/head:pull/16807 PR: https://git.openjdk.org/jdk/pull/16807
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString
On Mon, 15 Jan 2024 07:56:13 GMT, Per Minborg wrote:
> This PR proposes to add an improved Improve
> `LayoutPath.PathElement::toString` method simplifying debugging.
>
> Opinions and suggestions for `static PathElement sequenceElement(long start,
> long step)` are welcome.
test/jdk/java/foreign/TestLayoutPaths.java line 341:
> 339: public void testSequenceElementRangeToString() {
> 340: PathElement e = PathElement.sequenceElement(2, 4);
> 341: assertEquals(e.toString(), "sequence range 2 + N * 4, N >= 0");
Not sure this is the best way to express the toString for the range version.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17417#discussion_r1452026599
RFR: 8323601: Improve LayoutPath.PathElement::toString
This PR proposes to add an improved Improve `LayoutPath.PathElement::toString` method simplifying debugging. Opinions and suggestions for `static PathElement sequenceElement(long start, long step)` are welcome. - Commit messages: - Add toString to PathElement Changes: https://git.openjdk.org/jdk/pull/17417/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17417&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323601 Stats: 62 lines in 3 files changed: 57 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17417.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17417/head:pull/17417 PR: https://git.openjdk.org/jdk/pull/17417
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v3]
On Tue, 2 Jan 2024 12:21:21 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 two additional
> commits since the last revision:
>
> - Add more extensive testing of combined write/close failure modes
> - Don't suppress if finishException is null, mark stream as closed even when
> closing the wrapped stream failed
test/jdk/java/util/zip/ZipOutputStream/CloseWrappedStream.java line 47:
> 45: */
> 46: @Test
> 47: public void exceptionDuringFinish() {
Suggestion:
public void exceptionDuringFinish() {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1452026379
