Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v8]
> Please review this PR to use modern APIs and language features to simplify > equals, hashCode, and compareTo for BigInteger. If you have any performance > concerns, please raise them. > > This PR is cherry-picked from a bigger, not-yet-published PR, to test the > waters. That latter PR will be published soon. Pavel Rappo 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 15 additional commits since the last revision: - Merge branch 'master' into 8310813 - Merge branch 'master' into 8310813 - Fix bugs in Shared.createSingle - Merge branch 'master' into 8310813 - Group params coarser (suggested by @cl4es) - Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. Every testXYZ method invokes M operations, where M is the maximum number of elements in a group. Shorter groups are cyclically padded. - Uses the org.openjdk.jmh.infra.Blackhole API and increases benchmark time. - Fixes a bug in Shared that precluded 0 from being in a pair. - Use better overloads (suggested by @cl4es) - Uses simpler, more suitable overloads for the subrange starting from 0 - Improve benchmarks - Merge branch 'master' into 8310813 - Restore hash code values BigInteger is old and ubiquitous enough so that there might be inadvertent dependencies on its hash code. This commit also includes a test, to make sure hash code is unchanged. - Merge branch 'master' into 8310813 - ... and 5 more: https://git.openjdk.org/jdk/compare/16651ffc...155fedba - Changes: - all: https://git.openjdk.org/jdk/pull/14630/files - new: https://git.openjdk.org/jdk/pull/14630/files/77bfab34..155fedba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14630=07 - incr: https://webrevs.openjdk.org/?repo=jdk=14630=06-07 Stats: 111587 lines in 3212 files changed: 64533 ins; 24863 del; 22191 mod Patch: https://git.openjdk.org/jdk/pull/14630.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14630/head:pull/14630 PR: https://git.openjdk.org/jdk/pull/14630
Re: RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader [v2]
On Mon, 6 Nov 2023 20:22:43 GMT, Mandy Chung wrote: >> This is a regression caused by JDK-8302791. IAE should be thrown when an >> interface is not visible to the given class loader but NPE is thrown instead >> when the loader is null. The boot loader has no name and so the fix will >> print `null` in the exception message. >> `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to >> junit and updated to test IAE thrown because proxy interface is not visible >> to null loader. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Review feedback The fix to getLoaderNameID looks fine, the update and conversion of the test looks okay too. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16525#pullrequestreview-1717031599
Re: RFR: 8318971: jar v17 should either exit on error immediately or create archive as jar v1.8 did
On Mon, 30 Oct 2023 16:16:52 GMT, Ryan Wallace wrote: > Hi all, > > Please review this fix for jar tool not producing archive if there is a > missing file supplied. Fix is to throw an exception and exit processing when > a missing file is supplied. Current behaviour will recognise missing file as > an error but continue processing and not produce the archive. Updated > ClassPath test to verify jar is not created. > > Thanks, > Ryan. Hello Ryan, I ran a few experiments and I am unsure if we should be doing this change. Please consider this following scenario, when using the current implementation in mainline: echo "hello" > hello.txt jar -cf foo.jar missing.txt missing2.txt hello.txt - PR Comment: https://git.openjdk.org/jdk/pull/16423#issuecomment-1797979185
Re: RFR: 8319338: tools/jpackage/share/RuntimeImageTest.java fails with -XX:+UseZGC
On Tue, 7 Nov 2023 03:20:45 GMT, Alexey Semenyuk wrote: > Remove `-Xmx512m` from the jtreg `@run` command as @AlanBateman suggested Can you confirm that debug builds are passing with -XX:+UseZGC now? - PR Comment: https://git.openjdk.org/jdk/pull/16535#issuecomment-1797889256
Re: RFR: 8305814: Update Xalan Java to 2.7.3 [v2]
> Xalan 2.7.3: merge minor changes from the upstream project. > > Test: existing XML tests pass Joe Wang has updated the pull request incrementally with one additional commit since the last revision: remove commented out block in ExsltDatetime - Changes: - all: https://git.openjdk.org/jdk/pull/16532/files - new: https://git.openjdk.org/jdk/pull/16532/files/06535c4a..0d13a5a5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16532=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16532=00-01 Stats: 23 lines in 1 file changed: 0 ins; 23 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16532.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16532/head:pull/16532 PR: https://git.openjdk.org/jdk/pull/16532
Re: RFR: 8305814: Update Xalan Java to 2.7.3 [v2]
On Tue, 7 Nov 2023 01:43:21 GMT, Lance Andersen wrote:
>> Joe Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove commented out block in ExsltDatetime
>
> src/java.xml/share/classes/com/sun/org/apache/xalan/internal/lib/ExsltDatetime.java
> line 104:
>
>> 102:
>> buff.append(posneg).append(formatDigits(hrs)).append(':').append(formatDigits(min));
>> 103: }
>> 104: return buff.toString();*/
>
> This commented out block is difficult to see given Can we clean this up or
> better yet remove this code if not needed?
Thanks. Removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16532#discussion_r1384406645
Re: RFR: 8319338: tools/jpackage/share/RuntimeImageTest.java fails with -XX:+UseZGC
On Tue, 7 Nov 2023 03:20:45 GMT, Alexey Semenyuk wrote: > Remove `-Xmx512m` from the jtreg `@run` command as @AlanBateman suggested Looks good. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16535#pullrequestreview-1716751895
Re: RFR: 8319338: tools/jpackage/share/RuntimeImageTest.java fails with -XX:+UseZGC
On Tue, 7 Nov 2023 03:20:45 GMT, Alexey Semenyuk wrote: > Remove `-Xmx512m` from the jtreg `@run` command as @AlanBateman suggested @sashamatveev , @AlanBateman please review - PR Comment: https://git.openjdk.org/jdk/pull/16535#issuecomment-1797631432
RFR: 8319338: tools/jpackage/share/RuntimeImageTest.java fails with -XX:+UseZGC
Remove `-Xmx512m` from the jtreg `@run` command as @AlanBateman suggested - Commit messages: - 8319338: tools/jpackage/share/RuntimeImageTest.java fails with -XX:+UseZGC Changes: https://git.openjdk.org/jdk/pull/16535/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16535=00 Issue: https://bugs.openjdk.org/browse/JDK-8319338 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16535.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16535/head:pull/16535 PR: https://git.openjdk.org/jdk/pull/16535
Re: RFR: 8187655: jdk.lambda.vm.InterfaceAccessFlagsTest.testPrivateMethodCall needs update after nestmates support
On Tue, 7 Nov 2023 02:30:55 GMT, Chen Liang wrote:
>> `jdk.lambda.vm.InterfaceAccessFlagsTest` uses `ClassToInterfaceConverter` to
>> mechanically convert a classfile for a Class into an in-memory class
>> representation of an equivalent Interface. `testPrivateMethodCall` tests
>> to invoke a private method. Before nestmates, invoking a private class
>> method and a private interface method both use `Invokespecial`. With the
>> nestmate changes, the class uses `invokevirtual` but the interface must use
>> `invokeinterface` but this conversion is not handled by the existing
>> `ClassToInterfaceConverter`.
>>
>> This fix converts `ClassToInterfaceConverter` to use the Class-File API to
>> properly convert a classfile from a class to an interface including method
>> invocation from `invokevirtual` to `invokeinterface`. The old custom
>> bytecode manipulation code can be dropped.
>
> test/jdk/jdk/lambda/separate/ClassToInterfaceConverter.java line 43:
>
>> 41: for (ClassElement ce : classModel) {
>> 42: if (ce instanceof AccessFlags accessFlags) {
>> 43: classBuilder.withFlags(0x0601); //
>> ACC_INTERFACE | ACC_ABSTRACT | ACC_PUBLIC);
>
> We can just use `withFlags(Classfile.ACC_INTERFACE | Classfile.ACC_ABSTRACT |
> Classfile.ACC_PUBLIC)` as it's a constant expression and inlined by javac.
> And the `accessFlags` variable above appears unused.
I have same thought too. I have that changed in my local repo but not have the
chance to push it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16526#discussion_r1384326189
Re: RFR: 8187655: jdk.lambda.vm.InterfaceAccessFlagsTest.testPrivateMethodCall needs update after nestmates support
On Mon, 6 Nov 2023 19:26:26 GMT, Mandy Chung wrote:
> `jdk.lambda.vm.InterfaceAccessFlagsTest` uses `ClassToInterfaceConverter` to
> mechanically convert a classfile for a Class into an in-memory class
> representation of an equivalent Interface. `testPrivateMethodCall` tests to
> invoke a private method. Before nestmates, invoking a private class method
> and a private interface method both use `Invokespecial`. With the nestmate
> changes, the class uses `invokevirtual` but the interface must use
> `invokeinterface` but this conversion is not handled by the existing
> `ClassToInterfaceConverter`.
>
> This fix converts `ClassToInterfaceConverter` to use the Class-File API to
> properly convert a classfile from a class to an interface including method
> invocation from `invokevirtual` to `invokeinterface`. The old custom
> bytecode manipulation code can be dropped.
test/jdk/jdk/lambda/separate/ClassToInterfaceConverter.java line 39:
> 37:
> 38: private byte[] convertToInterface(ClassModel classModel) {
> 39: return Classfile.of().build(classModel.thisClass().asSymbol(),
You can use `transform` to transform a class instead of `build`.
test/jdk/jdk/lambda/separate/ClassToInterfaceConverter.java line 43:
> 41: for (ClassElement ce : classModel) {
> 42: if (ce instanceof AccessFlags accessFlags) {
> 43: classBuilder.withFlags(0x0601); //
> ACC_INTERFACE | ACC_ABSTRACT | ACC_PUBLIC);
We can just use `withFlags(Classfile.ACC_INTERFACE | Classfile.ACC_ABSTRACT |
Classfile.ACC_PUBLIC)` as it's a constant expression and inlined by javac. And
the `accessFlags` variable above appears unused.
test/jdk/jdk/lambda/separate/ClassToInterfaceConverter.java line 52:
> 50: // by other methods in the interface,
> convert it to InterfaceMethodref and
> 51: // if opcode is invokevirtual, convert it to
> invokeinterface
> 52:
> classBuilder.withMethod(mm.methodName().stringValue(),
Same remark, use `classBuilder.transformMethod(mm,
MethodTransform.transformCode((codeBuilder, e) -> {}))`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16526#discussion_r1384280905
PR Review Comment: https://git.openjdk.org/jdk/pull/16526#discussion_r1384279751
PR Review Comment: https://git.openjdk.org/jdk/pull/16526#discussion_r1384284062
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v2]
On Mon, 6 Nov 2023 22:41:17 GMT, Erik Gahlin wrote:
>> src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 44:
>>
>>> 42:
>>> 43: public static void traceError(Class clazz, String message) {
>>> 44: if (OutOfMemoryError.class.isAssignableFrom(clazz)) {
>>
>> StackOverflowError is likely problematic too, maybe it should be
>> VirtualMachineError.
>
> I remember asking the same question ten years ago, when Nils did the original
> implementation, but I think it was only needed for OOM, because it creates an
> infinite loop when the event object was allocated, which resulted in a
> StackOverflowError instead OOM.
>
> Some OOM tests failed with JFR enabled.
>
> The event object allocation has been removed, but I think we can run into the
> allocation recursion by other means. I looked into it a few years ago, but I
> don't remember exactly why it failed.
>
> If a SOE happens, I think we are fine. There is something that prevents
> infinite recursion when the SOE object is created. Perhaps it is
> preallocated? I prefer to not change the behavior, at least not in this PR.
I filed an issue to investigate if there is a problem with SOE, or if the OOM
check is really needed now.
https://bugs.openjdk.org/browse/JDK-8319579
Regardless of outcome, It would be good to document the results of the
investigation in the code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384263589
Re: RFR: 8305814: Update Xalan Java to 2.7.3
On Mon, 6 Nov 2023 22:52:47 GMT, Joe Wang wrote:
> Xalan 2.7.3: merge minor changes from the upstream project.
>
> Test: existing XML tests pass
src/java.xml/share/classes/com/sun/org/apache/xalan/internal/lib/ExsltDatetime.java
line 104:
> 102:
> buff.append(posneg).append(formatDigits(hrs)).append(':').append(formatDigits(min));
> 103: }
> 104: return buff.toString();*/
This commented out block is difficult to see given Can we clean this up or
better yet remove this code if not needed?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16532#discussion_r1384244649
Re: RFR: 8317742: ISO Standard Date Format implementation consistency on DateTimeFormatter and String.format [v5]
> j.u.Formatter now prints "%tF" (iso standard date) and the result is > incorrect when processing year < 0 or year > Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: use DecimalFormatSymbols#getMinusSign - Changes: - all: https://git.openjdk.org/jdk/pull/16033/files - new: https://git.openjdk.org/jdk/pull/16033/files/b6313fc0..7051ad51 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16033=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16033=03-04 Stats: 8 lines in 2 files changed: 7 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16033.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16033/head:pull/16033 PR: https://git.openjdk.org/jdk/pull/16033
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v3]
On Mon, 6 Nov 2023 18:33:46 GMT, Sandhya Viswanathan wrote: > This is not a masked operation so every lane of dst will be written through > pinsrw/pinsrb. An vpxor before is not required. xor here clears the intermediate vector after each iteration, this is eventually ORs with destination. Checkout line https://github.com/openjdk/jdk/pull/16354/files/86783403c453d329e33d94f787a5709ec35f7099#diff-318d0e76b9a97e8cf8936be1de34e52735c4d947a77cac38babbbf9a081d16fcR1644 https://github.com/openjdk/jdk/pull/16354/files/86783403c453d329e33d94f787a5709ec35f7099#diff-318d0e76b9a97e8cf8936be1de34e52735c4d947a77cac38babbbf9a081d16fcR1651 - PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1384225640
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]
On Mon, 6 Nov 2023 23:58:11 GMT, Sergey Bylokhov wrote: > > So we have somewhere around a fixed 125ms startup cost for the FFM case - > > as measured on my Mac, > > but only 35-40ms of that is attributable to the specific needs of layout. > > That looks unfortunate. I guess if we will start to use ffm in other places > we can easily spend of 1 second budget on startup=( Yes, this case is sufficiently uncommon, that it is OK, and is a decent way to help us track improvements to FFM. But it would be another matter to have to do it for however many of our core software loops and AWT window manager interaction calls we need to get running for a minimal app. > > > layoutCnt=16000 total=193ms <<< app fully displayed > > vs > > layoutCnt=16000 total=453ms <<< app fully displayed > > It looks strange that 16000 calls are not enough to warmup, and the > difference is so large. I am not a C2 expert, (not even an amateur), I just assume that it takes a lot of calls to be fully optimized. - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1797086938
Re: RFR: 8305814: Update Xalan Java to 2.7.3
On Mon, 6 Nov 2023 22:52:47 GMT, Joe Wang wrote: > Xalan 2.7.3: merge minor changes from the upstream project. > > Test: existing XML tests pass Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16532#pullrequestreview-1716553613
Re: RFR: 8317742: ISO Standard Date Format implementation consistency on DateTimeFormatter and String.format [v4]
On Mon, 6 Nov 2023 14:55:54 GMT, Shaojin Wen wrote:
>> j.u.Formatter now prints "%tF" (iso standard date) and the result is
>> incorrect when processing year < 0 or year >
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> revert localize change
src/java.base/share/classes/java/util/Formatter.java line 4495:
> 4493: int year = t.get(ChronoField.YEAR);
> 4494: if (year < 0) {
> 4495: sb.append('-');
Some locales use a different code point to express negative numbers from
`hyphen-minus`, such as `minus sign` (U+2212). I'd suggest getting it from
`DecimalFormatSymbols.getMinusSign()`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16033#discussion_r1384186755
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]
On Mon, 6 Nov 2023 23:28:30 GMT, Phil Race wrote: >So we have somewhere around a fixed 125ms startup cost for the FFM case - as >measured on my Mac, but only 35-40ms of that is attributable to the specific needs of layout. That looks unfortunate. I guess if we will start to use ffm in other places we can easily spend of 1 second budget on startup=( > layoutCnt=16000 total=193ms <<< app fully displayed vs > layoutCnt=16000 total=453ms <<< app fully displayed It looks strange that 16000 calls are not enough to warmup, and the difference is so large. - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1797052585
RFR: 8305814: Update Xalan Java to 2.7.3
Xalan 2.7.3: merge minor changes from the upstream project. Test: existing XML tests pass - Commit messages: - 8305814: Update Xalan Java to 2.7.3 Changes: https://git.openjdk.org/jdk/pull/16532/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16532=00 Issue: https://bugs.openjdk.org/browse/JDK-8305814 Stats: 66 lines in 4 files changed: 19 ins; 22 del; 25 mod Patch: https://git.openjdk.org/jdk/pull/16532.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16532/head:pull/16532 PR: https://git.openjdk.org/jdk/pull/16532
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]
On Mon, 6 Nov 2023 18:52:05 GMT, Sergey Bylokhov wrote: > Since we plan to import it into jdk22, do you have some performance data to > share? any positive or negative effects of this migration? There's three phases - (1) startup, (2) warmup and (3) warmed up performance. JNI has minimal startup / warmup cost, getting to warmed up performance right away. So if your app starts up and makes just one call to layout, JNI wins easily. But if it keeps going, then FFM comes out ahead, even counting that startup /warmup cost. There's a cost to the first time some code in JDK initialises the core FFM. If that code happens to be this layout code, it'll see that overhead. That was somewhere around 75ms on my Mac. On top of that there's the cost of creating the specific method handles and var handles I have 11 of these, and the total there is about 35-40ms. So we have somewhere around a fixed 125ms startup cost for the FFM case - as measured on my Mac, but only 35-40ms of that is attributable to the specific needs of layout. And there is some potential for that code to get faster some day Also if any of the techniques such as AppCDS, or some day, Leyden condensers, are used then there is also potential to eliminate much of the warmup cost. The FFM path then needs to be warmed. Once warmed up, FFM is always as fast or faster than JNI. 20% faster is typical as measured by a small test that just calls layout in a loop. It was tried with varying lengths of string. For just a single char, FFM was only a little faster, but gets better for longer strings. Once we start to use layout, we use it a lot, so you reach many thousands of calls very quickly. Just resizing your UI window causes that. It doesn't take long for FFM to become an overall win. That includes amortizing the cost of the startup / warmup time. As well as a microbenchmark, I looked at what it does in an app consisting of a Swing JTextArea displaying a decent amount of Hindi using an OpenType Indic font on Mac. That takes just over 16,000 (!) calls to layout to get to fully displayed. Then if you just resize back and forth in just a few seconds FFM catches up and overtakes I'll show numbers below - this measure all the FFM+layout costs but nothing else in the app. It bears out what I said about startup. "layoutCnt" is the number of calls to the method to do layout on a single run of text. The numbers look like a lot of calls to layout and you might think that took hours but this really is just about 20-30 secs of manual resizing to get to one million calls. JNI == layoutCnt=1 total=3ms <<< JNI very fast to start up layoutCnt=2 total=3ms layoutCnt=3 total=3ms layoutCnt=4 total=4ms layoutCnt=5 total=4ms layoutCnt=1000 total=31ms layoutCnt=2000 total=40ms << 9-10ms per thousand calls (40-31) layoutCnt=3000 total=51ms layoutCnt=4000 total=61ms layoutCnt=5000 total=69ms layoutCnt=6000 total=77ms layoutCnt=7000 total=90ms layoutCnt=8000 total=100ms layoutCnt=9000 total=113ms layoutCnt=1 total=122ms layoutCnt=11000 total=134ms layoutCnt=12000 total=150ms layoutCnt=13000 total=157ms layoutCnt=14000 total=169ms layoutCnt=15000 total=181ms layoutCnt=16000 total=193ms <<< app fully displayed ... layoutCnt=25 total=2450ms <<< rough point at which they are equal ... layoutCnt=100 total=9115ms <<< after 1 million calls FFM is clearly behind layoutCnt=1001000 total=9124ms << STILL 9-10ms per thousand calls (9124-9115) FFM === layoutCnt=1 total=186ms << // FFM slow to start up, includes 75ms core FFM, 35-40 varhandles + no JIT yet layoutCnt=2 total=188ms layoutCnt=3 total=189ms layoutCnt=4 total=195ms layoutCnt=5 total=195ms layoutCnt=1000 total=269ms layoutCnt=2000 total=284ms << 15 ms per thousand calls (284-269) layoutCnt=3000 total=301ms layoutCnt=4000 total=317ms layoutCnt=5000 total=333ms layoutCnt=6000 total=348ms layoutCnt=7000 total=365ms layoutCnt=8000 total=376ms layoutCnt=9000 total=388ms layoutCnt=1 total=397ms layoutCnt=11000 total=407ms layoutCnt=12000 total=419ms layoutCnt=13000 total=425ms layoutCnt=14000 total=435ms layoutCnt=15000 total=444ms layoutCnt=16000 total=453ms <<< app fully displayed ... layoutCnt=25 total=2426ms <<< rough point at which they are equal ... layoutCnt=100 total=8489ms <<< after 1 million calls FFM is clearly ahead layoutCnt=1001000 total=8496ms << now about 7 ms per thousand calls (8496-8489) - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1797025476
Re: RFR: 8319378: Spec for j.util.Timer::purge and j.util.Timer::cancel could be improved
On Fri, 3 Nov 2023 20:40:11 GMT, Justin Lu wrote: > Please review this PR which clarifies the definition of a _cancelled_ task in > _j.util.Timer::purge_ and _j.util.Timer::cancel_. > > Timer::purge claims that its return value is the number of tasks in the queue > that were cancelled. This can be misleading, as a user can call > Timer::cancel, thinking the rest of the tasks in the queue will be canceled > (which should be the return value of Timer::purge). > > In actuality, Timer::cancel _discards_ all of the tasks in the queue. For a > task to have been _cancelled_, the task itself must have called > TimerTask::cancel. > > This change emphasizes the difference between _discarding_ and _cancelling_ a > task. > Additionally, this change also includes a drive-by update to use an _apiNote_ > and _implNote_ tag in the class description. LGTM - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16503#pullrequestreview-1716478291
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v2]
> Could I have a review of a PR that removes the bytecode instrumentation for > the exception events. > > Testing: jdk/jdk/jfr + tier1 + tier2 Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Remove SecurityException and IllegalArgumentException from throws clause - Changes: - all: https://git.openjdk.org/jdk/pull/16493/files - new: https://git.openjdk.org/jdk/pull/16493/files/95a54349..23a07a18 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16493=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16493=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16493.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16493/head:pull/16493 PR: https://git.openjdk.org/jdk/pull/16493
Re: RFR: 8319374: JFR: Remove instrumentation for exception events
On Mon, 6 Nov 2023 15:49:02 GMT, Alan Bateman wrote:
>> Could I have a review of a PR that removes the bytecode instrumentation for
>> the exception events.
>>
>> Testing: jdk/jdk/jfr + tier1 + tier2
>
> src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 37:
>
>> 35: private static final AtomicLong numThrowables = new AtomicLong();
>> 36:
>> 37: public static void enable() throws NoSuchFieldException,
>> SecurityException, IllegalArgumentException, IllegalAccessException {
>
> SecurityException and IllegaArgumentException are unchecked so don't need
> them in the throws declaration.
Will fix.
> src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 44:
>
>> 42:
>> 43: public static void traceError(Class clazz, String message) {
>> 44: if (OutOfMemoryError.class.isAssignableFrom(clazz)) {
>
> StackOverflowError is likely problematic too, maybe it should be
> VirtualMachineError.
I remember asking the same question ten years ago, when Nils did the original
implementation, but I think it was only needed for OOM, because it creates an
infinite loop when the event object was allocated, which resulted in a
StackOverflowError instead OOM.
Some OOM tests failed with JFR enabled.
The event object allocation has been removed, but I think we can run into the
allocation recursion by other means. I looked into it a few years ago, but I
don't remember exactly why it failed.
If a SOE happens, I think we are fine. There is something that prevents
infinite recursion when the SOE object is created. Perhaps it is preallocated?
I prefer to not change the behavior, at least not in this PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384124648
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384124213
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v6]
> Limit native memory allocation and move write loop from the native layer into > Java. This change should make the OOME reported in the issue much less likely. Brian Burkhalter has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge - Merge - 6478546: do-while -> while-do in writeBytes; remove NULL and bounds checks in native - 6478546: Move buffer clamping up to Java layer; correct read behavior to match legacy - 6478546: Decrease malloc limit to 1.5 MB - 6478546: Minor refactoring - 6478546: Prevent short read - 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available - Changes: - all: https://git.openjdk.org/jdk/pull/14981/files - new: https://git.openjdk.org/jdk/pull/14981/files/3a016a72..408e2df6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14981=05 - incr: https://webrevs.openjdk.org/?repo=jdk=14981=04-05 Stats: 69129 lines in 2383 files changed: 38779 ins; 14073 del; 16277 mod Patch: https://git.openjdk.org/jdk/pull/14981.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14981/head:pull/14981 PR: https://git.openjdk.org/jdk/pull/14981
RFR: JDK-8311961 Update Manual Test Groups for ATR JDK22
Updated jdk_core_manual test groups. - Commit messages: - JDK-8311961 Update Manual Test Groups for ATR JDK22 Changes: https://git.openjdk.org/jdk/pull/16531/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16531=00 Issue: https://bugs.openjdk.org/browse/JDK-8311961 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16531.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16531/head:pull/16531 PR: https://git.openjdk.org/jdk/pull/16531
Re: RFR: 8319324: FFM: Reformat javadocs [v9]
On Mon, 6 Nov 2023 20:08:45 GMT, Per Minborg wrote: >> This PR proposes to reformat all the JavaDocs for the FFM API. This would >> bring the FFM API docs more in line with the existing Java documentation >> (see below). Occasional drive-by fixes are also included in this PR (such >> as spelling and capitalization). >> >> I am aware this PR will (if approved) make a significant mark in the change >> logs which is regrettable. >> >> Background: >> >> Older classes like `Object` and `List` have a maximum line length of 80 >> characters whereas newer classes like `ScopedValue` have a maximum line >> length of 90 characters. >> >> The FFM API currently has javadoc lines that exceed 135 characters per line. >> It is also customary to use double spaces when starting a new sentence. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Update after additional reviews src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 913: > 911: * @param step the step factor at which subsequence sequence > elements are to be > 912: * selected > 913: * @return a path element which selects the sequence element > layout in the Good catch! - PR Review Comment: https://git.openjdk.org/jdk/pull/16518#discussion_r1384074887
RFR: 8319563: Reformat code in the FFM API
This PR proposes to reformat a small number of code line for better appearance. - Commit messages: - Reformat some code Changes: https://git.openjdk.org/jdk/pull/16529/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16529=00 Issue: https://bugs.openjdk.org/browse/JDK-8319563 Stats: 12 lines in 3 files changed: 7 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16529.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16529/head:pull/16529 PR: https://git.openjdk.org/jdk/pull/16529
RFR: 8319556: Harmonize interface formatting in the FFM API
This PR proposes to remove two `permits` declarations where the line overflows the stipulated maximum column with. Also, it proposes to harmonize the layout of `permit` formatting so they are the the same throughout the API. This PR might be perceived as over worked but I think it nice to get consistency across the API now that we go final. - Commit messages: - Reformat interface permit declarations Changes: https://git.openjdk.org/jdk/pull/16528/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16528=00 Issue: https://bugs.openjdk.org/browse/JDK-8319556 Stats: 44 lines in 11 files changed: 21 ins; 0 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/16528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16528/head:pull/16528 PR: https://git.openjdk.org/jdk/pull/16528
RFR: 8319560: Reformat method parameters in the FFM API
This PR proposes to reformat some method parameters in the FFM API. The proposed changes are a bit opinion based eve though there are some underlying principles. - Commit messages: - Harmonize method parameter layouts Changes: https://git.openjdk.org/jdk/pull/16527/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16527=00 Issue: https://bugs.openjdk.org/browse/JDK-8319560 Stats: 28 lines in 4 files changed: 15 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/16527.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16527/head:pull/16527 PR: https://git.openjdk.org/jdk/pull/16527
Re: RFR: 8318144: Match on enum constants with body compiles but fails with MatchException [v3]
On Mon, 6 Nov 2023 20:47:06 GMT, Jan Lahoda wrote:
>> For code like:
>>
>> enum E {A {}, B {} }
>> Object o = E.A;
>> switch (o) {
>> case E.A -> System.err.println(o);
>> default -> System.err.println("default");
>> }
>>
>>
>> The result is `default`, not `A`, due to incorrect classes being compared.
>> Thanks for @liach for noting the solution here:
>> https://github.com/openjdk/jdk/pull/16489#discussion_r1381411165
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adding tests as suggested.
lgtm, thanks!
-
Marked as reviewed by vromero (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16499#pullrequestreview-1716233470
Re: RFR: 8318144: Match on enum constants with body compiles but fails with MatchException [v2]
On Mon, 6 Nov 2023 19:01:38 GMT, Vicente Romero wrote: >> This method is also used in the `typeSwitch` (i.e. ordinary pattern matching >> switch, which may contain qualified enum constants intermixed with >> patterns), and so the `value` may be any object. If it is not an Enum, we >> can avoid the potential costly resolution. > > I see shouldn't we have a test that stresses this code? I mean that we are > returning `false` for cases that are not enums? I thought there's quite a few such tests, but turns out there's just a few (or maybe only one). So adding some more: [cbc9cb5](https://github.com/openjdk/jdk/pull/16499/commits/cbc9cb569b06c2f3d5e503519082c6243b51f9c8). Thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/16499#discussion_r1383960309
Re: RFR: 8318144: Match on enum constants with body compiles but fails with MatchException [v3]
> For code like:
>
> enum E {A {}, B {} }
> Object o = E.A;
> switch (o) {
> case E.A -> System.err.println(o);
> default -> System.err.println("default");
> }
>
>
> The result is `default`, not `A`, due to incorrect classes being compared.
> Thanks for @liach for noting the solution here:
> https://github.com/openjdk/jdk/pull/16489#discussion_r1381411165
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Adding tests as suggested.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/16499/files
- new: https://git.openjdk.org/jdk/pull/16499/files/5f40a8eb..cbc9cb56
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=16499=02
- incr: https://webrevs.openjdk.org/?repo=jdk=16499=01-02
Stats: 12 lines in 2 files changed: 10 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/16499.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/16499/head:pull/16499
PR: https://git.openjdk.org/jdk/pull/16499
Re: RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader [v2]
> This is a regression caused by JDK-8302791. IAE should be thrown when an > interface is not visible to the given class loader but NPE is thrown instead > when the loader is null. The boot loader has no name and so the fix will > print `null` in the exception message. > `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to > junit and updated to test IAE thrown because proxy interface is not visible > to null loader. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/16525/files - new: https://git.openjdk.org/jdk/pull/16525/files/5fef84a4..4be40b5d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16525=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16525=00-01 Stats: 65 lines in 1 file changed: 10 ins; 34 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/16525.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16525/head:pull/16525 PR: https://git.openjdk.org/jdk/pull/16525
Re: RFR: 8319324: FFM: Reformat javadocs [v9]
> This PR proposes to reformat all the JavaDocs for the FFM API. This would > bring the FFM API docs more in line with the existing Java documentation (see > below). Occasional drive-by fixes are also included in this PR (such as > spelling and capitalization). > > I am aware this PR will (if approved) make a significant mark in the change > logs which is regrettable. > > Background: > > Older classes like `Object` and `List` have a maximum line length of 80 > characters whereas newer classes like `ScopedValue` have a maximum line > length of 90 characters. > > The FFM API currently has javadoc lines that exceed 135 characters per line. > It is also customary to use double spaces when starting a new sentence. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Update after additional reviews - Changes: - all: https://git.openjdk.org/jdk/pull/16518/files - new: https://git.openjdk.org/jdk/pull/16518/files/94518ac5..eef52287 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16518=08 - incr: https://webrevs.openjdk.org/?repo=jdk=16518=07-08 Stats: 36 lines in 2 files changed: 0 ins; 10 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/16518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16518/head:pull/16518 PR: https://git.openjdk.org/jdk/pull/16518
Re: RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader
On Mon, 6 Nov 2023 19:46:40 GMT, Alan Bateman wrote:
>> This is a regression caused by JDK-8302791. IAE should be thrown when an
>> interface is not visible to the given class loader but NPE is thrown instead
>> when the loader is null. The boot loader has no name and so the fix will
>> print `null` in the exception message.
>> `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to
>> junit and updated to test IAE thrown because proxy interface is not visible
>> to null loader.
>
> test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 90:
>
>> 88: } catch (IllegalArgumentException e) {
>> 89: System.err.println(e.getMessage());
>> 90: // assume exception is for intended failure
>
> The "throw new Error(message)" could be replaced with fail(message) or use
> assertThrows(IllegalArgument.class, () -> Proxy.getProxyClass(...)).
Will do. I should clean up more of the existing code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383905730
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v13]
On Mon, 6 Nov 2023 19:53:03 GMT, Jim Laskey wrote: >> Address changes from JEP 445 to JEP 463. >> >> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. >> >> - Don't mark class on read. >> >> - Remove reflection and annotation processing related to unnamed classes. >> >> - Simplify main method search. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Rename unnamed class tests and examples Look good. Thanks - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1716139190
Re: RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader
On Mon, 6 Nov 2023 19:12:28 GMT, Mandy Chung wrote:
> This is a regression caused by JDK-8302791. IAE should be thrown when an
> interface is not visible to the given class loader but NPE is thrown instead
> when the loader is null. The boot loader has no name and so the fix will
> print `null` in the exception message.
> `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to
> junit and updated to test IAE thrown because proxy interface is not visible
> to null loader.
test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 61:
> 59: }
> 60:
> 61: static Stream proxyInterfaces() {
These are cases where IAE is expected so maybe the method source should be
badProxyInterfaces to make it clearer that they are expected to fail?
test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 90:
> 88: } catch (IllegalArgumentException e) {
> 89: System.err.println(e.getMessage());
> 90: // assume exception is for intended failure
The "throw new Error(message)" should be replaced with fail(message) or use
assertThrows(IllegalArgument.class, () -> Proxy.getProxyClass(...)).
test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 105:
> 103: if (Modifier.isPublic(nonPublic2.getModifiers())) {
> 104: throw new Error("Interface " + nonPublicIntrfaceName +
> 105: " is public and need to be changed!");
You could use assertEquals here, or at least replace the throw new Error with
fail, up to you.
test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java line 125:
> 123:
> 124: private static final String[] CPATHS =
> System.getProperty("test.classes", ".")
> 125:
> .split(File.pathSeparator);
It might be a bit clearer to define TEST_CLASSES to be the value
System.getProperty("test.classes", "."), and split it in
testNonVisibleInterface.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383882661
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383885042
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383886740
PR Review Comment: https://git.openjdk.org/jdk/pull/16525#discussion_r1383896386
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v19]
On Sun, 12 Mar 2023 21:28:59 GMT, null wrote: >> Sorting: >> >> - adopt radix sort for sequential and parallel sorts on >> int/long/float/double arrays (almost random and length > 6K) >> - fix tryMergeRuns() to better handle case when the last run is a single >> element >> - minor javadoc and comment changes >> >> Testing: >> - add new data inputs in tests for sorting >> - add min/max/infinity values to float/double testing >> - add tests for radix sort > > null has updated the pull request incrementally with one additional commit > since the last revision: > > JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) > > * Use uninitialized array for buffers will resolve merge conflicts - PR Comment: https://git.openjdk.org/jdk/pull/3938#issuecomment-1796241336
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v13]
> Address changes from JEP 445 to JEP 463. > > - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. > > - Don't mark class on read. > > - Remove reflection and annotation processing related to unnamed classes. > > - Simplify main method search. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Rename unnamed class tests and examples - Changes: - all: https://git.openjdk.org/jdk/pull/16461/files - new: https://git.openjdk.org/jdk/pull/16461/files/0bd5b477..30db Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16461=12 - incr: https://webrevs.openjdk.org/?repo=jdk=16461=11-12 Stats: 24 lines in 8 files changed: 12 ins; 12 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16461.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16461/head:pull/16461 PR: https://git.openjdk.org/jdk/pull/16461
RFR: 8187655: jdk.lambda.vm.InterfaceAccessFlagsTest.testPrivateMethodCall needs update after nestmates support
`jdk.lambda.vm.InterfaceAccessFlagsTest` uses `ClassToInterfaceConverter` to mechanically convert a classfile for a Class into an in-memory class representation of an equivalent Interface. `testPrivateMethodCall` tests to invoke a private method. Before nestmates, invoking a private class method and a private interface method both use `Invokespecial`. With the nestmate changes, the class uses `invokevirtual` but the interface must use `invokeinterface` but this conversion is not handled by the existing `ClassToInterfaceConverter`. This fix converts `ClassToInterfaceConverter` to use the Class-File API to properly convert a classfile from a class to an interface including method invocation from `invokevirtual` to `invokeinterface`. The old custom bytecode manipulation code can be dropped. - Commit messages: - 8187655: InterfaceAccessFlagsTest.testPrivateMethodCall needs update after nestmates support Changes: https://git.openjdk.org/jdk/pull/16526/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16526=00 Issue: https://bugs.openjdk.org/browse/JDK-8187655 Stats: 594 lines in 5 files changed: 5 ins; 538 del; 51 mod Patch: https://git.openjdk.org/jdk/pull/16526.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16526/head:pull/16526 PR: https://git.openjdk.org/jdk/pull/16526
RFR: 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader
This is a regression caused by JDK-8302791. IAE should be thrown when an interface is not visible to the given class loader but NPE is thrown instead when the loader is null. The boot loader has no name and so the fix will print `null` in the exception message. `test/jdk/java/lang/reflect/Proxy/ClassRestrictions.java` is converted to junit and updated to test IAE thrown because proxy interface is not visible to null loader. - Commit messages: - 8319436: Proxy.newProxyInstance throws NPE if loader is null and interface not visible from class loader Changes: https://git.openjdk.org/jdk/pull/16525/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16525=00 Issue: https://bugs.openjdk.org/browse/JDK-8319436 Stats: 184 lines in 2 files changed: 63 ins; 96 del; 25 mod Patch: https://git.openjdk.org/jdk/pull/16525.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16525/head:pull/16525 PR: https://git.openjdk.org/jdk/pull/16525
Re: RFR: 8318144: Match on enum constants with body compiles but fails with MatchException [v2]
On Mon, 6 Nov 2023 18:51:59 GMT, Jan Lahoda wrote:
>> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 397:
>>
>>> 395:
>>> 396: try {
>>> 397: if (!(value instanceof Enum enumValue)) {
>>
>> this code seems to be here with the only purpose of casting `value` to
>> `Enum`, if this is the case shouldn't this intention be more explicit?
>
> This method is also used in the `typeSwitch` (i.e. ordinary pattern matching
> switch, which may contain qualified enum constants intermixed with patterns),
> and so the `value` may be any object. If it is not an Enum, we can avoid the
> potential costly resolution.
I see shouldn't we then have a test that stresses this code?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16499#discussion_r1383825208
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v3]
On Fri, 3 Nov 2023 22:44:39 GMT, Sandhya Viswanathan
wrote:
>> Jatin Bhateja has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Restricting masked sub-word gather to AVX512 target to align with integral
>> gather support.
>
> src/hotspot/cpu/x86/x86.ad line 4152:
>
>> 4150:
>> 4151: instruct vgather_subwordLE8B_off(vec dst, memory mem, rRegP idx, rRegI
>> offset, rRegP tmp, rRegI rtmp) %{
>> 4152: predicate(is_subword_type(Matcher::vector_element_basic_type(n)) &&
>> Matcher::vector_length_in_bytes(n) <= 8);
>
> !VM_Version::supports_avx512bw() is missing for non avx3 cases.
This comment is still pending?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1383806980
Re: RFR: 8318650: Optimized subword gather for x86 targets. [v3]
On Sun, 5 Nov 2023 12:58:57 GMT, Jatin Bhateja wrote:
>> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 1606:
>>
>>> 1604: void C2_MacroAssembler::vpgather8b_offset(BasicType elem_bt,
>>> XMMRegister dst, Register base, Register idx_base,
>>> 1605: Register offset, Register
>>> rtmp, int vlen_enc) {
>>> 1606: vpxor(dst, dst, dst, vlen_enc);
>>
>> Why the vpxor here and in vpgather8b. These are not masked gathers.
>
> This is to clear the vector gathering 8 subwords with each iteration.
This is not a masked operation so every lane of dst will be written through
pinsrw/pinsrb. An vpxor before is not required.
>> src/hotspot/cpu/x86/x86.ad line 1921:
>>
>>> 1919: case Op_LoadVectorGatherMasked:
>>> 1920: if (!is_subword_type(bt) && size_in_bits < 512 &&
>>> !VM_Version::supports_avx512vl()) {
>>> 1921: return false;
>>
>> Also need to return false when size_in_bits == 64 for non subword types.
>
> match_rule_supported_vector called in the beginning will enforce these
> checks.
This method is match_rule_support_vector and it is not enforcing this check
now. It was doing so before through fall through.
>> src/hotspot/cpu/x86/x86.ad line 1939:
>>
>>> 1937: // fallthrough
>>> 1938: case Op_LoadVectorGather:
>>> 1939: if (!is_subword_type(bt) && size_in_bits == 64 ) {
>>
>> Since this is a fallthrough case, the change also applies to
>> StoreVectorScatter*. The !is_subword_type() is needed only for
>> LoadVectorGather.
>
> We do not intrinsify sub-word scatter operations currently.
The StoreVectorScatter* should return false when size_in_bits == 64
irrespective of subword. It was doing so before and is not doing so now.
>> src/hotspot/share/opto/vectorIntrinsics.cpp line 1579:
>>
>>> 1577: Node* index= argument(11);
>>> 1578: Node* indexMap = argument(12);
>>> 1579: Node* indexM = argument(13);
>>
>> Could be renamed as follows for better understanding: index -> offset,
>> indexM -> indexOffset. Also this should be moved under else part of if
>> (is_scatter).
>
> Naming scheme is based on the actual names used in intrinsic entry point for
> these arguments.
How about moving this to else part as we are not using these for Scatter?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1383795523
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1383799090
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1383803037
PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1383818954
Re: RFR: 8318144: Match on enum constants with body compiles but fails with MatchException [v2]
On Mon, 6 Nov 2023 18:12:10 GMT, Vicente Romero wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Re-ordering tests as suggested.
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 397:
>
>> 395:
>> 396: try {
>> 397: if (!(value instanceof Enum enumValue)) {
>
> this code seems to be here with the only purpose of casting `value` to
> `Enum`, if this is the case shouldn't this intention be more explicit?
This method is also used in the `typeSwitch` (i.e. ordinary pattern matching
switch, which may contain qualified enum constants intermixed with patterns),
and so the `value` may be any object. If it is not an Enum, we can avoid the
potential costly resolution.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16499#discussion_r1383814245
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]
On Wed, 25 Oct 2023 23:42:08 GMT, Phil Race wrote: >> 8318364: Add an FFM-based implementation of harfbuzz OpenType layout > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > indentation Since we plan to import it into jdk22, do you have some performance data to share? any positive or negative effects of this migration? - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1795927326
Re: RFR: 8318144: Match on enum constants with body compiles but fails with MatchException [v2]
On Mon, 6 Nov 2023 07:34:38 GMT, Jan Lahoda wrote:
>> For code like:
>>
>> enum E {A {}, B {} }
>> Object o = E.A;
>> switch (o) {
>> case E.A -> System.err.println(o);
>> default -> System.err.println("default");
>> }
>>
>>
>> The result is `default`, not `A`, due to incorrect classes being compared.
>> Thanks for @liach for noting the solution here:
>> https://github.com/openjdk/jdk/pull/16489#discussion_r1381411165
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Re-ordering tests as suggested.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 397:
> 395:
> 396: try {
> 397: if (!(value instanceof Enum enumValue)) {
this code seems to be here with the only purpose of casting `value` to
`Enum`, if this is the case shouldn't this intention be more explicit?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16499#discussion_r1383763927
Re: RFR: 8319324: FFM: Reformat javadocs [v8]
On Mon, 6 Nov 2023 17:35:31 GMT, Per Minborg wrote:
>> This PR proposes to reformat all the JavaDocs for the FFM API. This would
>> bring the FFM API docs more in line with the existing Java documentation
>> (see below). Occasional drive-by fixes are also included in this PR (such
>> as spelling and capitalization).
>>
>> I am aware this PR will (if approved) make a significant mark in the change
>> logs which is regrettable.
>>
>> Background:
>>
>> Older classes like `Object` and `List` have a maximum line length of 80
>> characters whereas newer classes like `ScopedValue` have a maximum line
>> length of 90 characters.
>>
>> The FFM API currently has javadoc lines that exceed 135 characters per line.
>> It is also customary to use double spaces when starting a new sentence.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove additional redundant full stops
Looks good. I found some (few) sentences that still have a full stop in
`@param`/`@throws`
src/java.base/share/classes/java/nio/channels/FileChannel.java line 1072:
> 1070: * @throws WrongThreadException
> 1071: * If {@code arena} is a confined scoped arena, and this
> method is called
> 1072: * from a thread {@code T}, other than the scoped arena's
> owner thread.
period?
-
PR Review: https://git.openjdk.org/jdk/pull/16518#pullrequestreview-1715840997
PR Review Comment: https://git.openjdk.org/jdk/pull/16518#discussion_r1383737974
Re: RFR: 8319324: FFM: Reformat javadocs [v8]
On Mon, 6 Nov 2023 17:35:31 GMT, Per Minborg wrote:
>> This PR proposes to reformat all the JavaDocs for the FFM API. This would
>> bring the FFM API docs more in line with the existing Java documentation
>> (see below). Occasional drive-by fixes are also included in this PR (such
>> as spelling and capitalization).
>>
>> I am aware this PR will (if approved) make a significant mark in the change
>> logs which is regrettable.
>>
>> Background:
>>
>> Older classes like `Object` and `List` have a maximum line length of 80
>> characters whereas newer classes like `ScopedValue` have a maximum line
>> length of 90 characters.
>>
>> The FFM API currently has javadoc lines that exceed 135 characters per line.
>> It is also customary to use double spaces when starting a new sentence.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove additional redundant full stops
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 510:
> 508: * the given alignment constraint (in bytes)}
> 509: *
> 510: * @param byteAlignment the layout alignment constraint, expressed
> in bytes.
period?
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 848:
> 846:
> 847: /**
> 848: * Returns a path element which selects a member layout with the
> given name in a
It feels like the javadoc of the methods in here can be tightened using
`{@return}`. Maybe for another PR.
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 1017:
> 1015: * @param elementCount the sequence element count
> 1016: * @param elementLayout the sequence element layout
> 1017: * @return the new sequence layout with the given element layout
> and size.
period?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16518#discussion_r1383726736
PR Review Comment: https://git.openjdk.org/jdk/pull/16518#discussion_r1383728935
PR Review Comment: https://git.openjdk.org/jdk/pull/16518#discussion_r1383729384
Re: RFR: 8319324: FFM: Reformat javadocs [v8]
> This PR proposes to reformat all the JavaDocs for the FFM API. This would > bring the FFM API docs more in line with the existing Java documentation (see > below). Occasional drive-by fixes are also included in this PR (such as > spelling and capitalization). > > I am aware this PR will (if approved) make a significant mark in the change > logs which is regrettable. > > Background: > > Older classes like `Object` and `List` have a maximum line length of 80 > characters whereas newer classes like `ScopedValue` have a maximum line > length of 90 characters. > > The FFM API currently has javadoc lines that exceed 135 characters per line. > It is also customary to use double spaces when starting a new sentence. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Remove additional redundant full stops - Changes: - all: https://git.openjdk.org/jdk/pull/16518/files - new: https://git.openjdk.org/jdk/pull/16518/files/1cbb951f..94518ac5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16518=07 - incr: https://webrevs.openjdk.org/?repo=jdk=16518=06-07 Stats: 34 lines in 6 files changed: 0 ins; 0 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/16518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16518/head:pull/16518 PR: https://git.openjdk.org/jdk/pull/16518
Re: RFR: 8319324: FFM: Reformat javadocs [v7]
> This PR proposes to reformat all the JavaDocs for the FFM API. This would > bring the FFM API docs more in line with the existing Java documentation (see > below). Occasional drive-by fixes are also included in this PR (such as > spelling and capitalization). > > I am aware this PR will (if approved) make a significant mark in the change > logs which is regrettable. > > Background: > > Older classes like `Object` and `List` have a maximum line length of 80 > characters whereas newer classes like `ScopedValue` have a maximum line > length of 90 characters. > > The FFM API currently has javadoc lines that exceed 135 characters per line. > It is also customary to use double spaces when starting a new sentence. Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - Merge master and update Linker formatting - Harmonize the use of full stops in tags - Remove double spaces after full stops - Review classes not in the foreign package - Update after review - Merge master - Reformat javadocs - FFM: Harmonize the @throws tags in the javadocs - Merge branch 'master' into javadoc-throws - Harmonize some of the javadoc throws - Changes: https://git.openjdk.org/jdk/pull/16518/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16518=06 Stats: 3021 lines in 16 files changed: 868 ins; 18 del; 2135 mod Patch: https://git.openjdk.org/jdk/pull/16518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16518/head:pull/16518 PR: https://git.openjdk.org/jdk/pull/16518
Re: RFR: 8319324: FFM: Reformat javadocs [v6]
> This PR proposes to reformat all the JavaDocs for the FFM API. This would > bring the FFM API docs more in line with the existing Java documentation (see > below). Occasional drive-by fixes are also included in this PR (such as > spelling and capitalization). > > I am aware this PR will (if approved) make a significant mark in the change > logs which is regrettable. > > Background: > > Older classes like `Object` and `List` have a maximum line length of 80 > characters whereas newer classes like `ScopedValue` have a maximum line > length of 90 characters. > > The FFM API currently has javadoc lines that exceed 135 characters per line. > It is also customary to use double spaces when starting a new sentence. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Harmonize the use of full stops in tags - Changes: - all: https://git.openjdk.org/jdk/pull/16518/files - new: https://git.openjdk.org/jdk/pull/16518/files/a24344fb..114ba07d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16518=05 - incr: https://webrevs.openjdk.org/?repo=jdk=16518=04-05 Stats: 381 lines in 11 files changed: 5 ins; 1 del; 375 mod Patch: https://git.openjdk.org/jdk/pull/16518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16518/head:pull/16518 PR: https://git.openjdk.org/jdk/pull/16518
Re: RFR: 8311906: Improve robustness of String constructors with mutable array inputs
On Sat, 4 Nov 2023 00:07:33 GMT, Chen Liang wrote:
>> Strings, after construction, are immutable but may be constructed from
>> mutable arrays of bytes, characters, or integers.
>> The string constructors should guard against the effects of mutating the
>> arrays during construction that might invalidate internal invariants for the
>> correct behavior of operations on the resulting strings. In particular, a
>> number of operations have optimizations for operations on pairs of latin1
>> strings and pairs of non-latin1 strings, while operations between latin1 and
>> non-latin1 strings use a more general implementation.
>>
>> The changes include:
>>
>> - Adding a warning to each constructor with an array as an argument to
>> indicate that the results are indeterminate
>> if the input array is modified before the constructor returns.
>> The resulting string may contain any combination of characters sampled
>> from the input array.
>>
>> - Ensure that strings that are represented as non-latin1 contain at least
>> one non-latin1 character.
>> For latin1 inputs, whether the arrays contain ASCII, ISO-8859-1, UTF8, or
>> another encoding decoded to latin1 the scanning and compression is unchanged.
>> If a non-latin1 character is found, the string is represented as
>> non-latin1 with the added verification that a non-latin1 character is
>> present at the same index.
>> If that character is found to be latin1, then the input array has been
>> modified and the result of the scan may be incorrect.
>> Though a ConcurrentModificationException could be thrown, the risk to an
>> existing application of an unexpected exception should be avoided.
>> Instead, the non-latin1 copy of the input is re-scanned and compressed;
>> that scan determines whether the latin1 or the non-latin1 representation is
>> returned.
>>
>> - The methods that scan for non-latin1 characters and their intrinsic
>> implementations are updated to return the index of the non-latin1 character.
>>
>> - String construction from StringBuilder and CharSequence must also be
>> guarded as their contents may be modified during construction.
>
> src/java.base/share/classes/java/lang/StringUTF16.java line 202:
>
>> 200: @ForceInline
>> 201: public static byte[] compress(final char[] val, final int off,
>> final int count) {
>> 202: byte[] latin1 = new byte[count];
>
> Will this redundant array allocation be costly if we are working with
> mostly-utf16 strings, such as CJK strings with no latin characters?
>
> I suggest we can use a heuristic to read the initial char; if it's utf16 then
> we skip the latin-1 process altogether (and we can assign the utf16 value to
> the initial index to ensure it's non-latin-1 compressible.
We can reconsider this design as a separate PR.
Every additional check has a performance impact and in this bug the goal is to
avoid any regression.
We'll need to gain some insight into the distribution of strings when used in a
non-latin1 application.
How many of the strings are latin1 vs non-latin1, what is the distribution of
string lengths and which APIs are in use in the applications. The
implementation is already pretty good about working with strings of different
coders
but there may be some different choices when converting between char arrays and
int arrays and strings.
> src/java.base/share/classes/java/lang/StringUTF16.java line 411:
>
>> 409: return 2;
>> 410: } else
>> 411: throw new
>> IllegalArgumentException(Integer.toString(codePoint));
>
> `toHexString` might be more informative.
Perhaps, but changing the exception text is out scope for this PR; it has been
decimal since JDK 9 (2015).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1383521445
PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1383527621
Re: RFR: 8311906: Improve robustness of String constructors with mutable array inputs
On Mon, 30 Oct 2023 18:34:44 GMT, Roger Riggs wrote:
> Strings, after construction, are immutable but may be constructed from
> mutable arrays of bytes, characters, or integers.
> The string constructors should guard against the effects of mutating the
> arrays during construction that might invalidate internal invariants for the
> correct behavior of operations on the resulting strings. In particular, a
> number of operations have optimizations for operations on pairs of latin1
> strings and pairs of non-latin1 strings, while operations between latin1 and
> non-latin1 strings use a more general implementation.
>
> The changes include:
>
> - Adding a warning to each constructor with an array as an argument to
> indicate that the results are indeterminate
> if the input array is modified before the constructor returns.
> The resulting string may contain any combination of characters sampled from
> the input array.
>
> - Ensure that strings that are represented as non-latin1 contain at least one
> non-latin1 character.
> For latin1 inputs, whether the arrays contain ASCII, ISO-8859-1, UTF8, or
> another encoding decoded to latin1 the scanning and compression is unchanged.
> If a non-latin1 character is found, the string is represented as non-latin1
> with the added verification that a non-latin1 character is present at the
> same index.
> If that character is found to be latin1, then the input array has been
> modified and the result of the scan may be incorrect.
> Though a ConcurrentModificationException could be thrown, the risk to an
> existing application of an unexpected exception should be avoided.
> Instead, the non-latin1 copy of the input is re-scanned and compressed;
> that scan determines whether the latin1 or the non-latin1 representation is
> returned.
>
> - The methods that scan for non-latin1 characters and their intrinsic
> implementations are updated to return the index of the non-latin1 character.
>
> - String construction from StringBuilder and CharSequence must also be
> guarded as their contents may be modified during construction.
src/java.base/share/classes/java/lang/StringUTF16.java line 202:
> 200: @ForceInline
> 201: public static byte[] compress(final char[] val, final int off, final
> int count) {
> 202: byte[] latin1 = new byte[count];
Will this redundant array allocation be costly if we are working with
mostly-utf16 strings, such as CJK strings with no latin characters?
I suggest we can use a heuristic to read the initial char; if it's utf16 then
we skip the latin-1 process altogether (and we can assign the utf16 value to
the initial index to ensure it's non-latin-1 compressible.
src/java.base/share/classes/java/lang/StringUTF16.java line 411:
> 409: return 2;
> 410: } else
> 411: throw new
> IllegalArgumentException(Integer.toString(codePoint));
`toHexString` might be more informative.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1382296222
PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1382304769
Re: RFR: 8311906: Improve robustness of String constructors with mutable array inputs
On Sun, 5 Nov 2023 13:32:20 GMT, ExE Boss wrote:
>> Strings, after construction, are immutable but may be constructed from
>> mutable arrays of bytes, characters, or integers.
>> The string constructors should guard against the effects of mutating the
>> arrays during construction that might invalidate internal invariants for the
>> correct behavior of operations on the resulting strings. In particular, a
>> number of operations have optimizations for operations on pairs of latin1
>> strings and pairs of non-latin1 strings, while operations between latin1 and
>> non-latin1 strings use a more general implementation.
>>
>> The changes include:
>>
>> - Adding a warning to each constructor with an array as an argument to
>> indicate that the results are indeterminate
>> if the input array is modified before the constructor returns.
>> The resulting string may contain any combination of characters sampled
>> from the input array.
>>
>> - Ensure that strings that are represented as non-latin1 contain at least
>> one non-latin1 character.
>> For latin1 inputs, whether the arrays contain ASCII, ISO-8859-1, UTF8, or
>> another encoding decoded to latin1 the scanning and compression is unchanged.
>> If a non-latin1 character is found, the string is represented as
>> non-latin1 with the added verification that a non-latin1 character is
>> present at the same index.
>> If that character is found to be latin1, then the input array has been
>> modified and the result of the scan may be incorrect.
>> Though a ConcurrentModificationException could be thrown, the risk to an
>> existing application of an unexpected exception should be avoided.
>> Instead, the non-latin1 copy of the input is re-scanned and compressed;
>> that scan determines whether the latin1 or the non-latin1 representation is
>> returned.
>>
>> - The methods that scan for non-latin1 characters and their intrinsic
>> implementations are updated to return the index of the non-latin1 character.
>>
>> - String construction from StringBuilder and CharSequence must also be
>> guarded as their contents may be modified during construction.
>
> src/java.base/share/classes/java/lang/String.java line 566:
>
>> 564: }
>> 565: // Decode with a stable copy, to be the result if the
>> decoded length is the same
>> 566: byte[] latin1 = Arrays.copyOfRange(bytes, offset,
>> offset + length);
>
> This has to be moved before the `if (dp == length) { … }` check, as that also
> does a copy:
>
> // Decode with a stable copy, to be the result if the decoded
> length is the same
> byte[] latin1 = Arrays.copyOfRange(bytes, offset, offset +
> length);
> int dp = StringCoding.countPositives(latin1, offset, length);
> if (dp == length) {
> this.value = latin1;
> this.coder = LATIN1;
> return;
> }
That may look like an improvement, to share common code, but it results in a
performance hit in the normal case.
The best performing case is to copy and return immediately.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1383539522
Re: RFR: 8311906: Improve robustness of String constructors with mutable array inputs
On Mon, 30 Oct 2023 18:34:44 GMT, Roger Riggs wrote:
> Strings, after construction, are immutable but may be constructed from
> mutable arrays of bytes, characters, or integers.
> The string constructors should guard against the effects of mutating the
> arrays during construction that might invalidate internal invariants for the
> correct behavior of operations on the resulting strings. In particular, a
> number of operations have optimizations for operations on pairs of latin1
> strings and pairs of non-latin1 strings, while operations between latin1 and
> non-latin1 strings use a more general implementation.
>
> The changes include:
>
> - Adding a warning to each constructor with an array as an argument to
> indicate that the results are indeterminate
> if the input array is modified before the constructor returns.
> The resulting string may contain any combination of characters sampled from
> the input array.
>
> - Ensure that strings that are represented as non-latin1 contain at least one
> non-latin1 character.
> For latin1 inputs, whether the arrays contain ASCII, ISO-8859-1, UTF8, or
> another encoding decoded to latin1 the scanning and compression is unchanged.
> If a non-latin1 character is found, the string is represented as non-latin1
> with the added verification that a non-latin1 character is present at the
> same index.
> If that character is found to be latin1, then the input array has been
> modified and the result of the scan may be incorrect.
> Though a ConcurrentModificationException could be thrown, the risk to an
> existing application of an unexpected exception should be avoided.
> Instead, the non-latin1 copy of the input is re-scanned and compressed;
> that scan determines whether the latin1 or the non-latin1 representation is
> returned.
>
> - The methods that scan for non-latin1 characters and their intrinsic
> implementations are updated to return the index of the non-latin1 character.
>
> - String construction from StringBuilder and CharSequence must also be
> guarded as their contents may be modified during construction.
src/java.base/share/classes/java/lang/String.java line 566:
> 564: }
> 565: // Decode with a stable copy, to be the result if the
> decoded length is the same
> 566: byte[] latin1 = Arrays.copyOfRange(bytes, offset, offset
> + length);
This has to be moved before the `if (dp == length) { … }` check, as that also
does a copy:
// Decode with a stable copy, to be the result if the decoded
length is the same
byte[] latin1 = Arrays.copyOfRange(bytes, offset, offset +
length);
int dp = StringCoding.countPositives(latin1, offset, length);
if (dp == length) {
this.value = latin1;
this.coder = LATIN1;
return;
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16425#discussion_r1382576891
Re: RFR: 8311906: Improve robustness of String constructors with mutable array inputs
On Mon, 30 Oct 2023 18:34:44 GMT, Roger Riggs wrote: > Strings, after construction, are immutable but may be constructed from > mutable arrays of bytes, characters, or integers. > The string constructors should guard against the effects of mutating the > arrays during construction that might invalidate internal invariants for the > correct behavior of operations on the resulting strings. In particular, a > number of operations have optimizations for operations on pairs of latin1 > strings and pairs of non-latin1 strings, while operations between latin1 and > non-latin1 strings use a more general implementation. > > The changes include: > > - Adding a warning to each constructor with an array as an argument to > indicate that the results are indeterminate > if the input array is modified before the constructor returns. > The resulting string may contain any combination of characters sampled from > the input array. > > - Ensure that strings that are represented as non-latin1 contain at least one > non-latin1 character. > For latin1 inputs, whether the arrays contain ASCII, ISO-8859-1, UTF8, or > another encoding decoded to latin1 the scanning and compression is unchanged. > If a non-latin1 character is found, the string is represented as non-latin1 > with the added verification that a non-latin1 character is present at the > same index. > If that character is found to be latin1, then the input array has been > modified and the result of the scan may be incorrect. > Though a ConcurrentModificationException could be thrown, the risk to an > existing application of an unexpected exception should be avoided. > Instead, the non-latin1 copy of the input is re-scanned and compressed; > that scan determines whether the latin1 or the non-latin1 representation is > returned. > > - The methods that scan for non-latin1 characters and their intrinsic > implementations are updated to return the index of the non-latin1 character. > > - String construction from StringBuilder and CharSequence must also be > guarded as their contents may be modified during construction. Hello Roger, it looks like there are some whitespace related issues in the changes which jcheck has caught https://github.com/openjdk/jdk/pull/16425/checks?check_run_id=18357062638 and thus hasn't created a RFR for this yet. - PR Comment: https://git.openjdk.org/jdk/pull/16425#issuecomment-1793738211
RFR: 8311906: Improve robustness of String constructors with mutable array inputs
Strings, after construction, are immutable but may be constructed from mutable arrays of bytes, characters, or integers. The string constructors should guard against the effects of mutating the arrays during construction that might invalidate internal invariants for the correct behavior of operations on the resulting strings. In particular, a number of operations have optimizations for operations on pairs of latin1 strings and pairs of non-latin1 strings, while operations between latin1 and non-latin1 strings use a more general implementation. The changes include: - Adding a warning to each constructor with an array as an argument to indicate that the results are indeterminate if the input array is modified before the constructor returns. The resulting string may contain any combination of characters sampled from the input array. - Ensure that strings that are represented as non-latin1 contain at least one non-latin1 character. For latin1 inputs, whether the arrays contain ASCII, ISO-8859-1, UTF8, or another encoding decoded to latin1 the scanning and compression is unchanged. If a non-latin1 character is found, the string is represented as non-latin1 with the added verification that a non-latin1 character is present at the same index. If that character is found to be latin1, then the input array has been modified and the result of the scan may be incorrect. Though a ConcurrentModificationException could be thrown, the risk to an existing application of an unexpected exception should be avoided. Instead, the non-latin1 copy of the input is re-scanned and compressed; that scan determines whether the latin1 or the non-latin1 representation is returned. - The methods that scan for non-latin1 characters and their intrinsic implementations are updated to return the index of the non-latin1 character. - String construction from StringBuilder and CharSequence must also be guarded as their contents may be modified during construction. - Commit messages: - Cleanup javadoc, whitespace, and formatting in the JMH benchmark - Update RiscV implementation of intrinsic for java.lang.StringUTF16.compress - Javadoc formatting - 8311906: Improve robustness of String constructors with mutable array arguments Changes: https://git.openjdk.org/jdk/pull/16425/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16425=00 Issue: https://bugs.openjdk.org/browse/JDK-8311906 Stats: 1057 lines in 11 files changed: 859 ins; 82 del; 116 mod Patch: https://git.openjdk.org/jdk/pull/16425.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16425/head:pull/16425 PR: https://git.openjdk.org/jdk/pull/16425
Re: RFR: 8319423: Improve Year.isLeap by checking divisibility by 16 [v2]
On Mon, 6 Nov 2023 00:50:22 GMT, Claes Redestad wrote: > I suggest we go ahead and integrate this, file an RFE to re-examine the > division-by-constant in C2, then re-evaluate these `isLeapYear` micros in > that new environment. These are good improvements and are beneficial with or without other fixes, so should go ahead independently. - PR Comment: https://git.openjdk.org/jdk/pull/16491#issuecomment-1795320991
Re: RFR: 8319324: FFM: Reformat javadocs [v5]
> This PR proposes to reformat all the JavaDocs for the FFM API. This would > bring the FFM API docs more in line with the existing Java documentation (see > below). Occasional drive-by fixes are also included in this PR (such as > spelling and capitalization). > > I am aware this PR will (if approved) make a significant mark in the change > logs which is regrettable. > > Background: > > Older classes like `Object` and `List` have a maximum line length of 80 > characters whereas newer classes like `ScopedValue` have a maximum line > length of 90 characters. > > The FFM API currently has javadoc lines that exceed 135 characters per line. > It is also customary to use double spaces when starting a new sentence. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Remove double spaces after full stops - Changes: - all: https://git.openjdk.org/jdk/pull/16518/files - new: https://git.openjdk.org/jdk/pull/16518/files/b37bd31c..a24344fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16518=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16518=03-04 Stats: 390 lines in 13 files changed: 0 ins; 0 del; 390 mod Patch: https://git.openjdk.org/jdk/pull/16518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16518/head:pull/16518 PR: https://git.openjdk.org/jdk/pull/16518
Re: RFR: 8319423: Improve Year.isLeap by checking divisibility by 16 [v2]
On Fri, 3 Nov 2023 23:22:27 GMT, Claes Redestad wrote:
>> https://github.com/cassioneri/eaf suggest this code for leap year
>> calculation:
>>
>> public static boolean isLeap(long year) {
>> int d = year % 100 != 0 ? 4 : 16;
>> return (year & (d - 1)) == 0;
>> }
>>
>> .. with a claim this would compile down to branchless, easily pipelined code.
>>
>> This doesn't currently happen with C2. In the meantime I think we can
>> improve the current code in `Year.isLeap` and `IsoChronology.isLeapYear` by
>> leveraging the fact that the `% 100` check is only needed if `(year & 15) !=
>> 0`:
>>
>>
>> public static boolean isLeap(long year) {
>> return (year & 15) == 0 ? (year & 3) == 0 : (year & 3) == 0 && year
>> % 100 != 0;
>> }
>>
>>
>> Mac M1:
>>
>> Name Cnt Base Error Test Error Unit
>> Change
>> LeapYearBench.isLeapYear15 0,743 ± 0,009 0,994 ± 0,005 ops/us
>> 1,34x (p = 0,000*)
>> LeapYearBench.isLeapYearChrono 15 0,748 ± 0,006 0,991 ± 0,003 ops/us
>> 1,32x (p = 0,000*)
>> LeapYearBench.isLeapYearNS 15 0,558 ± 0,026 0,552 ± 0,033 ops/us
>> 0,99x (p = 0,602 )
>> * = significant
>>
>>
>> Linux x64:
>>
>> Name Cnt Base Error Test Error Unit
>> Change
>> LeapYearBench.isLeapYear15 0.534 ± 0.001 0.765 ± 0.004 ops/us
>> 1.43x (p = 0.000*)
>> LeapYearBench.isLeapYearChrono 15 0.535 ± 0.000 0.753 ± 0.040 ops/us
>> 1.41x (p = 0.000*)
>> LeapYearBench.isLeapYearNS 15 0.352 ± 0.000 0.351 ± 0.001 ops/us
>> 1.00x (p = 0.000*)
>> * = significant
>>
>>
>> 30% higher throughput on M1, 40% on x64. `isLeapYearNS` runs a variant of
>> the code from https://github.com/cassioneri/eaf ported to java - perhaps the
>> JIT can be improved to do whatever clang/gcc does here and achieve an even
>> better speed-up.
>>
>> Testing: so far only java/time/tck/java/time locally, will run a few tiers
>> before filing an enhancement and opening the PR for review.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply similar optimization to GregorianCalendar, sun.util.calendar
Marked as reviewed by rriggs (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/16491#pullrequestreview-1715613111
Re: RFR: 8315680: java/lang/ref/ReachabilityFenceTest.java should run with -Xbatch
On Tue, 3 Oct 2023 07:47:30 GMT, Gergö Barany wrote: > This test requires certain methods to be compiled, but without `-Xbatch` the > compiler races against the test code, which can lead to intermittent failures. Marked as reviewed by never (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16023#pullrequestreview-1715579342
Re: RFR: 8315680: java/lang/ref/ReachabilityFenceTest.java should run with -Xbatch
On Tue, 3 Oct 2023 07:47:30 GMT, Gergö Barany wrote: > This test requires certain methods to be compiled, but without `-Xbatch` the > compiler races against the test code, which can lead to intermittent failures. Marked as reviewed by dnsimon (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16023#pullrequestreview-1715574295
Re: RFR: 8319374: JFR: Remove instrumentation for exception events
On Fri, 3 Nov 2023 12:19:07 GMT, Erik Gahlin wrote:
> Could I have a review of a PR that removes the bytecode instrumentation for
> the exception events.
>
> Testing: jdk/jdk/jfr + tier1 + tier2
src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 37:
> 35: private static final AtomicLong numThrowables = new AtomicLong();
> 36:
> 37: public static void enable() throws NoSuchFieldException,
> SecurityException, IllegalArgumentException, IllegalAccessException {
SecurityException and IllegaArgumentException are unchecked so don't need them
in the throws declaration.
src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 44:
> 42:
> 43: public static void traceError(Class clazz, String message) {
> 44: if (OutOfMemoryError.class.isAssignableFrom(clazz)) {
StackOverflowError is likely problematic too, maybe it should be
VirtualMachineError.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1383552825
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1383552352
RFR: 8319374: JFR: Remove instrumentation for exception events
Could I have a review of a PR that removes the bytecode instrumentation for the exception events. Testing: jdk/jdk/jfr + tier1 + tier2 - Commit messages: - Remove Throwable and Error from instrumentation list - Initial Changes: https://git.openjdk.org/jdk/pull/16493/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16493=00 Issue: https://bugs.openjdk.org/browse/JDK-8319374 Stats: 560 lines in 19 files changed: 268 ins; 281 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/16493.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16493/head:pull/16493 PR: https://git.openjdk.org/jdk/pull/16493
Re: RFR: 8318971: jar v17 should either exit on error immediately or create archive as jar v1.8 did
On Mon, 6 Nov 2023 11:28:23 GMT, Ryan Wallace wrote: > I had a look and its been in since JDK 9 > (https://bugs.openjdk.org/browse/JDK-8158295) but haven’t found any mention > of this as a specific desired behaviour so I am going with just noticed now. > Its not a major blocker as the user can either make sure the missing file is > in place or exclude it from being added to the archive and rerun. > > I agree it will need some thought, I was of the opinion that we should fail > upfront and notify the user of why. The JDK 8 behaviour doesn’t make sense as > the state is recognised as an error and there are tests to validate this, but > they do not validate the archive has not been created, in my opinion we > shouldn’t continue on and create it. There are other areas in the jar tool in > 8 that will clean up (delete) the archive that has been created if there is > an error, this case just seems to fall through the cracks. As part of the decision process, we should consider the behavior of what simillar tools do in this area. We may also want to discuss the merits of whether there should be an option to toggle the behavior - PR Comment: https://git.openjdk.org/jdk/pull/16423#issuecomment-1795072524
Integrated: 8319316: Clarify text around which layouts a linker supports
On Fri, 3 Nov 2023 00:10:48 GMT, Jorn Vernee wrote: > - Add linker note about packed structs. > - Relax language a bit to avoid implying that only listed layouts are > supported. This pull request has now been integrated. Changeset: cdf33735 Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/cdf337357a62dd52c00e56e75912565e15b6adfd Stats: 18 lines in 1 file changed: 16 ins; 0 del; 2 mod 8319316: Clarify text around which layouts a linker supports Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/16485
Re: RFR: 8319324: FFM: Reformat javadocs [v4]
> This PR proposes to reformat all the JavaDocs for the FFM API. This would > bring the FFM API docs more in line with the existing Java documentation (see > below). Occasional drive-by fixes are also included in this PR (such as > spelling and capitalization). > > I am aware this PR will (if approved) make a significant mark in the change > logs which is regrettable. > > Background: > > Older classes like `Object` and `List` have a maximum line length of 80 > characters whereas newer classes like `ScopedValue` have a maximum line > length of 90 characters. > > The FFM API currently has javadoc lines that exceed 135 characters per line. > It is also customary to use double spaces when starting a new sentence. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Review classes not in the foreign package - Changes: - all: https://git.openjdk.org/jdk/pull/16518/files - new: https://git.openjdk.org/jdk/pull/16518/files/0c379b4a..b37bd31c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16518=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16518=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16518/head:pull/16518 PR: https://git.openjdk.org/jdk/pull/16518
Re: RFR: 8319324: FFM: Reformat javadocs [v3]
On Mon, 6 Nov 2023 11:41:23 GMT, Maurizio Cimadamore wrote: > You might also want to look into the MethodHandles XYZCoordinates combinators > (as they have also been added as part of FFM). And, also, > `ModuleLayer::Controller::enableNativeAccess`, > `Module::isNativeAccessEnabled` and `FileChannel::map(... Arena)`. These are checked now. Only FileChannel needed a very small adjustment. - PR Comment: https://git.openjdk.org/jdk/pull/16518#issuecomment-1795032761
Re: RFR: 8319324: FFM: Reformat javadocs [v3]
On Mon, 6 Nov 2023 13:27:40 GMT, Jorn Vernee wrote: > FWIW, I've been sticking to a soft 120 character limit per line when writing > javadoc. Why should we use 90 columns specifically? > > > It is also customary to use double spaces when starting a new sentence. > > I thought this was the exception rather than the rule, to be honest. We should try to get some clarity on this. I've sampled a handful of prominent Java classes and they have the style as in this PR. If we can find support that other styles are to be preferred, I am happy to update accordingly. - PR Comment: https://git.openjdk.org/jdk/pull/16518#issuecomment-1795020671
Re: RFR: 8317742: ISO Standard Date Format implementation consistency on DateTimeFormatter and String.format [v4]
> j.u.Formatter now prints "%tF" (iso standard date) and the result is > incorrect when processing year < 0 or year > Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: revert localize change - Changes: - all: https://git.openjdk.org/jdk/pull/16033/files - new: https://git.openjdk.org/jdk/pull/16033/files/8b31e875..b6313fc0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16033=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16033=02-03 Stats: 31 lines in 2 files changed: 0 ins; 27 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16033.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16033/head:pull/16033 PR: https://git.openjdk.org/jdk/pull/16033
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 12:40:48 GMT, Jim Laskey wrote: >> Address changes from JEP 445 to JEP 463. >> >> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. >> >> - Don't mark class on read. >> >> - Remove reflection and annotation processing related to unnamed classes. >> >> - Simplify main method search. > > Jim Laskey 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 12 additional commits since > the last revision: > > - Merge remote-tracking branch 'upstream/master' into 8315458 > - Don't get args unless necessary > - Remove unnamed classes from examples.not-yet.txt > - Requested corrections > - Changes recommended by Jan > - Revised implicit class test > - Don't store main method info globally. Use addition calls to fetch info. > - Update JEP number in PreviewFeature > - Remove MANDATED flag from implicit classes > - Remove .orig files > - ... and 2 more: https://git.openjdk.org/jdk/compare/8e9e3542...0bd5b477 lgtm - Marked as reviewed by vromero (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1715396892
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 12:40:48 GMT, Jim Laskey wrote: >> Address changes from JEP 445 to JEP 463. >> >> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. >> >> - Don't mark class on read. >> >> - Remove reflection and annotation processing related to unnamed classes. >> >> - Simplify main method search. > > Jim Laskey 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 12 additional commits since > the last revision: > > - Merge remote-tracking branch 'upstream/master' into 8315458 > - Don't get args unless necessary > - Remove unnamed classes from examples.not-yet.txt > - Requested corrections > - Changes recommended by Jan > - Revised implicit class test > - Don't store main method info globally. Use addition calls to fetch info. > - Update JEP number in PreviewFeature > - Remove MANDATED flag from implicit classes > - Remove .orig files > - ... and 2 more: https://git.openjdk.org/jdk/compare/baf8d59f...0bd5b477 Changes in the two source launcher files `Main.java` and `SourceLauncherTest.java` look good to me - less code is more! - PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1715230089
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 12:40:48 GMT, Jim Laskey wrote: >> Address changes from JEP 445 to JEP 463. >> >> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. >> >> - Don't mark class on read. >> >> - Remove reflection and annotation processing related to unnamed classes. >> >> - Simplify main method search. > > Jim Laskey 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 12 additional commits since > the last revision: > > - Merge remote-tracking branch 'upstream/master' into 8315458 > - Don't get args unless necessary > - Remove unnamed classes from examples.not-yet.txt > - Requested corrections > - Changes recommended by Jan > - Revised implicit class test > - Don't store main method info globally. Use addition calls to fetch info. > - Update JEP number in PreviewFeature > - Remove MANDATED flag from implicit classes > - Remove .orig files > - ... and 2 more: https://git.openjdk.org/jdk/compare/baf8d59f...0bd5b477 Javac changes look good - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1715227587
Re: RFR: 8319153: Fix: Class is a raw type in ProcessTools
On Tue, 31 Oct 2023 07:43:43 GMT, Leo Korinth wrote: > Changing from `Class c` to `Class c` removes two warnings. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/16431#issuecomment-1794831441
Integrated: 8319153: Fix: Class is a raw type in ProcessTools
On Tue, 31 Oct 2023 07:43:43 GMT, Leo Korinth wrote: > Changing from `Class c` to `Class c` removes two warnings. This pull request has now been integrated. Changeset: 1c2ea1d2 Author:Leo Korinth URL: https://git.openjdk.org/jdk/commit/1c2ea1d27b1895dca3b30073e3516978083dc70a Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8319153: Fix: Class is a raw type in ProcessTools Reviewed-by: dholmes, mli, lmesnik, jpai - PR: https://git.openjdk.org/jdk/pull/16431
Re: RFR: 8319324: FFM: Reformat javadocs [v3]
On Mon, 6 Nov 2023 10:10:41 GMT, Per Minborg wrote: >> This PR proposes to reformat all the JavaDocs for the FFM API. This would >> bring the FFM API docs more in line with the existing Java documentation >> (see below). Occasional drive-by fixes are also included in this PR (such >> as spelling and capitalization). >> >> I am aware this PR will (if approved) make a significant mark in the change >> logs which is regrettable. >> >> Background: >> >> Older classes like `Object` and `List` have a maximum line length of 80 >> characters whereas newer classes like `ScopedValue` have a maximum line >> length of 90 characters. >> >> The FFM API currently has javadoc lines that exceed 135 characters per line. >> It is also customary to use double spaces when starting a new sentence. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Update after review FWIW, I've been sticking to a 120 character limit per line when writing javadoc. Why should we use 90 columns specifically? > It is also customary to use double spaces when starting a new sentence. I thought this was the exception rather than the rule, to be honest. - PR Comment: https://git.openjdk.org/jdk/pull/16518#issuecomment-1794824402
Re: RFR: JDK-8319122: Improve documentation of various Zip-file related APIs
On Mon, 30 Oct 2023 17:26:53 GMT, Yakov Shafranovich wrote: > The various Zip/Jar-file related Java APIs have some long-standing > differences or peculiarities with respect to the ZIP-file specification or > compared to other implementations which should be documented in the API-doc. > This documents the following: > - Cache of JAR files in JarURLConnection class > - Cache of JAR/ZIP files in JarFile and ZipFile classes > - Unexpected behavior when parsing ZIP files with duplicate entries in > JarFile and ZipFile classes, as well as the zipfs provider > - Directories and filenames with the same name considered to be the same in > ZipFile class > - Possible issues when local and central headers conflict in ZipInputStream > class > > Related JBS report: > https://bugs.openjdk.org/browse/JDK-8319122 I will create the CSR once there's agreement on the exact wording of the proposed API-doc changes. - PR Comment: https://git.openjdk.org/jdk/pull/16424#issuecomment-1794822606
Re: RFR: 8319316: Clarify text around which layouts a linker supports [v3]
On Mon, 6 Nov 2023 10:13:14 GMT, Per Minborg wrote: >> Jorn Vernee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add 'strict' >> >> Co-authored-by: Maurizio Cimadamore >> <54672762+mcimadam...@users.noreply.github.com> > > Can we reformat the new improved text so it will fit in col <= 90? @minborg Shouldn't we do that as part of: https://github.com/openjdk/jdk/pull/16518 ? - PR Comment: https://git.openjdk.org/jdk/pull/16485#issuecomment-1794815716
Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v12]
On Fri, 3 Nov 2023 12:40:48 GMT, Jim Laskey wrote: >> Address changes from JEP 445 to JEP 463. >> >> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class. >> >> - Don't mark class on read. >> >> - Remove reflection and annotation processing related to unnamed classes. >> >> - Simplify main method search. > > Jim Laskey 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 12 additional commits since > the last revision: > > - Merge remote-tracking branch 'upstream/master' into 8315458 > - Don't get args unless necessary > - Remove unnamed classes from examples.not-yet.txt > - Requested corrections > - Changes recommended by Jan > - Revised implicit class test > - Don't store main method info globally. Use addition calls to fetch info. > - Update JEP number in PreviewFeature > - Remove MANDATED flag from implicit classes > - Remove .orig files > - ... and 2 more: https://git.openjdk.org/jdk/compare/caa2870d...0bd5b477 javac changes look reasonable to me. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1715097616
Re: RFR: JDK-8315457 Implementation of String Templates (Second Preview) [v4]
On Sat, 4 Nov 2023 13:26:04 GMT, Alan Bateman wrote: >> Jim Laskey has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains six additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'upstream/master' into 8315457 >> - Cache process method type in JCStringTemplate >> - Revert source >> - Revert @since 22 >> - Accept qualified STR and RAW >> - String Templates (second preview) > > src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 424: > >> 422: * @since 21 >> 423: */ >> 424:@PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES) > > I think you can drop both `@since` and `@PreviewFeature` from these methods. > This in an internal interface, used for shared secrets, there is a lot of > churn in JLA in each release. Removed - PR Review Comment: https://git.openjdk.org/jdk/pull/16202#discussion_r1383275177
Re: RFR: JDK-8315457 Implementation of String Templates (Second Preview) [v5]
> Update String Templates for a second preview. With the addition of > > - Expression type and throws are determined from the `process` method of the > processor type and not the processor type. > > - Qualified `STR` and `RAW` are treated the same as unqualified `STR` and > `RAW` . > > - Raw (generic) process types are no longer an error. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Remove preview from JavaLangAccess - Changes: - all: https://git.openjdk.org/jdk/pull/16202/files - new: https://git.openjdk.org/jdk/pull/16202/files/2afadc69..4641ec05 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16202=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16202=03-04 Stats: 8 lines in 1 file changed: 0 ins; 7 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16202.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16202/head:pull/16202 PR: https://git.openjdk.org/jdk/pull/16202
Re: RFR: 8318971: jar v17 should either exit on error immediately or create archive as jar v1.8 did
On Mon, 30 Oct 2023 16:16:52 GMT, Ryan Wallace wrote: > Hi all, > > Please review this fix for jar tool not producing archive if there is a > missing file supplied. Fix is to throw an exception and exit processing when > a missing file is supplied. Current behaviour will recognise missing file as > an error but continue processing and not produce the archive. Updated > ClassPath test to verify jar is not created. > > Thanks, > Ryan. Thanks for checking the history. A plus to going back to JDK 8 behavior is that it make the `@contents` consistent with the file1, file2, file3 case where one/more of the files doesn't exist or a bad file path is specified. It also means that bugs/issues will be detected quicker. So I think it has positives, it's just that it's changing 6+ year old behavior so we have to be sure. - PR Comment: https://git.openjdk.org/jdk/pull/16423#issuecomment-1794678429
Re: RFR: 8319324: FFM: Reformat javadocs [v3]
On Mon, 6 Nov 2023 10:10:41 GMT, Per Minborg wrote: >> This PR proposes to reformat all the JavaDocs for the FFM API. This would >> bring the FFM API docs more in line with the existing Java documentation >> (see below). Occasional drive-by fixes are also included in this PR (such >> as spelling and capitalization). >> >> I am aware this PR will (if approved) make a significant mark in the change >> logs which is regrettable. >> >> Background: >> >> Older classes like `Object` and `List` have a maximum line length of 80 >> characters whereas newer classes like `ScopedValue` have a maximum line >> length of 90 characters. >> >> The FFM API currently has javadoc lines that exceed 135 characters per line. >> It is also customary to use double spaces when starting a new sentence. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Update after review You might also want to look into the MethodHandles XYZCoordinates combinators (as they have also been added as part of FFM). And, also, `ModuleLayer::Controller::enableNativeAccess`, `Module::isNativeAccessEnabled` and `FileChannel::map(... Arena)`. - PR Review: https://git.openjdk.org/jdk/pull/16518#pullrequestreview-1714934553
Re: RFR: 8319423: Improve Year.isLeap by checking divisibility by 16 [v2]
On Fri, 3 Nov 2023 23:22:27 GMT, Claes Redestad wrote:
>> https://github.com/cassioneri/eaf suggest this code for leap year
>> calculation:
>>
>> public static boolean isLeap(long year) {
>> int d = year % 100 != 0 ? 4 : 16;
>> return (year & (d - 1)) == 0;
>> }
>>
>> .. with a claim this would compile down to branchless, easily pipelined code.
>>
>> This doesn't currently happen with C2. In the meantime I think we can
>> improve the current code in `Year.isLeap` and `IsoChronology.isLeapYear` by
>> leveraging the fact that the `% 100` check is only needed if `(year & 15) !=
>> 0`:
>>
>>
>> public static boolean isLeap(long year) {
>> return (year & 15) == 0 ? (year & 3) == 0 : (year & 3) == 0 && year
>> % 100 != 0;
>> }
>>
>>
>> Mac M1:
>>
>> Name Cnt Base Error Test Error Unit
>> Change
>> LeapYearBench.isLeapYear15 0,743 ± 0,009 0,994 ± 0,005 ops/us
>> 1,34x (p = 0,000*)
>> LeapYearBench.isLeapYearChrono 15 0,748 ± 0,006 0,991 ± 0,003 ops/us
>> 1,32x (p = 0,000*)
>> LeapYearBench.isLeapYearNS 15 0,558 ± 0,026 0,552 ± 0,033 ops/us
>> 0,99x (p = 0,602 )
>> * = significant
>>
>>
>> Linux x64:
>>
>> Name Cnt Base Error Test Error Unit
>> Change
>> LeapYearBench.isLeapYear15 0.534 ± 0.001 0.765 ± 0.004 ops/us
>> 1.43x (p = 0.000*)
>> LeapYearBench.isLeapYearChrono 15 0.535 ± 0.000 0.753 ± 0.040 ops/us
>> 1.41x (p = 0.000*)
>> LeapYearBench.isLeapYearNS 15 0.352 ± 0.000 0.351 ± 0.001 ops/us
>> 1.00x (p = 0.000*)
>> * = significant
>>
>>
>> 30% higher throughput on M1, 40% on x64. `isLeapYearNS` runs a variant of
>> the code from https://github.com/cassioneri/eaf ported to java - perhaps the
>> JIT can be improved to do whatever clang/gcc does here and achieve an even
>> better speed-up.
>>
>> Testing: so far only java/time/tck/java/time locally, will run a few tiers
>> before filing an enhancement and opening the PR for review.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply similar optimization to GregorianCalendar, sun.util.calendar
Filed https://bugs.openjdk.org/browse/JDK-8319526 to re-examine the integer
remainder optimization in C2.
-
PR Comment: https://git.openjdk.org/jdk/pull/16491#issuecomment-1794616062
Re: RFR: 8318971: jar v17 should either exit on error immediately or create archive as jar v1.8 did
On Mon, 30 Oct 2023 16:16:52 GMT, Ryan Wallace wrote: > Hi all, > > Please review this fix for jar tool not producing archive if there is a > missing file supplied. Fix is to throw an exception and exit processing when > a missing file is supplied. Current behaviour will recognise missing file as > an error but continue processing and not produce the archive. Updated > ClassPath test to verify jar is not created. > > Thanks, > Ryan. I had a look and its been in since JDK 9 (https://bugs.openjdk.org/browse/JDK-8158295) but haven’t found any mention of this as a specific desired behaviour so I am going with just noticed now. Its not a major blocker as the user can either make sure the missing file is in place or exclude it from being added to the archive and rerun. I agree it will need some thought, I was of the opinion that we should fail upfront and notify the user of why. The JDK 8 behaviour doesn’t make sense as the state is recognised as an error and there are tests to validate this, but they do not validate the archive has not been created, in my opinion we shouldn’t continue on and create it. There are other areas in the jar tool in 8 that will clean up (delete) the archive that has been created if there is an error, this case just seems to fall through the cracks. - PR Comment: https://git.openjdk.org/jdk/pull/16423#issuecomment-1794612971
Re: RFR: 8319462: Signature.ClassTypeSig::classDesc() incorrect for inner class types
On Mon, 6 Nov 2023 06:00:20 GMT, Chen Liang wrote: > Observed this erroneous implementation while browsing, and included a quick > test against javac output class file to confirm the correct implementation. > @asotona Please take a look. Good catch, thanks for the fix. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16513#pullrequestreview-1714726678
Re: RFR: 8319316: Clarify text around which layouts a linker supports [v3]
On Fri, 3 Nov 2023 18:16:17 GMT, Jorn Vernee wrote: >> - Add linker note about packed structs. >> - Relax language a bit to avoid implying that only listed layouts are >> supported. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Add 'strict' > > Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> Can we reformat the new improved text so it will fit in col <= 90? - PR Comment: https://git.openjdk.org/jdk/pull/16485#issuecomment-1794487546
Re: RFR: 8319324: FFM: Reformat javadocs [v3]
> This PR proposes to reformat all the JavaDocs for the FFM API. This would > bring the FFM API docs more in line with the existing Java documentation (see > below). Occasional drive-by fixes are also included in this PR (such as > spelling and capitalization). > > I am aware this PR will (if approved) make a significant mark in the change > logs which is regrettable. > > Background: > > Older classes like `Object` and `List` have a maximum line length of 80 > characters whereas newer classes like `ScopedValue` have a maximum line > length of 90 characters. > > The FFM API currently has javadoc lines that exceed 135 characters per line. > It is also customary to use double spaces when starting a new sentence. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Update after review - Changes: - all: https://git.openjdk.org/jdk/pull/16518/files - new: https://git.openjdk.org/jdk/pull/16518/files/d0fddb0f..0c379b4a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16518=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16518=01-02 Stats: 15 lines in 2 files changed: 10 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16518/head:pull/16518 PR: https://git.openjdk.org/jdk/pull/16518
Re: RFR: 8318971: jar v17 should either exit on error immediately or create archive as jar v1.8 did
On Mon, 30 Oct 2023 16:16:52 GMT, Ryan Wallace wrote: > Hi all, > > Please review this fix for jar tool not producing archive if there is a > missing file supplied. Fix is to throw an exception and exit processing when > a missing file is supplied. Current behaviour will recognise missing file as > an error but continue processing and not produce the archive. Updated > ClassPath test to verify jar is not created. > > Thanks, > Ryan. This one probably needs discussion to decide if the JDK 8 or current behavior is the right behavior. Once agreed then we will need to add tests to ensure that the behavior doesn't change, and maybe an update to docs too. Have you dug into the history to see if this behavior change was deliberate or just not noticed before now? - PR Comment: https://git.openjdk.org/jdk/pull/16423#issuecomment-1794474588
Re: RFR: 8319423: Improve Year.isLeap by checking divisibility by 16 [v2]
On Mon, 6 Nov 2023 02:34:21 GMT, Quan Anh Mai wrote: > > > > I have filed > [JDK-8319451](https://bugs.openjdk.org/projects/JDK/issues/JDK-8319451). I > would suggest waiting for this bug to be resolved before proceeding with this > PR. Nice analysis! While I'm sure we need to re-evaluate this enhancement after JDK-8319451 is resolved, I'm not a fan of blocking library enhancements on improvements to the runtime/compiler (as it's impossible to know up front if this is something we can fix in the next couple of days/weeks, or need to staff, plan and evaluate over a longer cycle). I included the Neri-Schneider variant in the microbenchmark here to make it easy to assess if an optimization such as JDK-8319451 would turn things around. We should file an enhancement to re-visit the Granlund & Montgomery/Hacker's delight division. Improving this should benefit either variant, and might be needed together with JDK-8319451 for the `isLeapYearNS` test to win. - PR Comment: https://git.openjdk.org/jdk/pull/16491#issuecomment-1794464999
RFR: 8318971: jar v17 should either exit on error immediately or create archive as jar v1.8 did
Hi all, Please review this fix for jar tool not producing archive if there is a missing file supplied. Fix is to throw an exception and exit processing when a missing file is supplied. Current behaviour will recognise missing file as an error but continue processing and not produce the archive. Updated ClassPath test to verify jar is not created. Thanks, Ryan. - Commit messages: - 8318971: jar v17 should either exit on error immediately or create archive as jar v1.8 did - 8318971: jar v17 should either exit on error immediately or create archive as jar v1.8 did Changes: https://git.openjdk.org/jdk/pull/16423/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16423=00 Issue: https://bugs.openjdk.org/browse/JDK-8318971 Stats: 8 lines in 2 files changed: 5 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16423.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16423/head:pull/16423 PR: https://git.openjdk.org/jdk/pull/16423
Re: RFR: 8319324: FFM: Reformat javadocs [v2]
On Mon, 6 Nov 2023 08:08:34 GMT, Per Minborg wrote:
>> This PR proposes to reformat all the JavaDocs for the FFM API. This would
>> bring the FFM API docs more in line with the existing Java documentation
>> (see below). Occasional drive-by fixes are also included in this PR (such
>> as spelling and capitalization).
>>
>> I am aware this PR will (if approved) make a significant mark in the change
>> logs which is regrettable.
>>
>> Background:
>>
>> Older classes like `Object` and `List` have a maximum line length of 80
>> characters whereas newer classes like `ScopedValue` have a maximum line
>> length of 90 characters.
>>
>> The FFM API currently has javadoc lines that exceed 135 characters per line.
>> It is also customary to use double spaces when starting a new sentence.
>
> Per Minborg has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Merge master
> - Reformat javadocs
> - FFM: Harmonize the @throws tags in the javadocs
> - Merge branch 'master' into javadoc-throws
> - Harmonize some of the javadoc throws
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1910:
> 1908: * in the provided layout
> 1909: * @throws IndexOutOfBoundsException if {@code offset > byteSize()
> - layout.byteSize()}
> 1910: * @throws UnsupportedOperationException if this segment is
one `UOE` went missing
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16518#discussion_r1382966088
Re: RFR: 8319324: FFM: Reformat javadocs [v2]
On Mon, 6 Nov 2023 08:08:34 GMT, Per Minborg wrote: >> This PR proposes to reformat all the JavaDocs for the FFM API. This would >> bring the FFM API docs more in line with the existing Java documentation >> (see below). Occasional drive-by fixes are also included in this PR (such >> as spelling and capitalization). >> >> I am aware this PR will (if approved) make a significant mark in the change >> logs which is regrettable. >> >> Background: >> >> Older classes like `Object` and `List` have a maximum line length of 80 >> characters whereas newer classes like `ScopedValue` have a maximum line >> length of 90 characters. >> >> The FFM API currently has javadoc lines that exceed 135 characters per line. >> It is also customary to use double spaces when starting a new sentence. > > Per Minborg has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Merge master > - Reformat javadocs > - FFM: Harmonize the @throws tags in the javadocs > - Merge branch 'master' into javadoc-throws > - Harmonize some of the javadoc throws Generally looks good (I've been relying a lot on github diff annotations, so I hope those are accurate :-) ). There seem to be a couple of issues with deleted text. - PR Review: https://git.openjdk.org/jdk/pull/16518#pullrequestreview-1714576629
Re: RFR: 8319324: FFM: Reformat javadocs [v2]
On Mon, 6 Nov 2023 08:53:55 GMT, Maurizio Cimadamore wrote: >> Per Minborg has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Merge master >> - Reformat javadocs >> - FFM: Harmonize the @throws tags in the javadocs >> - Merge branch 'master' into javadoc-throws >> - Harmonize some of the javadoc throws > > src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1630: > >> 1628: * Writes a boolean into this segment at the given offset, with >> the given layout. >> 1629: * >> 1630: * @param offset offset in bytes (relative to this segment >> address) at which this > > The first `@param` (layout) here has been deleted? Not just here - happened elsewhere too - PR Review Comment: https://git.openjdk.org/jdk/pull/16518#discussion_r1382956428
Re: RFR: 8319324: FFM: Reformat javadocs [v2]
On Mon, 6 Nov 2023 08:08:34 GMT, Per Minborg wrote: >> This PR proposes to reformat all the JavaDocs for the FFM API. This would >> bring the FFM API docs more in line with the existing Java documentation >> (see below). Occasional drive-by fixes are also included in this PR (such >> as spelling and capitalization). >> >> I am aware this PR will (if approved) make a significant mark in the change >> logs which is regrettable. >> >> Background: >> >> Older classes like `Object` and `List` have a maximum line length of 80 >> characters whereas newer classes like `ScopedValue` have a maximum line >> length of 90 characters. >> >> The FFM API currently has javadoc lines that exceed 135 characters per line. >> It is also customary to use double spaces when starting a new sentence. > > Per Minborg has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Merge master > - Reformat javadocs > - FFM: Harmonize the @throws tags in the javadocs > - Merge branch 'master' into javadoc-throws > - Harmonize some of the javadoc throws src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1630: > 1628: * Writes a boolean into this segment at the given offset, with the > given layout. > 1629: * > 1630: * @param offset offset in bytes (relative to this segment address) > at which this The first `@param` (layout) here has been deleted? - PR Review Comment: https://git.openjdk.org/jdk/pull/16518#discussion_r1382955037
Re: RFR: 8316641: VarHandle template classes can share code in the base class [v8]
> VarHandle implementations have many static fields and methods that can be > pulled to the common superclass to avoid repeated initialization and code > duplication. > > In addition, the Unsafe-based Buffer field access are replaced by usage of > public methods or JavaNioAccess. Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge branch 'master' of https://github.com/openjdk/jdk into cleanup/vh-template-share - 8316641: VarHandle template classes can share code in the base class - Changes: https://git.openjdk.org/jdk/pull/15854/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15854=07 Stats: 200 lines in 7 files changed: 57 ins; 79 del; 64 mod Patch: https://git.openjdk.org/jdk/pull/15854.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15854/head:pull/15854 PR: https://git.openjdk.org/jdk/pull/15854
Re: RFR: 8319324: FFM: Reformat javadocs [v2]
> This PR proposes to reformat all the JavaDocs for the FFM API. This would > bring the FFM API docs more in line with the existing Java documentation (see > below). Occasional drive-by fixes are also included in this PR (such as > spelling and capitalization). > > I am aware this PR will (if approved) make a significant mark in the change > logs which is regrettable. > > Background: > > Older classes like `Object` and `List` have a maximum line length of 80 > characters whereas newer classes like `ScopedValue` have a maximum line > length of 90 characters. > > The FFM API currently has javadoc lines that exceed 135 characters per line. > It is also customary to use double spaces when starting a new sentence. Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge master - Reformat javadocs - FFM: Harmonize the @throws tags in the javadocs - Merge branch 'master' into javadoc-throws - Harmonize some of the javadoc throws - Changes: - all: https://git.openjdk.org/jdk/pull/16518/files - new: https://git.openjdk.org/jdk/pull/16518/files/5da69f89..d0fddb0f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16518=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16518=00-01 Stats: 7264 lines in 172 files changed: 2735 ins; 2345 del; 2184 mod Patch: https://git.openjdk.org/jdk/pull/16518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16518/head:pull/16518 PR: https://git.openjdk.org/jdk/pull/16518
