Withdrawn: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote: > This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. There is no change for read/write operations on files opened for > direct I/O or when writing to files that are opened with options for > synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery > Kuksenko is polishing benchmarks that includes this area, this is for a > future PR. > > In addition, the blocker mechanism is updated to handle reentrancy as can > happen if debugging code is added to ForkJoinPool, if there is preemption > when attempting to compensate, or potentially forced preemption in the > future. This part is a pre-requisite to the changes to better support object > monitor there are more places where preemption is possible and this quickly > leads to unbalanced begin/end. > > The changes have been baking in the loom repo for several months. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/18598
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 10 Apr 2024 00:41:39 GMT, Brian Burkhalter wrote: > changes look to be the most complicated here but I don't see any problems. I have some changes come that should make this easier to read, I'll update the PR in a few days. - PR Comment: https://git.openjdk.org/jdk/pull/18598#issuecomment-2046613420
Integrated: 8329729: java/util/Properties/StoreReproducibilityTest.java times out
On Tue, 9 Apr 2024 01:13:49 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which proposes to address > the intermittent failures in > `java/util/Properties/StoreReproducibilityTest.java`? This should address > https://bugs.openjdk.org/browse/JDK-8329729. > > These failures in `StoreReproducibilityTest` have been observed in higher > tiers where the test tasks are launched with various JVM options, one of them > being `-Xcomp`. The goal of the `StoreReproducibilityTest` is to verify that > the content which the `java.util.Properties` code generates for a properties > file and reproducible across multiple different runs/launches of an Java > application. To do that it launches a test application (using `java` command) > several times within the test (for different scenarios). That comes up to a > combined total of 25 launches, for different scenarios. Normally each such > launch of the `java` application completes within a second or two. > > Recently, we have been updating our tests to pass along the JVM options that > were used for launching the test task, to the child processes that are > launched from within the tests. That now means that these trivial small java > application that this test launches several times will now be passed the > `-Xcomp` option too (when the test task is launched with that option). It has > been observed that when `-Xcomp` is used to launch those trivial applications > from within the test, each such application takes around 30 seconds to a > minute to complete. This then causes the test to timeout. > > Given the context of this test case, it's not necessary to run this test when > `-Xcomp` is used. The commit in this PR add a `@requires` to disable this > test when `-Xcomp` is present in the test task's JVM args. > > I've run this change in our CI and the test continues to run (and pass) when > `-Xcomp` is absent and is skipped when it is present. This pull request has now been integrated. Changeset: b81b86da Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/b81b86da9849fbc4fb341bff8a23d10aee9967b3 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod 8329729: java/util/Properties/StoreReproducibilityTest.java times out Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/18681
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 16:29:07 GMT, Naoto Sato wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded
>> values
>
> src/java.base/share/classes/java/time/temporal/ChronoField.java line 590:
>
>> 588: * This is necessary to ensure interoperation between calendars.
>> 589: *
>> 590: * Range of {@code InstantSeconds} is between {@link Instant#MIN}
>> and {@link Instant#MAX}
>
> Nit: `InstantSeconds` -> `INSTANT_SECONDS`
Hello Naoto, I had used `InstantSeconds` to keep it consistent with how a
similar doc is used for the `EPOCH_DAY` field. Let me know if you still prefer
this to be `INSTANT_SECONDS` and I will update it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1558644428
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote: > This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. There is no change for read/write operations on files opened for > direct I/O or when writing to files that are opened with options for > synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery > Kuksenko is polishing benchmarks that includes this area, this is for a > future PR. > > In addition, the blocker mechanism is updated to handle reentrancy as can > happen if debugging code is added to ForkJoinPool, if there is preemption > when attempting to compensate, or potentially forced preemption in the > future. This part is a pre-requisite to the changes to better support object > monitor there are more places where preemption is possible and this quickly > leads to unbalanced begin/end. > > The changes have been baking in the loom repo for several months. The `CarrierThread` changes look to be the most complicated here but I don't see any problems. Otherwise, I don't have any comments aside from some that @jaikiran already made hence not worth repeating. - Marked as reviewed by bpb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18598#pullrequestreview-1990512527
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v6]
On Sun, 7 Apr 2024 05:14:08 GMT, Francesco Nigro wrote: >> I went ahead and tried a pure-Java implementation, and it is faster for >> small sizes (up to 8) and only about 1.5x slower for larger sizes, so that >> might make for an interesting fallback if there is no customized assembler >> implementation available or if the size is known to me small. >> >> Ideally, I think we would want C2 to be more aware of setMemory stores, so >> that it can remove redundant stores, like it does with InitializeNode. > > @dean-long in my old PR I have done the same, choosing a (not yet) > configurable cutoff value. > > See https://github.com/openjdk/jdk/pull/16760 As an experiment I added the java code that @franz1981 supplied and ran performance vs. the intrinsic stub. I used 128 bytes as the cutoff value as in that code. I saw about 0.75 to 1ns improvement for sizes of 1 or 2 bytes only. Anything larger and the stub performed better. @mcimadamore Is there any way to disable some of the optimizations C2 will attempt on the IR? We need to maintain atomicity, so vectorization shouldn't occur, for instance. This seems like a rat-hole that would need constant maintenance as C2 optimizations get better. - PR Comment: https://git.openjdk.org/jdk/pull/18555#issuecomment-2046208254
Re: RFR: JDK-8326836 Incorrect `@since` Tags for ClassSignature methods [v2]
On Tue, 9 Apr 2024 21:03:57 GMT, Pavel Rappo wrote: >> Nizar Benalla has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update the copyright year to 2024 > > Trivially, what's the reason you capitalised the word "tags" in the PR title > and the respective JBS issue? @pavelrappo no real reason, that's just how I wrote it in my notes and it persisted until now. I can change it here if you change it in the JBS - PR Comment: https://git.openjdk.org/jdk/pull/18030#issuecomment-2046204353
Re: RFR: JDK-8326836 Incorrect `@since` Tags for ClassSignature methods
On Tue, 9 Apr 2024 20:50:18 GMT, Chen Liang wrote: >> `/covered` > > @nizarbenalla Why don't you add since tags for `superclassSignature` and > `superinterfaceSignatures` methods, both of which have their signatures > changed like the two `of` methods? @liach good find! maybe I should look more carefully into members of Classes annotated with `@PreviewFeature`, I'll keep those for now. Until I can get my tool to programmatically pickup on them. - PR Comment: https://git.openjdk.org/jdk/pull/18030#issuecomment-2046133913
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v11]
On Mon, 11 Mar 2024 19:31:45 GMT, Srinivas Vamsi Parasa wrote: >> Hi Vladimir (@iaroslavski), >> >> Please see the data below. >> >> Thanks, >> Vamsi >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40";> >> >> >> >> >> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> >> > href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> >> >> >> >> >> >> >> >> >> Builder | Size | Stock JDK | b01 | r27b | r27p | r27s >> -- | -- | -- | -- | -- | -- | -- >> RANDOM | 600 | 1.615 | 1.59 | 2.316 | 1.805 | 1.77 >> RANDOM | 2000 | 6.794 | 6.638 | 8.443 | 6.354 | 6.295 >> RANDOM | 9 | 296.877 | 304.15 | 337.625 | 341.999 | 307.099 >> RANDOM | 40 | 838.061 | 801.108 | 1136.688 | 1161.181 | 781.487 >> RANDOM | 300 | 5468.214 | 5452.125 | 8522.698 | 8476.445 | 5368.777 >> PERIOD | 600 | 0.877 | 0.875 | 0.663 | 0.663 | 0.685 >> PERIOD | 2000 | 1.57 | 1.548 | 1.458 | 1.451 | 1.487 >> PERIOD | 9 | 97.208 | 97.677 | 106.01 | 106.516 | 106.629 >> PERIOD | 40 | 237.4 | 264.103 | 235.466 | 231.349 | 231.235 >> PERIOD | 300 | 2604.56 | 2829.935 | 4867.668 | 4872.361 | 4888.391 >> STAGGER | 600 | 1.052 | 1.064 | 0.774 | 0.78 | 0.791 >> STAGGER | 2000 | 3.449 | 3.443 | 2.604 | 2.627 | 2.597 >> STAGGER | 9 | 102.331 | 103.464 | 73.582 | 73.532 | 75.85 >> STAGGER | 40 | 210.829 | 229.37 | 207.356 | 208.565 | 205.141 >> STAGGER | 300 | 2205.565 | 2174.588 | 2086.885 | 2070.132 | 2373.443 >> SHUFFLE | 600 | 1.885 | 1.892 | 1.934 | 1.36 | 1.386 >> SHUFFLE | 2000 | 6.787 | 6.724 | 7.338 | 4.994 | 4.96 >> SHUFFLE | 9 | 158.065 | 154.48 | 152.874 | 148.337 | 140.703 >> SHUFFLE | 40 | 415.089 | 424.777 | 676.272 | 676.89 | 410.717 >> SHUFFLE | 300 | 3999.006 | 4017.496 | 6861.872 | 6894.785 | 3880.883 >> RANDOM | 600 | 1.614 | 1.588 | 2.329 | 1.789 | 1.847 >> RANDOM | 2000 | 6.756 | 6.634 | 7.757 | 6.224 | 6.23 >> RANDOM | 9 | 516.671 | 512.52 | 623.995 | 488.492 | 482.646 >> RANDOM | 40 | 2400.818 | 2399.264 | 2903.654 | 2356.675 | 2358.409 >> RANDOM | 300 | 20933.23 | 20822.49 | 24428.27 | 20847.57 | 20868.68 >> PERIOD | 600 | 0.864 | 0.871 | 0.681 | 0.665 | 0.664 >> PERIOD | 2000 | 1.583 | 1.547 | 1.451 | 1.46 | 1.483 >> PERIOD | 9 | 63.4... > >> Hi Vamsi (@vamsi-parasa), few questions on your test environment: >> >> * what are the hardware specs of your server ? >> * bare-metal or virtual ? >> * are other services or big processes running ? >> * os tuning ? CPU HT: off? Fixed CPU governor or frequency ? >> * isolation using taskset ? >> >> Maybe C2 JIT (+ CDS archive) are given more performance on stock jdk sort >> than same code running outside jdk... >> >> Thanks, Laurent > > Hi Laurent, > > The benchmarks are run on Intel TigerLake Core i7 machine. It's bare-metal > without any virtualization. HT is ON and there is no other specific OS tuning > or isolation using taskset. > > Thanks, > Vamsi Hello Vamsi (@vamsi-parasa), Could you please run the new benchmarking? To save time and don't patch JDK several times, I've created JavaBenchmarkHarness class which is under package java.util and compares several versions of DPQS. Also I prepared several versions of current sorting source from JDK to detect what is going wrong. What you need is to compile and run JavaBenchmarkHarness once: javac --patch-module java.base=. -d classes *.java java -XX:CompileThreshold=1 -XX:-TieredCompilation --patch-module java.base=classes -cp classes java.util.JavaBenchmarkHarness Find the sources there: https://github.com/iaroslavski/sorting/blob/master/radixsort/JavaBenchmarkHarness.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01_ins.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01_mrg.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01_piv.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_b01_prt.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r29p.java https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r29p5.java Thank you, Vladimir - PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-2046090360
Re: RFR: JDK-8326836 Incorrect `@since` Tags for ClassSignature methods [v2]
On Wed, 20 Mar 2024 02:10:35 GMT, Nizar Benalla wrote: >> # Issue >> - [JDK-8326836](https://bugs.openjdk.org/browse/JDK-8326836): changes were >> made to the method signatures but this modification isn't reflected in the @ >> since tags. The @ since tags need to be updated. >> >> I changed the `@since` tags to better accurately show when the methods were >> introduced. This is similar to #18032 and #18373 >> >> For context, I am writing tests to check for accurate use of `@since` tags >> in documentation comments in source code. >> We're following these rules for now: >> >> ### Rule 1: Introduction of New Elements >> >> - If an element is new in JDK N, with no equivalent in JDK N-1, it must >> include `@since N`. >> - Exception: Member elements (fields, methods, nested classes) may omit >> `@since` if their version matches the value specified for the enclosing >> class or interface. >> >> ### Rule 2: Existing Elements in Subsequent JDK Versions >> >> - If an element exists in JDK N, with an equivalent in JDK N-1, it should >> not include `@since N`. >> >> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since` >> >> - When inspecting methods, prioritize the `@since` annotation of the >> supertype's overridden method. >> - If unavailable or if the enclosing class's `@since` is newer, use the >> enclosing element's `@since`. >> >> I.e. if A extends B, and we add a method to B in JDK N, and add an >> override of the method to A in JDK M (M > N), we will use N as the effective >> `@since` for the method. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > update the copyright year to 2024 Trivially, what's the reason you capitalised the word "tags" in the PR title and the respective JBS issue? - PR Comment: https://git.openjdk.org/jdk/pull/18030#issuecomment-2046043571
Re: RFR: JDK-8326836 Incorrect `@since` Tags for ClassSignature methods
On Mon, 11 Mar 2024 15:50:41 GMT, Nizar Benalla wrote: >> # Issue >> - [JDK-8326836](https://bugs.openjdk.org/browse/JDK-8326836): changes were >> made to the method signatures but this modification isn't reflected in the @ >> since tags. The @ since tags need to be updated. >> >> I changed the `@since` tags to better accurately show when the methods were >> introduced. This is similar to #18032 and #18373 >> >> For context, I am writing tests to check for accurate use of `@since` tags >> in documentation comments in source code. >> We're following these rules for now: >> >> ### Rule 1: Introduction of New Elements >> >> - If an element is new in JDK N, with no equivalent in JDK N-1, it must >> include `@since N`. >> - Exception: Member elements (fields, methods, nested classes) may omit >> `@since` if their version matches the value specified for the enclosing >> class or interface. >> >> ### Rule 2: Existing Elements in Subsequent JDK Versions >> >> - If an element exists in JDK N, with an equivalent in JDK N-1, it should >> not include `@since N`. >> >> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since` >> >> - When inspecting methods, prioritize the `@since` annotation of the >> supertype's overridden method. >> - If unavailable or if the enclosing class's `@since` is newer, use the >> enclosing element's `@since`. >> >> I.e. if A extends B, and we add a method to B in JDK N, and add an >> override of the method to A in JDK M (M > N), we will use N as the effective >> `@since` for the method. > > `/covered` @nizarbenalla Why don't you add since tags for `superclassSignature` and `superinterfaceSignatures` methods, both of which have their signatures changed like the two `of` methods? - PR Comment: https://git.openjdk.org/jdk/pull/18030#issuecomment-2046026372
Re: RFR: 8329948: Remove string template feature
On Tue, 9 Apr 2024 11:34:45 GMT, Maurizio Cimadamore wrote: > This PR removes support for the string template feature from the Java > compiler and the Java SE API, as discussed here: > > https://mail.openjdk.org/pipermail/amber-spec-experts/2024-April/004106.html src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 120: > 118: * @since 21 > 119: */ > 120: public static final int MAX_INDY_CONCAT_ARG_SLOTS; May this value change in the future? If yes, we might change this to a method to avoid incorrect eager inlining by the java compiler, or drop this with string templates. src/java.base/share/classes/jdk/internal/util/OctalDigits.java line 44: > 42: * > 43: * @since 21 > 44: */ Spurious javadoc - PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1558241809 PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1558250463
Re: RFR: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v18]
> Allow support for both .a and .so files in AIX. > If .so file is not found, allow fallback to .a extension. > JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) Suchismith Roy has updated the pull request incrementally with two additional commits since the last revision: - test change - test change - Changes: - all: https://git.openjdk.org/jdk/pull/17945/files - new: https://git.openjdk.org/jdk/pull/17945/files/f0459fc3..d2f040b1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17945&range=17 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17945&range=16-17 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17945.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17945/head:pull/17945 PR: https://git.openjdk.org/jdk/pull/17945
Re: RFR: JDK-8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing
On Tue, 9 Apr 2024 15:28:08 GMT, Matthias Baesken wrote: > We have already good JLI tracing capabilities. But GetApplicationHome and > GetApplicationHomeFromDll lack some tracing and should be enhanced. This looks very ad hoc. Not opposed to expanding the set of trace messages when this env variable is set but what is proposed here looks like trace messages that are left over from a debugging session. If we expand the tracing then I think it should be consistent with the existing tracing. - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2045740185
Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams [v2]
On Tue, 9 Apr 2024 10:07:46 GMT, Viktor Klang wrote: >> This PR implements Gatherer-inspired encoding of `flatMap` that shows that >> it is both competitive performance-wise as well as improve correctness. >> >> Below is the performance of `Stream::flatMap` (for reference types): >> >> Before this PR: >> >> Benchmark(size) Mode CntScore Error Units >> FlatMap.par_array10 thrpt 12 294008,937 ? 54369,110 ops/s >> FlatMap.par_array 100 thrpt 1262411,229 ? 14868,119 ops/s >> FlatMap.par_array 1000 thrpt 12 8263,821 ? 452,622 ops/s >> FlatMap.par_iterate 10 thrpt 1223029,978 ? 4274,449 ops/s >> FlatMap.par_iterate 100 thrpt 1210532,907 ? 321,694 ops/s >> FlatMap.par_iterate1000 thrpt 12 981,571 ? 135,270 ops/s >> FlatMap.seq_array10 thrpt 12 2955648,495 ? 32539,142 ops/s >> FlatMap.seq_array 100 thrpt 1241851,009 ? 377,546 ops/s >> FlatMap.seq_array 1000 thrpt 12 1740,281 ? 1229,974 ops/s >> FlatMap.seq_iterate 10 thrpt 12 321727,690 ? 5149,356 ops/s >> FlatMap.seq_iterate 100 thrpt 12 8437,198 ?56,635 ops/s >> FlatMap.seq_iterate1000 thrpt 12 76,994 ? 0,965 ops/s >> >> >> After this PR: >> >> >> Benchmark(size) Mode CntScoreError Units >> FlatMap.par_array10 thrpt 12 283350,051 ? 35567,223 ops/s >> FlatMap.par_array 100 thrpt 1253846,906 ? 19241,913 ops/s >> FlatMap.par_array 1000 thrpt 12 8230,909 ?156,362 ops/s >> FlatMap.par_iterate 10 thrpt 1226328,500 ? 5411,401 ops/s >> FlatMap.par_iterate 100 thrpt 1210470,862 ?249,991 ops/s >> FlatMap.par_iterate1000 thrpt 12 986,511 ?224,050 ops/s >> FlatMap.seq_array10 thrpt 12 5654826,565 ? 27317,453 ops/s >> FlatMap.seq_array 100 thrpt 12 187929,786 ?542,787 ops/s >> FlatMap.seq_array 1000 thrpt 12 2385,346 ? 9,827 ops/s >> FlatMap.seq_iterate 10 thrpt 12 812722,403 ? 160500,399 ops/s >> FlatMap.seq_iterate 100 thrpt 1213542,472 ?118,769 ops/s >> FlatMap.seq_iterate1000 thrpt 12 157,056 ? 1,814 ops/s >> >> >> For streams of size 100k, the following numbers are interesting: >> >> Before this PR: >> >> Benchmark(size) Mode Cnt ScoreError Units >> FlatMap.par_array10 thrpt 12 0,325 ? 0,004 ops/s >> FlatMap.par_iterate 10 thrpt 12 0,106 ? 0,008 o... > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Updating copyright year src/java.base/share/classes/java/util/stream/DoublePipeline.java line 280: > 278: result.sequential().allMatch(this); > 279: else > 280: > result.sequential().forEach(sink::accept); I think that might create a new double consumer instance for every input element. Alternatively you can compute and cache it as a field, replacing `shorts` and use a `null` check. - PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1557995257
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 08:37:29 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to fix the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8212895?
>>
>> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is
>> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and
>> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports
>> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values
>> for the epoch second.
>>
>> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value
>> range to match the supported min and max values of `Instant` (as suggested
>> by Stephen in that JBS issue). This commit also introduces a test to verify
>> this change. This new test method as well as existing tests in tier1, tier2
>> and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded
> values
Thanks, Jai. The code change looks good. (Left a minor nit)
src/java.base/share/classes/java/time/temporal/ChronoField.java line 590:
> 588: * This is necessary to ensure interoperation between calendars.
> 589: *
> 590: * Range of {@code InstantSeconds} is between {@link Instant#MIN}
> and {@link Instant#MAX}
Nit: `InstantSeconds` -> `INSTANT_SECONDS`
-
PR Review: https://git.openjdk.org/jdk/pull/18674#pullrequestreview-1989561065
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1557961266
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 15:15:52 GMT, Jaikiran Pai wrote:
>> This change drops the adjustments to the virtual thread scheduler's target
>> parallelism when virtual threads do file operations on files that are opened
>> for buffered I/O. These operations are usually just too short to have any
>> benefit and may have a negative benefit when reading/writing a small number
>> of bytes. There is no change for read/write operations on files opened for
>> direct I/O or when writing to files that are opened with options for
>> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
>> Kuksenko is polishing benchmarks that includes this area, this is for a
>> future PR.
>>
>> In addition, the blocker mechanism is updated to handle reentrancy as can
>> happen if debugging code is added to ForkJoinPool, if there is preemption
>> when attempting to compensate, or potentially forced preemption in the
>> future. This part is a pre-requisite to the changes to better support object
>> monitor there are more places where preemption is possible and this quickly
>> leads to unbalanced begin/end.
>>
>> The changes have been baking in the loom repo for several months.
>
> src/java.base/share/classes/jdk/internal/misc/CarrierThread.java line 81:
>
>> 79: value = ForkJoinPools.beginCompensatedBlock(getPool());
>> 80: } catch (Throwable e) {
>> 81: if (compensating == COMPENSATE_IN_PROGRESS) {
>
> I don't fully follow these checks `if (compensating ==
> COMPENSATE_IN_PROGRESS) {`. Surely it's not for concurrent access (given the
> `assert` at the start of this method, this code path happens in a single
> thread). So I think these checks are about the re-entrancy that is mentioned
> in the description of this PR.
> In the context of re-entrancy, if I am reading the code correctly, I don't
> see how a re-entrant call would end up on this line (and other similar lines)
> in this top level `if` block. When a thread enters the top level `if` block
> it immediately sets the `compensating` to `COMPENSATE_IN_PROGRESS`:
>
>
> if (compensating == NOT_COMPENSATING) {
> compensating = COMPENSATE_IN_PROGRESS;
> ...
>
> So a subsequent re-entrant call would never enter that top level `if` block
> again. Which leads me to question the need of these additional `if
> (compensating == COMPENSATE_IN_PROGRESS) {` checks. I feel like I am missing
> something in this code.
> In the context of re-entrancy, if I am reading the code correctly, I don't
> see how a re-entrant call would end up on this line (and other similar lines)
> in this top level if block. When a thread enters the top level if block it
> immediately sets the compensating to COMPENSATE_IN_PROGRESS: I feel like I am
> missing something in this code.
These are fields on the carrier. If a virtual thread is preempted when
compensating (or in the progress of) then it may continue on a different
carrier or the original carrier may execute the task for a different virtual
thread that does I/O. These are the cases that are handled here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557950295
Re: RFR: JDK-8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing
On Tue, 9 Apr 2024 15:28:08 GMT, Matthias Baesken wrote: > We have already good JLI tracing capabilities. But GetApplicationHome and > GetApplicationHomeFromDll lack some tracing and should be enhanced. Btw. I noticed that GetModuleFileName return value is not checked, should this be done (at other locations in the JDk codebase it is done) ? See https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamea - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2045514750
Integrated: 8325659: Normalize Random usage by incubator vector tests
On Mon, 8 Apr 2024 10:50:00 GMT, Evgeny Nikitin wrote: > Improve RNG usage in said tests: > > 1. The RNG is not created by our Utils class, as suggested in our JTReg tests; > 2. The seed, accordingly, is not a fixed value now, but truly random; > 3. The tests that had been creating their own Random instances, are now using > RAND static member from the AbstractVectorTest; > 4. The generated tests sources have been re-generated. > > The most important change is #2, it could add variability and help cover more > JIT Compiler and Runtime scenarios. This pull request has now been integrated. Changeset: 4bba445d Author:Evgeny Nikitin Committer: Paul Sandoz URL: https://git.openjdk.org/jdk/commit/4bba445d835837db5ab145edb24030fc6f42ec24 Stats: 469 lines in 69 files changed: 188 ins; 0 del; 281 mod 8325659: Normalize Random usage by incubator vector tests Reviewed-by: psandoz - PR: https://git.openjdk.org/jdk/pull/18675
RFR: JDK-8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing
We have already good JLI tracing capabilities. But GetApplicationHome and GetApplicationHomeFromDll lack some tracing and should be enhanced. - Commit messages: - JDK-8329862 Changes: https://git.openjdk.org/jdk/pull/18699/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18699&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329862 Stats: 18 lines in 3 files changed: 15 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18699.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18699/head:pull/18699 PR: https://git.openjdk.org/jdk/pull/18699
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 14:50:00 GMT, Alan Bateman wrote: > It's to increase the target parallelism for the duration of the transfer > rather than specific read operations. Do you think we would need to do something similar in`PipeInputStream` that belongs to `Process`? - PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557842319
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/jdk/internal/misc/CarrierThread.java line 81:
> 79: value = ForkJoinPools.beginCompensatedBlock(getPool());
> 80: } catch (Throwable e) {
> 81: if (compensating == COMPENSATE_IN_PROGRESS) {
I don't fully follow these checks `if (compensating == COMPENSATE_IN_PROGRESS)
{`. Surely it's not for concurrent access (given the `assert` at the start of
this method, this code path happens in a single thread). So I think these
checks are about the re-entrancy that is mentioned in the description of this
PR.
In the context of re-entrancy, if I am reading the code correctly, I don't see
how a re-entrant call would end up on this line (and other similar lines) in
this top level `if` block. When a thread enters the top level `if` block it
immediately sets the `compensating` to `COMPENSATE_IN_PROGRESS`:
if (compensating == NOT_COMPENSATING) {
compensating = COMPENSATE_IN_PROGRESS;
...
So a subsequent re-entrant call would never enter that top level `if` block
again. Which leads me to question the need of these additional `if
(compensating == COMPENSATE_IN_PROGRESS) {` checks. I feel like I am missing
something in this code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557834582
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 14:59:00 GMT, Jaikiran Pai wrote: > Was the use the word `tryCompensate` intentional here? I don't see that > method in this class or in the implementation of > `CarrierThread.beginBlocking()` The FJP method is named tryCompensate and the same method name was used here at one point. - PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557814857
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote: > This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. What I understand of these changes overall is that, we no longer consider most of the file operations that work against an actual file as needing to be compensated by the ForkJoinPool. It doesn't matter even if the read() is being done with a large buffer - we still consider it an operation that doesn't deserve a compensation. The only file operations where we do appear to compensate is certain `write` operations which are backed by `O_SYNC`, `O_DSYNC` or direct IO writes. The places where `FileOutputStream` is used against a file descriptor which isn't a file have been update to continue to using the `Blocker` to potentially compensate the operations. Overall, based on my limited knowledge of this area, the changes look OK to me. There are a few questions I had which I've added inline. The copyright years on many of the files will need an update. - PR Comment: https://git.openjdk.org/jdk/pull/18598#issuecomment-2045417779
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote: > This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. There is no change for read/write operations on files opened for > direct I/O or when writing to files that are opened with options for > synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery > Kuksenko is polishing benchmarks that includes this area, this is for a > future PR. > > In addition, the blocker mechanism is updated to handle reentrancy as can > happen if debugging code is added to ForkJoinPool, if there is preemption > when attempting to compensate, or potentially forced preemption in the > future. This part is a pre-requisite to the changes to better support object > monitor there are more places where preemption is possible and this quickly > leads to unbalanced begin/end. > > The changes have been baking in the loom repo for several months. src/java.base/share/classes/jdk/internal/misc/Blocker.java line 79: > 77: * Marks the beginning of a possibly blocking operation. > 78: * @param blocking true if the operation may block, otherwise false > 79: * @return true if tryCompensate attempted Was the use the word `tryCompensate` intentional here? I don't see that method in this class or in the implementation of `CarrierThread.beginBlocking()` - PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557805449
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 13:58:31 GMT, Jaikiran Pai wrote:
>> This change drops the adjustments to the virtual thread scheduler's target
>> parallelism when virtual threads do file operations on files that are opened
>> for buffered I/O. These operations are usually just too short to have any
>> benefit and may have a negative benefit when reading/writing a small number
>> of bytes. There is no change for read/write operations on files opened for
>> direct I/O or when writing to files that are opened with options for
>> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
>> Kuksenko is polishing benchmarks that includes this area, this is for a
>> future PR.
>>
>> In addition, the blocker mechanism is updated to handle reentrancy as can
>> happen if debugging code is added to ForkJoinPool, if there is preemption
>> when attempting to compensate, or potentially forced preemption in the
>> future. This part is a pre-requisite to the changes to better support object
>> monitor there are more places where preemption is possible and this quickly
>> leads to unbalanced begin/end.
>>
>> The changes have been baking in the loom repo for several months.
>
> src/java.base/share/classes/java/lang/System.java line 2262:
>
>> 2260: @Override
>> 2261: public long transferTo(OutputStream out) throws IOException {
>> 2262: boolean attempted = Blocker.begin();
>
> Hello Alan, why do we mark transferTo as potentially blocking operation which
> might need compensation? As far as I can see in the underlying implementation
> of `FileInputStream.transferTo()` it either calls the
> `FileChannel.transferTo()` or does a `in.read()` followed by `out.write()`.
> In either of those cases wouldn't the underlying `InputStream/OutputStream`
> already have the necessary `Blocker` calls?
It's to increase the target parallelism for the duration of the transfer rather
than specific read operations. System.in.transferTo(out) should be rare so it's
not super important of course.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557786640
Integrated: 8329955: Class-File API ClassPrinter does not print bootstrap methods arguments
On Tue, 9 Apr 2024 13:39:47 GMT, Adam Sotona wrote: > Class-File API `ClassPrinter` prints many useful info about bootstrap methods > and invoke dynamic instructions, however bootstrap methods arguments are > missing. > This patch fixes bootstrap methods and invoke dynamic instructions printing. > `ClassPrinterTest` is extended to print bootstrap method and invoke dynamic > instruction. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: e75e1cb0 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/e75e1cb02c3d115762846e47fb2d2b10a177f6de Stats: 335 lines in 2 files changed: 71 ins; 0 del; 264 mod 8329955: Class-File API ClassPrinter does not print bootstrap methods arguments Reviewed-by: briangoetz - PR: https://git.openjdk.org/jdk/pull/18695
Re: RFR: 8329955: Class-File API ClassPrinter does not print bootstrap methods arguments
On Tue, 9 Apr 2024 14:21:59 GMT, Brian Goetz wrote: >> Class-File API `ClassPrinter` prints many useful info about bootstrap >> methods and invoke dynamic instructions, however bootstrap methods arguments >> are missing. >> This patch fixes bootstrap methods and invoke dynamic instructions printing. >> `ClassPrinterTest` is extended to print bootstrap method and invoke dynamic >> instruction. >> >> Please review. >> >> Thanks, >> Adam > > Marked as reviewed by briangoetz (Reviewer). @briangoetz Thank you! - PR Comment: https://git.openjdk.org/jdk/pull/18695#issuecomment-2045359689
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 13:53:11 GMT, Jaikiran Pai wrote:
>> This change drops the adjustments to the virtual thread scheduler's target
>> parallelism when virtual threads do file operations on files that are opened
>> for buffered I/O. These operations are usually just too short to have any
>> benefit and may have a negative benefit when reading/writing a small number
>> of bytes. There is no change for read/write operations on files opened for
>> direct I/O or when writing to files that are opened with options for
>> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
>> Kuksenko is polishing benchmarks that includes this area, this is for a
>> future PR.
>>
>> In addition, the blocker mechanism is updated to handle reentrancy as can
>> happen if debugging code is added to ForkJoinPool, if there is preemption
>> when attempting to compensate, or potentially forced preemption in the
>> future. This part is a pre-requisite to the changes to better support object
>> monitor there are more places where preemption is possible and this quickly
>> leads to unbalanced begin/end.
>>
>> The changes have been baking in the loom repo for several months.
>
> src/java.base/share/classes/java/lang/System.java line 2279:
>
>> 2277: }
>> 2278:
>> 2279: public void write(int b) throws IOException {
>
> Nit - missing an `@Override`
Okay.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557769608
Integrated: 8325371: Missing ClassFile.Option in package summary
On Wed, 3 Apr 2024 08:48:43 GMT, Adam Sotona wrote: > Some of the Class-File API options were not mentioned in the package summary > and one exception mentioned `ClassFile.DeadLabelsOption` javadoc was wrong. > This patch fixes the javadoc. > > Please review. > > Thank you, > Adam This pull request has now been integrated. Changeset: f9bc2db9 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/f9bc2db9a9b8e5b7314fba5f70cb79e07ed02bd8 Stats: 23 lines in 2 files changed: 12 ins; 6 del; 5 mod 8325371: Missing ClassFile.Option in package summary Reviewed-by: briangoetz - PR: https://git.openjdk.org/jdk/pull/18594
Re: RFR: 8325371: Missing ClassFile.Option in package summary
On Tue, 9 Apr 2024 14:22:37 GMT, Brian Goetz wrote: >> Some of the Class-File API options were not mentioned in the package summary >> and one exception mentioned `ClassFile.DeadLabelsOption` javadoc was wrong. >> This patch fixes the javadoc. >> >> Please review. >> >> Thank you, >> Adam > > Marked as reviewed by briangoetz (Reviewer). @briangoetz Thank you! - PR Comment: https://git.openjdk.org/jdk/pull/18594#issuecomment-2045353899
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 14:20:51 GMT, Jaikiran Pai wrote:
>> This change drops the adjustments to the virtual thread scheduler's target
>> parallelism when virtual threads do file operations on files that are opened
>> for buffered I/O. These operations are usually just too short to have any
>> benefit and may have a negative benefit when reading/writing a small number
>> of bytes. There is no change for read/write operations on files opened for
>> direct I/O or when writing to files that are opened with options for
>> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
>> Kuksenko is polishing benchmarks that includes this area, this is for a
>> future PR.
>>
>> In addition, the blocker mechanism is updated to handle reentrancy as can
>> happen if debugging code is added to ForkJoinPool, if there is preemption
>> when attempting to compensate, or potentially forced preemption in the
>> future. This part is a pre-requisite to the changes to better support object
>> monitor there are more places where preemption is possible and this quickly
>> leads to unbalanced begin/end.
>>
>> The changes have been baking in the loom repo for several months.
>
> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 304:
>
>> 302: return 0;
>> 303: do {
>> 304: boolean attempted = Blocker.begin(sync | direct);
>
> Is the use of the bitwise operator `|` instead of `||` on `boolean` fields,
> here and other places, intentional?
Well spotted, these should be ||.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557761764
Re: RFR: 8329099: Undocumented exception thrown by Instruction factory methods accepting Opcode [v4]
On Tue, 9 Apr 2024 09:32:38 GMT, Adam Sotona wrote: >> `IllegalArgumentException` thrown by some static factory methods of the >> following `java.lang.classfile.instruction` interfaces are not documented: >> >> - `ArrayLoadInstruction` >> - `ArrayStoreInstruction` >> - `BranchInstruction` >> - `ConvertInstruction` >> - `DiscontinuedInstruction` >> - `FieldInstruction` >> - `InvokeInstruction` >> - `LoadInstruction` >> - `MonitorInstruction` >> - `NewPrimitiveArrayInstruction` >> - `OperatorInstruction` >> - `ReturnInstruction` >> - `StackInstruction` >> - `StoreInstruction` >> - `TypeCheckInstruction` >> >> `NewPrimitiveArrayInstruction::of` factory method also does not perform the >> check for invalid argument. >> >> This patch adds all the missing `@throws` Javadoc tags and fixes >> `NewPrimitiveArrayInstruction::of` factory method to perform the argument >> check. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > added NPE statement to package-info While the @throws is not technically necessary here, specifying it here is reasonable. - Marked as reviewed by briangoetz (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18578#pullrequestreview-1989201955
Re: RFR: 8325371: Missing ClassFile.Option in package summary
On Wed, 3 Apr 2024 08:48:43 GMT, Adam Sotona wrote: > Some of the Class-File API options were not mentioned in the package summary > and one exception mentioned `ClassFile.DeadLabelsOption` javadoc was wrong. > This patch fixes the javadoc. > > Please review. > > Thank you, > Adam Marked as reviewed by briangoetz (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18594#pullrequestreview-1989198603
Re: RFR: 8329955: Class-File API ClassPrinter does not print bootstrap methods arguments
On Tue, 9 Apr 2024 13:39:47 GMT, Adam Sotona wrote: > Class-File API `ClassPrinter` prints many useful info about bootstrap methods > and invoke dynamic instructions, however bootstrap methods arguments are > missing. > This patch fixes bootstrap methods and invoke dynamic instructions printing. > `ClassPrinterTest` is extended to print bootstrap method and invoke dynamic > instruction. > > Please review. > > Thanks, > Adam Marked as reviewed by briangoetz (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18695#pullrequestreview-1989196787
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 304:
> 302: return 0;
> 303: do {
> 304: boolean attempted = Blocker.begin(sync | direct);
Is the use of the bitwise operator `|` instead of `||` on `boolean` fields,
here and other places, intentional?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557727112
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/jdk/internal/misc/CarrierThread.java line 104:
> 102: if (compensating == COMPENSATING) {
> 103: ForkJoinPools.endCompensatedBlock(getPool(),
> compensateValue);
> 104: compensating = NOT_COMPENSATING;
For the sake for consistency between the state and the `compensateValue`, would
it be good to reset `compensateValue` to `0` when we switch to
`NOT_COMPENSATING`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557714608
Re: RFR: 8323707: Adjust Classfile API's type arg model to better represent the embodied type [v4]
On Mon, 26 Feb 2024 17:48:53 GMT, Chen Liang wrote: >> API changes as discussed on the mailing list: >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-November/000419.html >> >> Additional questions: >> 1. Whether to rename `WildcardIndicator.DEFAULT` to `NONE` > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 13 commits: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/typearg-model > - Missed renaming in tests, thanks to Adam Sotona > >Co-authored-by: Adam Sotona <10807609+asot...@users.noreply.github.com> > - Rename no wildcard indicator, improve docs slightly > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/typearg-model > - Merge branch 'master' into fix/typearg-model > - redundant line > - Fix a test in langtools, copyright year > - Merge branch 'master' of https://github.com/openjdk/jdk into > fix/typearg-model > - Implementation cleanup, test update > - Merge branch 'master' into fix/typearg-model > - ... and 3 more: https://git.openjdk.org/jdk/compare/f62b5789...839efabd Looks good, thanks. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16517#pullrequestreview-1989125867
Re: RFR: JDK-8326836 Incorrect `@since` Tags for ClassSignature methods [v2]
On Wed, 20 Mar 2024 02:10:35 GMT, Nizar Benalla wrote: >> # Issue >> - [JDK-8326836](https://bugs.openjdk.org/browse/JDK-8326836): changes were >> made to the method signatures but this modification isn't reflected in the @ >> since tags. The @ since tags need to be updated. >> >> I changed the `@since` tags to better accurately show when the methods were >> introduced. This is similar to #18032 and #18373 >> >> For context, I am writing tests to check for accurate use of `@since` tags >> in documentation comments in source code. >> We're following these rules for now: >> >> ### Rule 1: Introduction of New Elements >> >> - If an element is new in JDK N, with no equivalent in JDK N-1, it must >> include `@since N`. >> - Exception: Member elements (fields, methods, nested classes) may omit >> `@since` if their version matches the value specified for the enclosing >> class or interface. >> >> ### Rule 2: Existing Elements in Subsequent JDK Versions >> >> - If an element exists in JDK N, with an equivalent in JDK N-1, it should >> not include `@since N`. >> >> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since` >> >> - When inspecting methods, prioritize the `@since` annotation of the >> supertype's overridden method. >> - If unavailable or if the enclosing class's `@since` is newer, use the >> enclosing element's `@since`. >> >> I.e. if A extends B, and we add a method to B in JDK N, and add an >> override of the method to A in JDK M (M > N), we will use N as the effective >> `@since` for the method. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > update the copyright year to 2024 Looks good, thanks. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18030#pullrequestreview-1989118816
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/java/lang/System.java line 2262:
> 2260: @Override
> 2261: public long transferTo(OutputStream out) throws IOException {
> 2262: boolean attempted = Blocker.begin();
Hello Alan, why do we mark transferTo as potentially blocking operation which
might need compensation? As far as I can see in the underlying implementation
of `FileInputStream.transferTo()` it either calls the
`FileChannel.transferTo()` or does a `in.read()` followed by `out.write()`. In
either of those cases wouldn't the underlying `InputStream/OutputStream`
already have the necessary `Blocker` calls?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557679570
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/java/lang/System.java line 2279:
> 2277: }
> 2278:
> 2279: public void write(int b) throws IOException {
Nit - missing an `@Override`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557670428
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 08:34:39 GMT, Jaikiran Pai wrote:
>> Should `INSTANT_SECONDS("InstantSeconds", SECONDS, FOREVER,
>> ValueRange.of(Instant.MIN.getEpochSecond(), Instant.MAX.getEpochSecond())),
>> ` work?
>
> Hello Naoto, that's a very good point. I was too caught up with the constant
> values that it didn't occur to me that the `Instant.MIN` and `Instant.MAX`
> public fields could be used for this. I have followed your suggestion and
> updated the PR. I have also updated the javadoc to link to `Instant.MIN` and
> `Instant.MAX` as the supported epoch second range.
>
> The test continues to pass with this change and fails (as expected) without
> the source change.
Good, that was going to be my backup suggestion; trying to avoid a method call
even for the init. :)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1557657347
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 08:37:29 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix the issue >> noted in https://bugs.openjdk.org/browse/JDK-8212895? >> >> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is >> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and >> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports >> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values >> for the epoch second. >> >> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value >> range to match the supported min and max values of `Instant` (as suggested >> by Stephen in that JBS issue). This commit also introduces a test to verify >> this change. This new test method as well as existing tests in tier1, tier2 >> and tier3 continue to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded > values Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18674#pullrequestreview-1989084996
RFR: 8329955: Class-File API ClassPrinter does not print bootstrap methods arguments
Class-File API `ClassPrinter` prints many useful info about bootstrap methods and invoke dynamic instructions, however bootstrap methods arguments are missing. This patch fixes bootstrap methods and invoke dynamic instructions printing. `ClassPrinterTest` is extended to print bootstrap method and invoke dynamic instruction. Please review. Thanks, Adam - Commit messages: - added test - 8329955: Class-File API ClassPrinter does not print bootstrap methods arguments Changes: https://git.openjdk.org/jdk/pull/18695/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18695&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329955 Stats: 335 lines in 2 files changed: 71 ins; 0 del; 264 mod Patch: https://git.openjdk.org/jdk/pull/18695.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18695/head:pull/18695 PR: https://git.openjdk.org/jdk/pull/18695
Re: RFR: 8328481: Implement Module Imports [v4]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Adding more tests for ambiguities. - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/c44031ed..0ca05b7d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18614&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18614&range=02-03 Stats: 69 lines in 1 file changed: 68 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases
This patch suggests a workaround to an issue with huge SCF MH expression trees taking excessive JIT compilation resources by reviving (part of) the simple bytecode-generating strategy that was originally available as an all-or-nothing strategy choice. Instead of reintroducing a binary strategy choice I propose a threshold parameter, controlled by `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions below or at this threshold there's no change, for expressions with an arity above it we use the straightforward one-class per expression. There are a few trade-offs at play here which influence the choice of threshold. The simple high arity strategy will for example not see any reuse of LambdaForms, which means we might end up with a higher number of classes generated and loaded in applications if we set this value too low. It may also produce worse performance on average. On the other hand there is the observed increase in C2 resource usage as expressions grow unwieldy. On the other other hand high arity expressions are likely rare to begin with, with less opportunities for sharing than the more common low-arity expressions. I turned the submitted test case into a few JMH benchmarks and did some experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: Baseline strategy: 13 args: 6.3M 23 args: 18M 123 args: 868M `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: 13 args: 2.11M 23 args: 3.67M 123 args: 4.75M For 123 args the memory overhead of the baseline strategy is 180x, but for 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13 args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) I've conservatively chosen a threshold at arity 20. This keeps C2 resource pressure at a reasonable level (< 18M) while avoiding perturbing performance at the vast majority of call sites. I was asked to use the new class file API for mainline. There's a version of this patch implemented using ASM in 7c52a9f which might be a reasonable basis for a backport. - Commit messages: - Bump threshold after experiments - Port ASM to CFA - Lower compilation overhead for large concat expressions, initial ASM-based version based on pre-existing implementation Changes: https://git.openjdk.org/jdk/pull/18690/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18690&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327247 Stats: 213 lines in 2 files changed: 210 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18690/head:pull/18690 PR: https://git.openjdk.org/jdk/pull/18690
RFR: 8329948: Remove string template feature
This PR removes support for the string template feature from the Java compiler and the Java SE API, as discussed here: https://mail.openjdk.org/pipermail/amber-spec-experts/2024-April/004106.html - Commit messages: - Drop spurious changes - Merge branch 'master' into template_removal - Drop string templates Changes: https://git.openjdk.org/jdk/pull/18688/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18688&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329948 Stats: 7402 lines in 67 files changed: 135 ins; 7208 del; 59 mod Patch: https://git.openjdk.org/jdk/pull/18688.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18688/head:pull/18688 PR: https://git.openjdk.org/jdk/pull/18688
Re: RFR: 8328481: Implement Module Imports [v3]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits: - Merge branch 'master' into module-imports - Merge branch 'master' into module-imports - Merge branch 'master' into module-imports - Fixing test. - Fixing disambiguation of module imports. - Fixing file name computation. - Merge branch 'master' into module-imports - Cleanup. - Fixing CheckExamples. - Adding support for import modules to JShell. - ... and 7 more: https://git.openjdk.org/jdk/compare/b9331cd2...c44031ed - Changes: https://git.openjdk.org/jdk/pull/18614/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18614&range=02 Stats: 1065 lines in 28 files changed: 989 ins; 11 del; 65 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Integrated: 8325485: IncrementInstructions.of(int, int) is not storing the args
On Thu, 8 Feb 2024 13:18:41 GMT, Adam Sotona wrote: > ClassFile API provides two sets of instructions implementations (bound and > unbound). > Unbound implementation of `IncrementInstruction::constant` returns invalid > value. > This bug discovered a hole in the ClassFile API test coverage. > > This patch provides very simple fix of `IncrementInstruction` > and more complex fix of the test framework to cover all unbound instruction > with automated tests. > > The test framework fix includes correction of hash calculation of > instructions in `ClassRecord` and > two-pass transformation in `RebuildingTransformation`. Second pass has been > added to discover bugs in unbound-to-unbound instruction conversions. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: 8907eda7 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/8907eda779f0c3f870bb31deb74c0a483251f1e2 Stats: 786 lines in 4 files changed: 391 ins; 379 del; 16 mod 8325485: IncrementInstructions.of(int, int) is not storing the args Reviewed-by: liach, jlahoda - PR: https://git.openjdk.org/jdk/pull/17770
Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams [v2]
On Tue, 9 Apr 2024 10:04:44 GMT, Viktor Klang wrote:
>> This PR implements Gatherer-inspired encoding of `flatMap` that shows that
>> it is both competitive performance-wise as well as improve correctness.
>>
>> Below is the performance of `Stream::flatMap` (for reference types):
>>
>> Before this PR:
>>
>>
>> Benchmark(size) Mode CntScore Error Units
>> FlatMap.par_array10 thrpt 12 294008,937 ? 54369,110 ops/s
>> FlatMap.par_array 100 thrpt 1262411,229 ? 14868,119 ops/s
>> FlatMap.par_array 1000 thrpt 12 8263,821 ? 452,622 ops/s
>> FlatMap.par_iterate 10 thrpt 1223029,978 ? 4274,449 ops/s
>> FlatMap.par_iterate 100 thrpt 1210532,907 ? 321,694 ops/s
>> FlatMap.par_iterate1000 thrpt 12 981,571 ? 135,270 ops/s
>> FlatMap.seq_array10 thrpt 12 2955648,495 ? 32539,142 ops/s
>> FlatMap.seq_array 100 thrpt 1241851,009 ? 377,546 ops/s
>> FlatMap.seq_array 1000 thrpt 12 1740,281 ? 1229,974 ops/s
>> FlatMap.seq_iterate 10 thrpt 12 321727,690 ? 5149,356 ops/s
>> FlatMap.seq_iterate 100 thrpt 12 8437,198 ?56,635 ops/s
>> FlatMap.seq_iterate1000 thrpt 12 76,994 ? 0,965 ops/s
>>
>>
>> After this PR:
>>
>>
>> Benchmark(size) Mode CntScoreError Units
>> FlatMap.par_array10 thrpt 12 283350,051 ? 35567,223 ops/s
>> FlatMap.par_array 100 thrpt 1253846,906 ? 19241,913 ops/s
>> FlatMap.par_array 1000 thrpt 12 8230,909 ?156,362 ops/s
>> FlatMap.par_iterate 10 thrpt 1226328,500 ? 5411,401 ops/s
>> FlatMap.par_iterate 100 thrpt 1210470,862 ?249,991 ops/s
>> FlatMap.par_iterate1000 thrpt 12 986,511 ?224,050 ops/s
>> FlatMap.seq_array10 thrpt 12 5654826,565 ? 27317,453 ops/s
>> FlatMap.seq_array 100 thrpt 12 187929,786 ?542,787 ops/s
>> FlatMap.seq_array 1000 thrpt 12 2385,346 ? 9,827 ops/s
>> FlatMap.seq_iterate 10 thrpt 12 812722,403 ? 160500,399 ops/s
>> FlatMap.seq_iterate 100 thrpt 1213542,472 ?118,769 ops/s
>> FlatMap.seq_iterate1000 thrpt 12 157,056 ? 1,814 ops/s
>
> Viktor Klang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updating copyright year
src/java.base/share/classes/java/util/stream/DoublePipeline.java line 268:
> 266: class FlatMap implements Sink.OfDouble, DoublePredicate {
> 267: boolean cancel;
> 268: private final boolean shorts =
> isShortCircuitingPipeline();
If there's a known, short-circuiting, operation in the pipeline, we have to use
`allMatch` but in the case where we know that there's nothing which could
short-circuit we can go down a fast-path to `forEach`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1557369039
Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams [v2]
> This PR implements Gatherer-inspired encoding of `flatMap` that shows that it > is both competitive performance-wise as well as improve correctness. > > Below is the performance of `Stream::flatMap` (for reference types): > > Before this PR: > > > Benchmark(size) Mode CntScore Error Units > FlatMap.par_array10 thrpt 12 294008,937 ? 54369,110 ops/s > FlatMap.par_array 100 thrpt 1262411,229 ? 14868,119 ops/s > FlatMap.par_array 1000 thrpt 12 8263,821 ? 452,622 ops/s > FlatMap.par_iterate 10 thrpt 1223029,978 ? 4274,449 ops/s > FlatMap.par_iterate 100 thrpt 1210532,907 ? 321,694 ops/s > FlatMap.par_iterate1000 thrpt 12 981,571 ? 135,270 ops/s > FlatMap.seq_array10 thrpt 12 2955648,495 ? 32539,142 ops/s > FlatMap.seq_array 100 thrpt 1241851,009 ? 377,546 ops/s > FlatMap.seq_array 1000 thrpt 12 1740,281 ? 1229,974 ops/s > FlatMap.seq_iterate 10 thrpt 12 321727,690 ? 5149,356 ops/s > FlatMap.seq_iterate 100 thrpt 12 8437,198 ?56,635 ops/s > FlatMap.seq_iterate1000 thrpt 12 76,994 ? 0,965 ops/s > > > After this PR: > > > Benchmark(size) Mode CntScoreError Units > FlatMap.par_array10 thrpt 12 283350,051 ? 35567,223 ops/s > FlatMap.par_array 100 thrpt 1253846,906 ? 19241,913 ops/s > FlatMap.par_array 1000 thrpt 12 8230,909 ?156,362 ops/s > FlatMap.par_iterate 10 thrpt 1226328,500 ? 5411,401 ops/s > FlatMap.par_iterate 100 thrpt 1210470,862 ?249,991 ops/s > FlatMap.par_iterate1000 thrpt 12 986,511 ?224,050 ops/s > FlatMap.seq_array10 thrpt 12 5654826,565 ? 27317,453 ops/s > FlatMap.seq_array 100 thrpt 12 187929,786 ?542,787 ops/s > FlatMap.seq_array 1000 thrpt 12 2385,346 ? 9,827 ops/s > FlatMap.seq_iterate 10 thrpt 12 812722,403 ? 160500,399 ops/s > FlatMap.seq_iterate 100 thrpt 1213542,472 ?118,769 ops/s > FlatMap.seq_iterate1000 thrpt 12 157,056 ? 1,814 ops/s Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Updating copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/18625/files - new: https://git.openjdk.org/jdk/pull/18625/files/e31c764f..3ff40739 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18625&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18625&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18625.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18625/head:pull/18625 PR: https://git.openjdk.org/jdk/pull/18625
Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams
On Thu, 4 Apr 2024 12:18:07 GMT, Viktor Klang wrote: > This PR implements Gatherer-inspired encoding of `flatMap` that shows that it > is both competitive performance-wise as well as improve correctness. > > Below is the performance of `Stream::flatMap` (for reference types): > > Before this PR: > > > Benchmark(size) Mode CntScore Error Units > FlatMap.par_array10 thrpt 12 294008,937 ? 54369,110 ops/s > FlatMap.par_array 100 thrpt 1262411,229 ? 14868,119 ops/s > FlatMap.par_array 1000 thrpt 12 8263,821 ? 452,622 ops/s > FlatMap.par_iterate 10 thrpt 1223029,978 ? 4274,449 ops/s > FlatMap.par_iterate 100 thrpt 1210532,907 ? 321,694 ops/s > FlatMap.par_iterate1000 thrpt 12 981,571 ? 135,270 ops/s > FlatMap.seq_array10 thrpt 12 2955648,495 ? 32539,142 ops/s > FlatMap.seq_array 100 thrpt 1241851,009 ? 377,546 ops/s > FlatMap.seq_array 1000 thrpt 12 1740,281 ? 1229,974 ops/s > FlatMap.seq_iterate 10 thrpt 12 321727,690 ? 5149,356 ops/s > FlatMap.seq_iterate 100 thrpt 12 8437,198 ?56,635 ops/s > FlatMap.seq_iterate1000 thrpt 12 76,994 ? 0,965 ops/s > > > After this PR: > > > Benchmark(size) Mode CntScoreError Units > FlatMap.par_array10 thrpt 12 283350,051 ? 35567,223 ops/s > FlatMap.par_array 100 thrpt 1253846,906 ? 19241,913 ops/s > FlatMap.par_array 1000 thrpt 12 8230,909 ?156,362 ops/s > FlatMap.par_iterate 10 thrpt 1226328,500 ? 5411,401 ops/s > FlatMap.par_iterate 100 thrpt 1210470,862 ?249,991 ops/s > FlatMap.par_iterate1000 thrpt 12 986,511 ?224,050 ops/s > FlatMap.seq_array10 thrpt 12 5654826,565 ? 27317,453 ops/s > FlatMap.seq_array 100 thrpt 12 187929,786 ?542,787 ops/s > FlatMap.seq_array 1000 thrpt 12 2385,346 ? 9,827 ops/s > FlatMap.seq_iterate 10 thrpt 12 812722,403 ? 160500,399 ops/s > FlatMap.seq_iterate 100 thrpt 1213542,472 ?118,769 ops/s > FlatMap.seq_iterate1000 thrpt 12 157,056 ? 1,814 ops/s @AlanBateman @PaulSandoz Moving this to a candidate PR, see the updated description for performance numbers, the sequential improvements are very encouraging, as well as the improvements which addresses https://bugs.openjdk.org/browse/JDK-8196106 - PR Comment: https://git.openjdk.org/jdk/pull/18625#issuecomment-2044624899
Re: RFR: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v17]
On Mon, 8 Apr 2024 19:45:14 GMT, Martin Doerr wrote:
>> Suchismith Roy has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> test change
>
> test/jdk/java/lang/RuntimeTests/loadLibrary/aix/LoadAIXLibraryFromArchiveObject.java
> line 36:
>
>> 34: String libraryName = "awt";
>> 35: File awtSharedObjectPath = new File("/test/lib/libawt.so");
>> 36: File awtArchivePath = new File("/test/lib/libawt.a");
>
> How does this work? Did you create a "/test" directory? I don't have it on my
> machine.
I am not 100% sure. I think it is a temporary directory under jtreg. I referred
to the other test under loadLibrary. @mlchung Could you clarify this ?
> test/jdk/java/lang/RuntimeTests/loadLibrary/aix/LoadAIXLibraryFromArchiveObject.java
> line 37:
>
>> 35: File awtSharedObjectPath = new File("/test/lib/libawt.so");
>> 36: File awtArchivePath = new File("/test/lib/libawt.a");
>> 37: awtSharedObjectPath.renameTo(awtArchivePath);
>
> This should work for this test. But, what if an AWT test gets executed after
> your test? I think copy is safer than renaming.
Maybe i need to rename it back. If we just copy, it would take the .so itself,
so we cant enforce it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1557366150
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1557365063
Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams
On Mon, 8 Apr 2024 16:47:24 GMT, Paul Sandoz wrote: > > @PaulSandoz @AlanBateman I've added a commit to this PR which removes the > > use of Gatherer for Stream::flatMap, but instead implements flatMap for all > > of the pipelines using the same encoding which Gatherer would use. It seems > > very competitive performance-wise, and resolves at least one open JBS-issue > > with flatMap (will look to see if it resolves more than that) > > That's a rather clever use of `allMatch`! Thank you :) > > Did you observe performance improvements using `@Stable` on the `cancel` > field? It's really hard to predict in the abstract (since the default value > of the field will be read in proportion to the number of elements until the > stream is cancelled). I'm currently exploring some different venues for optimization. Stay tuned :) - PR Comment: https://git.openjdk.org/jdk/pull/18625#issuecomment-2043559783
Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams
On Mon, 8 Apr 2024 10:20:11 GMT, Viktor Klang wrote: > @PaulSandoz @AlanBateman I've added a commit to this PR which removes the use > of Gatherer for Stream::flatMap, but instead implements flatMap for all of > the pipelines using the same encoding which Gatherer would use. It seems very > competitive performance-wise, and resolves at least one open JBS-issue with > flatMap (will look to see if it resolves more than that) That's a rather clever use of `allMatch`! Did you observe performance improvements using `@Stable` on the `cancel` field? It's really hard to predict in the abstract (since the default value of the field will be read in proportion to the number of elements until the stream is cancelled). - PR Comment: https://git.openjdk.org/jdk/pull/18625#issuecomment-2043218633
Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams
On Thu, 4 Apr 2024 12:18:07 GMT, Viktor Klang wrote: > This PR implements Gatherer-inspired encoding of `flatMap` that shows that it > is both competitive performance-wise as well as improve correctness. > > Below is the performance of `Stream::flatMap` (for reference types): > > Before this PR: > > > Benchmark(size) Mode CntScore Error Units > FlatMap.par_array10 thrpt 12 294008,937 ? 54369,110 ops/s > FlatMap.par_array 100 thrpt 1262411,229 ? 14868,119 ops/s > FlatMap.par_array 1000 thrpt 12 8263,821 ? 452,622 ops/s > FlatMap.par_iterate 10 thrpt 1223029,978 ? 4274,449 ops/s > FlatMap.par_iterate 100 thrpt 1210532,907 ? 321,694 ops/s > FlatMap.par_iterate1000 thrpt 12 981,571 ? 135,270 ops/s > FlatMap.seq_array10 thrpt 12 2955648,495 ? 32539,142 ops/s > FlatMap.seq_array 100 thrpt 1241851,009 ? 377,546 ops/s > FlatMap.seq_array 1000 thrpt 12 1740,281 ? 1229,974 ops/s > FlatMap.seq_iterate 10 thrpt 12 321727,690 ? 5149,356 ops/s > FlatMap.seq_iterate 100 thrpt 12 8437,198 ?56,635 ops/s > FlatMap.seq_iterate1000 thrpt 12 76,994 ? 0,965 ops/s > > > After this PR: > > > Benchmark(size) Mode CntScoreError Units > FlatMap.par_array10 thrpt 12 283350,051 ? 35567,223 ops/s > FlatMap.par_array 100 thrpt 1253846,906 ? 19241,913 ops/s > FlatMap.par_array 1000 thrpt 12 8230,909 ?156,362 ops/s > FlatMap.par_iterate 10 thrpt 1226328,500 ? 5411,401 ops/s > FlatMap.par_iterate 100 thrpt 1210470,862 ?249,991 ops/s > FlatMap.par_iterate1000 thrpt 12 986,511 ?224,050 ops/s > FlatMap.seq_array10 thrpt 12 5654826,565 ? 27317,453 ops/s > FlatMap.seq_array 100 thrpt 12 187929,786 ?542,787 ops/s > FlatMap.seq_array 1000 thrpt 12 2385,346 ? 9,827 ops/s > FlatMap.seq_iterate 10 thrpt 12 812722,403 ? 160500,399 ops/s > FlatMap.seq_iterate 100 thrpt 1213542,472 ?118,769 ops/s > FlatMap.seq_iterate1000 thrpt 12 157,056 ? 1,814 ops/s @PaulSandoz @AlanBateman I've added a commit to this PR which removes the use of Gatherer for Stream::flatMap, but instead implements flatMap for all of the pipelines using the same encoding which Gatherer would use. It seems very competitive performance-wise, and resolves at least one open JBS-issue with flatMap (will look to see if it resolves more than that) src/java.base/share/classes/java/util/stream/AbstractPipeline.java line 436: > 434: } > 435: > 436: /** Note to self, update Copyright year to 2024 src/java.base/share/classes/java/util/stream/GathererOp.java line 30: > 28: > 29: import java.lang.invoke.MethodHandles; > 30: import java.lang.invoke.VarHandle; Note to self, update copyright year - PR Comment: https://git.openjdk.org/jdk/pull/18625#issuecomment-2042381405 PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1557358718 PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1557359258
RFR: 8196106: Support nested infinite or recursive flat mapped streams
This PR implements Gatherer-inspired encoding of `flatMap` that shows that it is both competitive performance-wise as well as improve correctness. Below is the performance of `Stream::flatMap` (for reference types): Before this PR: Benchmark(size) Mode CntScore Error Units FlatMap.par_array10 thrpt 12 294008,937 ? 54369,110 ops/s FlatMap.par_array 100 thrpt 1262411,229 ? 14868,119 ops/s FlatMap.par_array 1000 thrpt 12 8263,821 ? 452,622 ops/s FlatMap.par_iterate 10 thrpt 1223029,978 ? 4274,449 ops/s FlatMap.par_iterate 100 thrpt 1210532,907 ? 321,694 ops/s FlatMap.par_iterate1000 thrpt 12 981,571 ? 135,270 ops/s FlatMap.seq_array10 thrpt 12 2955648,495 ? 32539,142 ops/s FlatMap.seq_array 100 thrpt 1241851,009 ? 377,546 ops/s FlatMap.seq_array 1000 thrpt 12 1740,281 ? 1229,974 ops/s FlatMap.seq_iterate 10 thrpt 12 321727,690 ? 5149,356 ops/s FlatMap.seq_iterate 100 thrpt 12 8437,198 ?56,635 ops/s FlatMap.seq_iterate1000 thrpt 12 76,994 ? 0,965 ops/s After this PR: Benchmark(size) Mode CntScoreError Units FlatMap.par_array10 thrpt 12 283350,051 ? 35567,223 ops/s FlatMap.par_array 100 thrpt 1253846,906 ? 19241,913 ops/s FlatMap.par_array 1000 thrpt 12 8230,909 ?156,362 ops/s FlatMap.par_iterate 10 thrpt 1226328,500 ? 5411,401 ops/s FlatMap.par_iterate 100 thrpt 1210470,862 ?249,991 ops/s FlatMap.par_iterate1000 thrpt 12 986,511 ?224,050 ops/s FlatMap.seq_array10 thrpt 12 5654826,565 ? 27317,453 ops/s FlatMap.seq_array 100 thrpt 12 187929,786 ?542,787 ops/s FlatMap.seq_array 1000 thrpt 12 2385,346 ? 9,827 ops/s FlatMap.seq_iterate 10 thrpt 12 812722,403 ? 160500,399 ops/s FlatMap.seq_iterate 100 thrpt 1213542,472 ?118,769 ops/s FlatMap.seq_iterate1000 thrpt 12 157,056 ? 1,814 ops/s - Commit messages: - Adding JavaDoc to isShortCircuitingPipeline() - Removing the use of @Stable - Adding conditional logic to optimize for non-shortcircuiting flatmaps - Converting the Int/Long/DoublePipelines - Switching flatMap implementations to ones inspired by Gatherer encoding Changes: https://git.openjdk.org/jdk/pull/18625/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18625&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8196106 Stats: 409 lines in 8 files changed: 236 ins; 84 del; 89 mod Patch: https://git.openjdk.org/jdk/pull/18625.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18625/head:pull/18625 PR: https://git.openjdk.org/jdk/pull/18625
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Wed, 3 Apr 2024 22:31:39 GMT, Mandy Chung wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Move CreateLinkableRuntimePlugin to build folder >> >> Keep runtime link supporting classes in package >> jdk.tools.jlink.internal.runtimelink > > Thanks for the investigation w.r.t. extending jimage. It does not seem worth > the effort in pursuing the support of adding resources to an existing jimage > file. To me, putting the diff data under `lib` directory as a private file > is a simpler and acceptable solution. It allows the second jlink invocation > to avoid the plugin pipeline if the per-module metadata is generated in > normal jlink invocation. > > (An observation: The current implementation requires the diffs to be > generated as a plugin. For this approach, it may be possible to implement > with a separate main class that calls JLink API and generates the diffs.) @mlchung @AlanBateman Any thoughts on this latest version? Is this going into the direction you had envisioned? Any more blockers? The CSR should be up-to-date and is open for review as well. If no more blockers I'll go over this patch once more and clean it up to prepare for integration. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2044593102
Integrated: 8329527: Opcode.IFNONNULL.primaryTypeKind() is not ReferenceType
On Wed, 3 Apr 2024 08:15:52 GMT, Adam Sotona wrote: > `Opcode.IFNONNULL.primaryTypeKind()` wrongly returned `IntType` instead of > the right `ReferenceType`. > Primary type kinds of `BRANCH` opcodes have yet no functional effect in the > Class-File API. > This simple patch fixes the typo. > > Please review. > > Thank you, > Adam This pull request has now been integrated. Changeset: 71c5bbce Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/71c5bbcec7052a8394dd49c0a8c46801adbfcae4 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8329527: Opcode.IFNONNULL.primaryTypeKind() is not ReferenceType Reviewed-by: jlahoda - PR: https://git.openjdk.org/jdk/pull/18593
Re: RFR: 8329099: Undocumented exception thrown by Instruction factory methods accepting Opcode [v4]
> `IllegalArgumentException` thrown by some static factory methods of the > following `java.lang.classfile.instruction` interfaces are not documented: > > - `ArrayLoadInstruction` > - `ArrayStoreInstruction` > - `BranchInstruction` > - `ConvertInstruction` > - `DiscontinuedInstruction` > - `FieldInstruction` > - `InvokeInstruction` > - `LoadInstruction` > - `MonitorInstruction` > - `NewPrimitiveArrayInstruction` > - `OperatorInstruction` > - `ReturnInstruction` > - `StackInstruction` > - `StoreInstruction` > - `TypeCheckInstruction` > > `NewPrimitiveArrayInstruction::of` factory method also does not perform the > check for invalid argument. > > This patch adds all the missing `@throws` Javadoc tags and fixes > `NewPrimitiveArrayInstruction::of` factory method to perform the argument > check. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added NPE statement to package-info - Changes: - all: https://git.openjdk.org/jdk/pull/18578/files - new: https://git.openjdk.org/jdk/pull/18578/files/71165f1e..7af3f7c5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18578&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18578&range=02-03 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18578.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18578/head:pull/18578 PR: https://git.openjdk.org/jdk/pull/18578
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v9]
On Mon, 8 Apr 2024 16:36:49 GMT, Vicente Romero wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding tests as suggested. > > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java > line 101: > >> 99: import com.sun.tools.javac.tree.JCTree.JCPattern; >> 100: import com.sun.tools.javac.tree.JCTree.JCPatternCaseLabel; >> 101: import com.sun.tools.javac.tree.JCTree.JCDerivedInstance; > > changes to this file can be removed now Thanks, removed: https://github.com/openjdk/jdk/pull/18509/commits/a05480cd1a436014ba00505db71d54d82a37ecd1 > test/langtools/tools/javac/patterns/withers/Model.java line 2: > >> 1: /* >> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. > > probably this test should be in another location not under `patterns`, same > for other tests added inside `patterns` folder Thanks, moved: https://github.com/openjdk/jdk/pull/18509/commits/a05480cd1a436014ba00505db71d54d82a37ecd1 - PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1557304771 PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1557304921
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v10]
> This is a patch for javac, that adds the Derived Record Creation expressions. > The current draft specification for the feature is: > https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html > > The current CSR is here: > https://bugs.openjdk.org/browse/JDK-8328637 > > The patch is mostly straightforward, with two notable changes: > - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the > specification introduces this term, and it seems consistent with > `ElementKind.BINDING_VARIABLE` that was introduced some time ago. > - there are a bit broader changes in `Flow`, to facilitate the introduction > of variables without an explicit declaration for definite assignment and > effectively final computation. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback: - reverting unnecessary changes in TransPatterns - moving the patters/withers/Model test to a more appropriate place - Changes: - all: https://git.openjdk.org/jdk/pull/18509/files - new: https://git.openjdk.org/jdk/pull/18509/files/e0930688..a05480cd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18509&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18509&range=08-09 Stats: 37 lines in 4 files changed: 14 ins; 17 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18509.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509 PR: https://git.openjdk.org/jdk/pull/18509
Re: RFR: 8325485: IncrementInstructions.of(int, int) is not storing the args [v2]
On Fri, 9 Feb 2024 10:47:08 GMT, Adam Sotona wrote: >> ClassFile API provides two sets of instructions implementations (bound and >> unbound). >> Unbound implementation of `IncrementInstruction::constant` returns invalid >> value. >> This bug discovered a hole in the ClassFile API test coverage. >> >> This patch provides very simple fix of `IncrementInstruction` >> and more complex fix of the test framework to cover all unbound instruction >> with automated tests. >> >> The test framework fix includes correction of hash calculation of >> instructions in `ClassRecord` and >> two-pass transformation in `RebuildingTransformation`. Second pass has been >> added to discover bugs in unbound-to-unbound instruction conversions. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/jdk/jdk/classfile/helpers/RebuildingTransformation.java > > Co-authored-by: Andrey Turbanov Looks reasonable. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17770#pullrequestreview-1988500946
Re: RFR: 8329527: Opcode.IFNONNULL.primaryTypeKind() is not ReferenceType
On Wed, 3 Apr 2024 08:15:52 GMT, Adam Sotona wrote: > `Opcode.IFNONNULL.primaryTypeKind()` wrongly returned `IntType` instead of > the right `ReferenceType`. > Primary type kinds of `BRANCH` opcodes have yet no functional effect in the > Class-File API. > This simple patch fixes the typo. > > Please review. > > Thank you, > Adam Looks good. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18593#pullrequestreview-1988500479
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v8]
On Fri, 5 Apr 2024 18:30:30 GMT, Joe Darcy wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflecting review feedback:
>> - pre-generating the JCVarDecls in Attr, to aid Flow
>> - adding a note on how the desugared code looks like
>
> src/java.compiler/share/classes/javax/lang/model/element/ElementKind.java
> line 135:
>
>> 133: COMPONENT_LOCAL_VARIABLE;
>> 134:
>> 135: // Maintenance note: check if the default implementation of
>
> From a quick look, I don't think Elements.getOutermostTypeElement needs
> updating for COMPONENT_LOCAL_VARIABLE, but it would be good to add a test
> case.
Tests added:
https://github.com/openjdk/jdk/pull/18509/commits/e09306885e5c6cd2aba7779025fc7efcb8991e8e
Thanks!
> src/java.compiler/share/classes/javax/lang/model/util/ElementKindVisitorPreview.java
> line 91:
>
>> 89:
>> 90: /**
>> 91: * {@inheritDoc ElementKindVisitor6}
>
> If possible, would be good to add some minimal testing/use of the element
> kind visitors on component local variables.
Tests added:
https://github.com/openjdk/jdk/pull/18509/commits/e09306885e5c6cd2aba7779025fc7efcb8991e8e
Thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1557243414
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1557243525
Integrated: 8326744: Class-File API transition to Second Preview
On Tue, 27 Feb 2024 08:45:12 GMT, Adam Sotona wrote: > Task providing Class-File API transition to Second Preview. This pull request has now been integrated. Changeset: 19a99d02 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/19a99d023e32fa9f4d26b76bd36993719e1dfe21 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8326744: Class-File API transition to Second Preview Reviewed-by: jlahoda - PR: https://git.openjdk.org/jdk/pull/18021
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
On Tue, 9 Apr 2024 01:34:56 GMT, Naoto Sato wrote:
>> Hello Roger,
>>
>>> The code should reference the constants in Instant.java (though may need to
>>> make them package private.)
>>
>> The `ChronoField` class and the `Instant` class reside in separate packages,
>> so making these two fields in `Instant`, package private will not help. I
>> will have to make them public, which I think probably isn't a good idea.
>> Unless you think we should do it? There is one other place already in the
>> JDK where we have copy/pasted these values
>> `src/java.base/share/classes/java/nio/file/attribute/FileTime.java`, so
>> maybe we can continue with this copy/paste here too?
>>
>> As for the javadoc, after we decide about this field access detail, I'll
>> update it accordingly to note that the values min and max range complies
>> with the min and max epoch seconds supported by `Instant`.
>
> Should `INSTANT_SECONDS("InstantSeconds", SECONDS, FOREVER,
> ValueRange.of(Instant.MIN.getEpochSecond(), Instant.MAX.getEpochSecond())),
> ` work?
Hello Naoto, that's a very good point. I was too caught up with the constant
values that it didn't occur to me that the `Instant.MIN` and `Instant.MAX`
public fields could be used for this. I have followed your suggestion and
updated the PR. I have also updated the javadoc to link to `Instant.MIN` and
`Instant.MAX` as the supported epoch second range.
The test continues to pass with this change and fails (as expected) without the
source change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1557216492
Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]
> Can I please get a review of this change which proposes to fix the issue > noted in https://bugs.openjdk.org/browse/JDK-8212895? > > As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is > initialized to have a minimum and maximum values of `Long.MIN_VALUE` and > `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports > `-31557014167219200L` and `31556889864403199L` as minimum and maximum values > for the epoch second. > > The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value range > to match the supported min and max values of `Instant` (as suggested by > Stephen in that JBS issue). This commit also introduces a test to verify this > change. This new test method as well as existing tests in tier1, tier2 and > tier3 continue to pass with this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded values - Changes: - all: https://git.openjdk.org/jdk/pull/18674/files - new: https://git.openjdk.org/jdk/pull/18674/files/1fea0f71..6e535779 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18674&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18674&range=00-01 Stats: 5 lines in 1 file changed: 3 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18674.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18674/head:pull/18674 PR: https://git.openjdk.org/jdk/pull/18674
Re: RFR: 8329729: java/util/Properties/StoreReproducibilityTest.java times out [v2]
On Tue, 9 Apr 2024 01:27:29 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which proposes to address >> the intermittent failures in >> `java/util/Properties/StoreReproducibilityTest.java`? This should address >> https://bugs.openjdk.org/browse/JDK-8329729. >> >> These failures in `StoreReproducibilityTest` have been observed in higher >> tiers where the test tasks are launched with various JVM options, one of >> them being `-Xcomp`. The goal of the `StoreReproducibilityTest` is to verify >> that the content which the `java.util.Properties` code generates for a >> properties file and reproducible across multiple different runs/launches of >> an Java application. To do that it launches a test application (using `java` >> command) several times within the test (for different scenarios). That comes >> up to a combined total of 25 launches, for different scenarios. Normally >> each such launch of the `java` application completes within a second or two. >> >> Recently, we have been updating our tests to pass along the JVM options that >> were used for launching the test task, to the child processes that are >> launched from within the tests. That now means that these trivial small java >> application that this test launches several times will now be passed the >> `-Xcomp` option too (when the test task is launched with that option). It >> has been observed that when `-Xcomp` is used to launch those trivial >> applications from within the test, each such application takes around 30 >> seconds to a minute to complete. This then causes the test to timeout. >> >> Given the context of this test case, it's not necessary to run this test >> when `-Xcomp` is used. The commit in this PR add a `@requires` to disable >> this test when `-Xcomp` is present in the test task's JVM args. >> >> I've run this change in our CI and the test continues to run (and pass) when >> `-Xcomp` is absent and is skipped when it is present. > > Jaikiran Pai 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 two additional > commits since the last revision: > > - merge latest from master branch > - 8329729: java/util/Properties/StoreReproducibilityTest.java times out Hello Alan, thank you for the review. I've shortened the test's comment as per your suggestion. - PR Comment: https://git.openjdk.org/jdk/pull/18681#issuecomment-2044409962
Re: RFR: 8329729: java/util/Properties/StoreReproducibilityTest.java times out [v3]
> Can I please get a review of this test-only change which proposes to address > the intermittent failures in > `java/util/Properties/StoreReproducibilityTest.java`? This should address > https://bugs.openjdk.org/browse/JDK-8329729. > > These failures in `StoreReproducibilityTest` have been observed in higher > tiers where the test tasks are launched with various JVM options, one of them > being `-Xcomp`. The goal of the `StoreReproducibilityTest` is to verify that > the content which the `java.util.Properties` code generates for a properties > file and reproducible across multiple different runs/launches of an Java > application. To do that it launches a test application (using `java` command) > several times within the test (for different scenarios). That comes up to a > combined total of 25 launches, for different scenarios. Normally each such > launch of the `java` application completes within a second or two. > > Recently, we have been updating our tests to pass along the JVM options that > were used for launching the test task, to the child processes that are > launched from within the tests. That now means that these trivial small java > application that this test launches several times will now be passed the > `-Xcomp` option too (when the test task is launched with that option). It has > been observed that when `-Xcomp` is used to launch those trivial applications > from within the test, each such application takes around 30 seconds to a > minute to complete. This then causes the test to timeout. > > Given the context of this test case, it's not necessary to run this test when > `-Xcomp` is used. The commit in this PR add a `@requires` to disable this > test when `-Xcomp` is present in the test task's JVM args. > > I've run this change in our CI and the test continues to run (and pass) when > `-Xcomp` is absent and is skipped when it is present. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Alan's suggestion - shorten the test comment - Changes: - all: https://git.openjdk.org/jdk/pull/18681/files - new: https://git.openjdk.org/jdk/pull/18681/files/8cca30dc..692eb90e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18681&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18681&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18681.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18681/head:pull/18681 PR: https://git.openjdk.org/jdk/pull/18681
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v4]
On Fri, 5 Apr 2024 02:40:16 GMT, Dean Long wrote: > That way C2 can do all its usual optimizations, like unrolling, > vectorization, and redundant store elimination (if it is an on-heap primitive > array that was just allocated, then there is no need to zero the parts that > are being "set"). I second that. It is something that came up quite frequently in the discussions around the FFM API. - PR Comment: https://git.openjdk.org/jdk/pull/18555#issuecomment-2044409509
