Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v18]
> jdk.jlink internal plugins are heavily using ASM > > This patch converts ASM calls to Classfile API. > > Please review. > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: improved exception message as suggested - Changes: - all: https://git.openjdk.org/jdk/pull/12944/files - new: https://git.openjdk.org/jdk/pull/12944/files/c1d5a77a..2af22a27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12944&range=17 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12944&range=16-17 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12944.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12944/head:pull/12944 PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v14]
On Tue, 21 Mar 2023 11:54:38 GMT, Adam Sotona wrote: >> Yes, I think so. If `java -XX:+UnlockDiagnosticVMOptions >> -XX:+BytecodeVerificationLocal -version` doesn't fail and you've run all the >> tests then it should be okay to drop it. > > I can confirm `java -XX:+UnlockDiagnosticVMOptions > -XX:+BytecodeVerificationLocal -version` successful execution and passing all > jdk/tools/jlink tests. Now waiting for all tier1 tests results. all tier1, 2, and 3 tests passed - PR Review Comment: https://git.openjdk.org/jdk/pull/12944#discussion_r1144284079
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
On Tue, 21 Mar 2023 15:08:33 GMT, Viktor Klang wrote: >> If there isn't any value running this test with a debug build then you could >> have it skip (`@requires vm.debug == false`) to save resources. >> (fixed typo in original message) > > @AlanBateman Wouldn't that be `@requires vm.debug == false` ? Hello Viktor, > @jaikiran Having a long timeout doesn't seem like a problem given that it > just needs enough time to run through the iterations (i.e. that's the max > duration of the test before giving up). Having a extremely long timeout of T1 secods for a test which one can (reasonably) expect to finish in (T1 - X) seconds can mean that when/if it genuinely times out, then it holds on to the limited shared resources (like the host machine) for those X seconds longer instead of potentially letting other tests run during that period. So I think it's always better to have a reasonable timeout instead of an extremely large one - a value past which you know that if the test is still running then it's surely be a sign that it should no longer continue to run. Typically the timeout for such tests is decided by running the test against the hosts/environment where it failed and gathering data to see how long it usually takes to finish successfully on those and then adding some extra seconds to it. - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1144281676
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v17]
> jdk.jlink internal plugins are heavily using ASM > > This patch converts ASM calls to Classfile API. > > Please review. > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/12944/files - new: https://git.openjdk.org/jdk/pull/12944/files/3c12e88c..c1d5a77a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12944&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12944&range=15-16 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12944.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12944/head:pull/12944 PR: https://git.openjdk.org/jdk/pull/12944
Integrated: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved
On Mon, 20 Mar 2023 11:15:52 GMT, Adam Sotona wrote: > Classfile API class hierarchy makes assumptions when class is not resolved > and that may lead to silent generation of invalid stack maps. Only > debug-level log information of case is actually provided. > > Proposed patch throws IllegalArgumentException when the class is not resolved > instead. > > Thanks for review. > > Adam This pull request has now been integrated. Changeset: 0156909a Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/0156909ab38072869e2eb9f5049042b9199d14a0 Stats: 21 lines in 4 files changed: 8 ins; 8 del; 5 mod 8304502: Classfile API class hierarchy makes assumptions when class is not resolved Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/13099
Re: RFR: 8304031: Classfile API cannot encode Primitive Class as Condy
On Mon, 13 Mar 2023 08:13:44 GMT, Chen Liang wrote: > Without this patch, the Classfile API tries to encode PrimitiveClassDesc as > CONSTANT_Class_info and error in `toInternalName`. Seems this bug exists in `ConstantPoolBuilder::constantValueEntry` as well. - PR Comment: https://git.openjdk.org/jdk/pull/12996#issuecomment-1478962553
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v12]
On Fri, 17 Mar 2023 17:01:43 GMT, Adam Sotona wrote: >> jdk.jlink internal plugins are heavily using ASM >> >> This patch converts ASM calls to Classfile API. >> >> Please review. >> Thanks, >> Adam > > Adam Sotona has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 220 commits: > > - Merge branch 'master' into JDK-8294972-jlink-plugins > - SystemModulesPlugin::genClassBytes rename > - SystemModulesPlugin::getClassBytes rename > - Revert "implementation of custom ResourceHelper in IncludeLocalesPlugin" > - remaining cleanup in SystemModulesPlugin > - implementation of custom ResourceHelper in IncludeLocalesPlugin > - fixed SystemModulesPlugin > - fixed StripJavaDebugAttribute to drop line numbers from code > - long lines wrapped > - StripJavaDebugAttributesPlugin transformation fixed > - ... and 210 more: https://git.openjdk.org/jdk/compare/4486f1b7...4e5b9651 src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 112: > 110: private static final String DEFAULT_SYSTEM_MODULES_CLASSNAME = > 111: SYSTEM_MODULES_CLASS_PREFIX + "default"; > 112: private static final ClassDesc CD_SYSTEM_MODULES = Suggestion: private static final ClassDesc CD_ALL_SYSTEM_MODULES = ClassDesc.ofInternalName(ALL_SYSTEM_MODULES_CLASSNAME); private static final ClassDesc CD_SYSTEM_MODULES = src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java line 272: > 270: > 271: // generate SystemModulesMap > 272: rn = > genSystemModulesMapClass(ClassDesc.ofInternalName(ALL_SYSTEM_MODULES_CLASSNAME), `ClassDesc.ofInternalName(ALL_SYSTEM_MODULES_CLASSNAME)` should probably be made into a constant. Suggestion: rn = genSystemModulesMapClass(CD_ALL_SYSTEM_MODULES, - PR Review Comment: https://git.openjdk.org/jdk/pull/12944#discussion_r1141018698 PR Review Comment: https://git.openjdk.org/jdk/pull/12944#discussion_r1141018455
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v12]
On Fri, 17 Mar 2023 17:30:41 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 220 commits:
>>
>> - Merge branch 'master' into JDK-8294972-jlink-plugins
>> - SystemModulesPlugin::genClassBytes rename
>> - SystemModulesPlugin::getClassBytes rename
>> - Revert "implementation of custom ResourceHelper in IncludeLocalesPlugin"
>> - remaining cleanup in SystemModulesPlugin
>> - implementation of custom ResourceHelper in IncludeLocalesPlugin
>> - fixed SystemModulesPlugin
>> - fixed StripJavaDebugAttribute to drop line numbers from code
>> - long lines wrapped
>> - StripJavaDebugAttributesPlugin transformation fixed
>> - ... and 210 more: https://git.openjdk.org/jdk/compare/4486f1b7...4e5b9651
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/IncludeLocalesPlugin.java
> line 309:
>
>> 307: TAG_INVOKEDYNAMIC -> offset += 5;
>> 308: case TAG_LONG,
>> 309: TAG_DOUBLE -> {offset += 9; cpSlot++;}
>> //additional slot for double and long entries
>
> should this add the default case and throw to prepare for future change?
That’s definitely better than breaking silently.
Suggestion:
TAG_DOUBLE -> {offset += 9; cpSlot++;} //additional slot
for double and long entries
default -> throw new IllegalArgumentException("Unknown constant
pool entry: 0x" +
Integer.toHexString(Byte.toUnsignedInt(bytes[offset])).toUpperCase(Locale.ROOT));
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12944#discussion_r1141017881
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote:
>> Currently, the two subclasses of `java.util.EnumSet` optimize bulk
>> operations when the argument is also a `EnumSet`, but there is no such
>> optimization for wrapper sets (returned by `Collections.unmodifiableSet`,
>> `Collections.synchronizedSet`, etc.) and immutable sets (returned by
>> `Set.of` methods) of `Enum`s.
>>
>> This PR introduces optimization classes for these situations. No public
>> APIs are changed.
>
> Tingjun Yuan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Set.copyOf: need defensive copy
I have deleted `{Regular,Jumbo}EnumSetCompatible` interfaces and introduced
some package-private methods in `AbstractCollection`. This avoids rarely-used
checks from static factories in `Collections` and does not reduce much
performance compared against the previous implementation.
-
PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478790594
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v7]
> Currently, the two subclasses of `java.util.EnumSet` optimize bulk operations > when the argument is also a `EnumSet`, but there is no such optimization for > wrapper sets (returned by `Collections.unmodifiableSet`, > `Collections.synchronizedSet`, etc.) and immutable sets (returned by `Set.of` > methods) of `Enum`s. > > This PR introduces optimization classes for these situations. No public APIs > are changed. Tingjun Yuan has updated the pull request incrementally with one additional commit since the last revision: Fix a whitespace error - Changes: - all: https://git.openjdk.org/jdk/pull/12498/files - new: https://git.openjdk.org/jdk/pull/12498/files/c02ca488..4a7299b3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12498.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12498/head:pull/12498 PR: https://git.openjdk.org/jdk/pull/12498
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v6]
> Currently, the two subclasses of `java.util.EnumSet` optimize bulk operations
> when the argument is also a `EnumSet`, but there is no such optimization for
> wrapper sets (returned by `Collections.unmodifiableSet`,
> `Collections.synchronizedSet`, etc.) and immutable sets (returned by `Set.of`
> methods) of `Enum`s.
>
> This PR introduces optimization classes for these situations. No public APIs
> are changed.
Tingjun Yuan has updated the pull request incrementally with one additional
commit since the last revision:
Delete compatible interfaces and introduce some methods
I have deleted `{Regular,Jumbo}EnumSetCompatible` interfaces and introduced
some package-private methods in `AbstractCollection`. This avoids rarely-used
checks from static factories in `Collections` and does not reduce much
performance compared against the previous implementation.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12498/files
- new: https://git.openjdk.org/jdk/pull/12498/files/0e0b2bf1..c02ca488
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=04-05
Stats: 392 lines in 9 files changed: 65 ins; 218 del; 109 mod
Patch: https://git.openjdk.org/jdk/pull/12498.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/12498/head:pull/12498
PR: https://git.openjdk.org/jdk/pull/12498
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote: >> Currently, the two subclasses of `java.util.EnumSet` optimize bulk >> operations when the argument is also a `EnumSet`, but there is no such >> optimization for wrapper sets (returned by `Collections.unmodifiableSet`, >> `Collections.synchronizedSet`, etc.) and immutable sets (returned by >> `Set.of` methods) of `Enum`s. >> >> This PR introduces optimization classes for these situations. No public >> APIs are changed. > > Tingjun Yuan has updated the pull request incrementally with one additional > commit since the last revision: > > Set.copyOf: need defensive copy If this level of complexity is indeed needed to get whatever improvement you're after then I don't see how this can be worth its weight. Microbenchmarking might help support your case here, but assessing the potential performance costs from gradually increasing the number of classes floating around at various call sites in arbitrary applications is hard. Thus it is something we need to be very careful not to do without solid evidence. - PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478731354
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote:
>> Currently, the two subclasses of `java.util.EnumSet` optimize bulk
>> operations when the argument is also a `EnumSet`, but there is no such
>> optimization for wrapper sets (returned by `Collections.unmodifiableSet`,
>> `Collections.synchronizedSet`, etc.) and immutable sets (returned by
>> `Set.of` methods) of `Enum`s.
>>
>> This PR introduces optimization classes for these situations. No public
>> APIs are changed.
>
> Tingjun Yuan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Set.copyOf: need defensive copy
Also, in JEP 441, there is pattern matching support for different enums
implementing one interface in one switch, see the enum constants section there.
I wonder how useful this is, and how often do we have interfaces with more than
one enum implementations (e.g. StandardOptions/ExtendedOptions)
src/java.base/share/classes/java/util/ImmutableCollections.java line 1110:
> 1108: static final JavaLangAccess JLA =
> SharedSecrets.getJavaLangAccess();
> 1109:
> 1110: @Stable
Useless
src/java.base/share/classes/java/util/ImmutableCollections.java line 1174:
> 1172: static final class ImmutableRegularEnumSet>
> extends AbstractImmutableEnumSet
> 1173: implements RegularEnumSetCompatible, Serializable {
> 1174: @Stable
Useless
src/java.base/share/classes/java/util/ImmutableCollections.java line 1267:
> 1265: final long[] elements;
> 1266:
> 1267: @Stable
Useless
-
PR Review: https://git.openjdk.org/jdk/pull/12498#pullrequestreview-1351490416
PR Review Comment: https://git.openjdk.org/jdk/pull/12498#discussion_r1144062917
PR Review Comment: https://git.openjdk.org/jdk/pull/12498#discussion_r1144064197
PR Review Comment: https://git.openjdk.org/jdk/pull/12498#discussion_r1144062709
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote:
>> Currently, the two subclasses of `java.util.EnumSet` optimize bulk
>> operations when the argument is also a `EnumSet`, but there is no such
>> optimization for wrapper sets (returned by `Collections.unmodifiableSet`,
>> `Collections.synchronizedSet`, etc.) and immutable sets (returned by
>> `Set.of` methods) of `Enum`s.
>>
>> This PR introduces optimization classes for these situations. No public
>> APIs are changed.
>
> Tingjun Yuan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Set.copyOf: need defensive copy
The `RandomAccess` `SortedSet` `NaviagableSet` implementations are public to
users while `RegularEnumSetCompatible` and `JumboEnumSetCompatible` aren't. So
I guess we can implement another interface for these wrappers to retrieve their
backing instances, like:
interface WrapperCollection {
Collection getBacking();
}
and thus:
interface RegularEnumSetCompatible {
static RegularEnumSetCompatible tryConvert(Collection coll) {
if (coll instanceof RegularEnumSetCompatible compat) return compat;
if (coll instanceof WrapperCollection wrap) return
tryConvert(wrap.getBacking());
return null;
}
}
Adding extra `getClass() == UnmodifiableRegularEnumSet.class` indeed will
affect most users, which rarely are enum set compatibles.
-
PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478689026
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote:
>> Currently, the two subclasses of `java.util.EnumSet` optimize bulk
>> operations when the argument is also a `EnumSet`, but there is no such
>> optimization for wrapper sets (returned by `Collections.unmodifiableSet`,
>> `Collections.synchronizedSet`, etc.) and immutable sets (returned by
>> `Set.of` methods) of `Enum`s.
>>
>> This PR introduces optimization classes for these situations. No public
>> APIs are changed.
>
> Tingjun Yuan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Set.copyOf: need defensive copy
Thanks again for reviewing. Correct me if I'm wrong.
@cl4es I need to add these new wrapper classes
`{Unmodifiable,Synchronized}{Regular,Jumbo}EnumSet` to implement interfaces
`{Regular,Jumbo}EnumSetCompatible`, just like `*RandomAccessList` which appear
to implement `RandomAccess`. I don't think removing these wrapper classes and
introducing accessors is a good idea, because I will have to check for the
wrapper classes in bulk operation methods, which are used more frequently.
@pavelrappo `{Unmodifiable,Sychronized}Set` also have subclasses implementing
`SortedSet` and `NavigableSet`, I'm not sure whether `instanceof` is safe for
these subclasses.
-
PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478658361
Re: RFR: 8294977: Convert test/jdk/java tests from ASM library to Classfile API [v7]
> Summaries: > 1. A few recommendations about updating the constant API is made at > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000233.html > and I may update this patch shall the API changes be integrated before > 2. One ASM library-specific test, `LambdaAsm` is removed. Others have their > code generation infrastructure upgraded from ASM to Classfile API. > 3. Most tests are included in tier1, but some are not: > In `:jdk_io`: (tier2, part 2) > > test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java > test/jdk/java/io/Serializable/records/ProhibitedMethods.java > test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java > > In `:jdk_instrument`: (tier 3) > > test/jdk/java/lang/instrument/RetransformAgent.java > test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java > test/jdk/java/lang/instrument/asmlib/Instrumentor.java > > > @asotona Would you mind reviewing? Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Switch to ConstantDescs for and void constants - Merge AnnotationsTest, remove ModuleTargetAttribute call - Merge branch 'invoke-test-classfile' of https://github.com/liachmodded/jdk into invoke-test-classfile - Update test/jdk/java/lang/invoke/8022701/MHIllegalAccess.java Co-authored-by: Andrey Turbanov - Merge branch 'master' into invoke-test-classfile - Fix LambdaStackTrace after running - formatting - Fix failed LambdaStackTrace test, use more convenient APIs - Merge branch 'master' of https://git.openjdk.java.net/jdk into invoke-test-classfile - Shorten lines, move from mask() to ACC_ constants, other misc improvements - ... and 1 more: https://git.openjdk.org/jdk/compare/0deb6489...7dc785b3 - Changes: https://git.openjdk.org/jdk/pull/13009/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13009&range=06 Stats: 1946 lines in 31 files changed: 302 ins; 889 del; 755 mod Patch: https://git.openjdk.org/jdk/pull/13009.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13009/head:pull/13009 PR: https://git.openjdk.org/jdk/pull/13009
Re: RFR: 8304691: Remove jlink --post-process-path option
On Tue, 21 Mar 2023 20:39:38 GMT, Ian Graves wrote: > Removing the hidden `jlink --post-process-path` option. The localized resource bundles for other languages are updated together in bulk toward the end of the release. `plugins_xx.properties` can be kept unchanged and let them updated by the L10N team in bulk. - PR Comment: https://git.openjdk.org/jdk/pull/13126#issuecomment-1478607924
Re: RFR: 8304691: Remove jlink --post-process-path option
On Tue, 21 Mar 2023 20:39:38 GMT, Ian Graves wrote: > Removing the hidden `jlink --post-process-path` option. I expect `JlinkTask::postProcessImage` and `Jlink::postProcess` to be removed. - PR Review: https://git.openjdk.org/jdk/pull/13126#pullrequestreview-1351401366
RFR: 8304691: Remove jlink --post-process-path option
Removing the hidden `jlink --post-process-path` option. - Commit messages: - Removing Post Processing functionality Changes: https://git.openjdk.org/jdk/pull/13126/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13126&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8304691 Stats: 193 lines in 8 files changed: 0 ins; 193 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13126.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13126/head:pull/13126 PR: https://git.openjdk.org/jdk/pull/13126
Re: RFR: 8300979: Lazily initialize (byte, char)arr in java.io.DataInputStream
On Tue, 21 Mar 2023 15:41:13 GMT, Per Minborg wrote: > This PR proposed to lazily initialize the scratch arrays only if/when needed. Nice idea. Just wonder could we do the same for `BufferedInputStream`? - PR Comment: https://git.openjdk.org/jdk/pull/13121#issuecomment-1478546070
Re: RFR: 8303930: Fix ConstantUtils.skipOverFieldSignature void case return value [v3]
> This typo doesn't allow creation of malformed ClassDesc or MethodTypeDesc,
> but it produces an erroneous exception on certain inputs. Running
> `java.lang.constant.MethodTypeDesc.ofDescriptor("(I[[[V)I")` in Jshell
> 19.0.2 throws StringIndexOutOfBoundsException, and throws
> IllegalArgumentException in the Jshell for this patch.
Chen Liang has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains five additional commits since
the last revision:
- Merge branch 'master' of https://git.openjdk.java.net/jdk into 8303930
- Add test for skipOverFieldSignature bug
- Merge branch 'master' of https://git.openjdk.java.net/jdk into 8303930
- Copyright year
- 8303930: Fix ConstantUtils.skipOverFieldSignature void case return value
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12964/files
- new: https://git.openjdk.org/jdk/pull/12964/files/d7b38bcf..f53269ca
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12964&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12964&range=01-02
Stats: 82754 lines in 1011 files changed: 50645 ins; 22368 del; 9741 mod
Patch: https://git.openjdk.org/jdk/pull/12964.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/12964/head:pull/12964
PR: https://git.openjdk.org/jdk/pull/12964
Re: RFR: 8304006: jlink should create the jimage file in the native endian for the target platform [v10]
On Tue, 21 Mar 2023 18:38:06 GMT, Mandy Chung wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Alan's suggestions - don't parse arch out of osname-arch for determining >> endianness and reduce the number of supported/known target platforms for >> cross linking > > src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Platform.java line 192: > >> 190: case "s390x" -> Architecture.s390x; >> 191: case "sparc" -> Architecture.SPARC; >> 192: case "sparcv9" -> Architecture.SPARCv9; > > This list should be trimmed to the ones supported in the mainline as in > `target.properties`. Also `is64Bit` will need update for other 64-bit platforms. - PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143865820
Re: RFR: 8304006: jlink should create the jimage file in the native endian for the target platform [v10]
On Mon, 20 Mar 2023 14:21:55 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this change which proposes to fix the issue
>> reported in https://bugs.openjdk.org/browse/JDK-8206890?
>>
>> The `jlink` command allows a `--endian` option to specify the byte order in
>> the generated image. Before this change, when such a image was being
>> launched, the code would assume the byte order in the image to be the native
>> order of the host where the image is being launched. That would result in
>> failure to launch java, as noted in the linked issue.
>>
>> The commit in this PR, changes relevant places to not assume native order
>> and instead determine the byte order by reading the magic bytes in the image
>> file's header content.
>>
>> A new jtreg test has been added which reproduces the issue and verifies the
>> fix.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Alan's suggestions - don't parse arch out of osname-arch for determining
> endianness and reduce the number of supported/known target platforms for
> cross linking
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 548:
> 546: Path
> retainModulesPath,
> 547: boolean
> ignoreSigning,
> 548: boolean bindService,
formatting nit: align the parameters with the first one.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 822:
> 820: PrintWriter log) throws IOException {
> 821: if (order != null) {
> 822: this.targetPlatform = Platform.runtime();
The target platform should always be fetched from `java.base` (as it was
implemented in `DefaultImageBuilder`) or use the current runtime if `java.base`
is from the module path of the current runtime.
If `--endian ` is specified, jlink can check if the target platform's
endianness matches the specified value and emit an error if mismatch unless
`Platform::getNativeByteOrder` is null.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 856:
> 854: // unsupported target platform
> 855: throw new IOException(
> 856:
> taskHelper.getMessage("err.unsupported.target.platform",
IIUC, the current jlink implementation does not fail if the target platform is
unknown as long as `--endian` option is specified. So I think `--endian` may
be useful to keep (rather than taking it out). This error key should be
`err.unknown.target.endianness` and the message can suggest to use `--endian`
option to specify.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Platform.java line 192:
> 190: case "s390x" -> Architecture.s390x;
> 191: case "sparc" -> Architecture.SPARC;
> 192: case "sparcv9" -> Architecture.SPARCv9;
This list should be trimmed to the ones supported in the mainline as in
`target.properties`.
src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 151:
> 149: err.signing=signed modular JAR {0} is currently not supported,\
> 150: \ use --ignore-signing-information to suppress error
> 151: err.cannot.determine.target.platform=cannot determine target platform
> for image generation
Suggestion:
err.cannot.determine.target.platform=cannot determine target platform from {0}
`{0}` can be the path to `java.base.jmod` (i.e. `ModuleReference::location`)
src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 152:
> 150: \ use --ignore-signing-information to suppress error
> 151: err.cannot.determine.target.platform=cannot determine target platform
> for image generation
> 152: err.unsupported.target.platform=image generation for target platform {0}
> is not supported
Suggestion:
err.unsupported.target.endianness=Unknown native byte order for target platform
{0}`
`{0}` is `targetPlatformVal`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143813371
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143823833
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143872844
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143847888
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143875777
PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1143877925
Re: RFR: 8303923: ZipOutStream::putEntry should include an apiNote to indicate that the STORED compression method should be used when writing directory entries [v3]
On Tue, 21 Mar 2023 18:21:31 GMT, Lance Andersen wrote: > Thank you for making the changes. I think this looks better. > > I go back and forth as to whether to include the sentence regarding > performance but I think it is OK. Let's see if anyone else has thoughts prior > to finalizing the CSR with this change I understand one can look at this differently. For me, winning friends and influencing people has always seemed easier when I provide an answer for 'What's in it for me?'. Here, I wanted to flip the reader's state of mind from 'hmm!?' to 'hah!'. - PR Comment: https://git.openjdk.org/jdk/pull/12899#issuecomment-1478398916
Re: RFR: 8303923: ZipOutStream::putEntry should include an apiNote to indicate that the STORED compression method should be used when writing directory entries [v3]
On Sun, 19 Mar 2023 14:50:41 GMT, Eirik Bjorsnos wrote: >> ZipOutputStream currently writes directory entries using the DEFLATED >> compression method. This does not strictly comply with the APPNOTE.TXT >> specification and is also about 10x slower than using the STORED compression >> method. >> >> Because of these concerns, `ZipOutputStream.putNextEntry` should be updated >> with an `@apiNote` recommending >> the use of the STORED compression method for directory entries. >> >> Suggested CSR in the first comment. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Remove speculation that using DEFLATED for directory entries is not in > strict compliance with APPNOTE.txt Thank you for making the changes. I think this looks better. I go back and forth as to whether to include the sentence regarding performance but I think it is OK. Let's see if anyone else has thoughts prior to finalizing the CSR with this change - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12899#pullrequestreview-1351148686
Re: RFR: 8304450: [vectorapi] Refactor VectorShuffle implementation [v2]
On Tue, 21 Mar 2023 16:29:44 GMT, Paul Sandoz wrote:
>> I have moved most of the methods to `AbstractVector` and `AbstractShuffle`,
>> I have to resort to raw types, though, since there seems to be no way to do
>> the same with wild cards, and the generics mechanism is not powerful enough
>> for things like `Vector`. The remaining failure seems to be
>> related to
>> [JDK-8304676](https://bugs.openjdk.org/projects/JDK/issues/JDK-8304676), so
>> I think this patch is ready for review now.
>>
>>> The mask implementation is specialized by the species of vectors it
>>> operates on, but does it have to be
>>
>> Apart from the mask implementation, shuffle implementation definitely has to
>> take into consideration the element type. However, this information does not
>> have to be visible to the API, similar to how we currently handle the vector
>> length, we can have `class AbstractMask implements VectorMask`. As a
>> result, the cast method would be useless and can be removed in the API, but
>> our implementation details would still use it, for example
>>
>> Vector blend(Vector v, VectorMask w) {
>> AbstractMask aw = (AbstractMask) w;
>> AbstractMask tw = aw.cast(vspecies());
>> return VectorSupport.blend(...);
>> }
>>
>> Vector rearrange(VectorShuffle s) {
>> AbstractShuffle as = (AbstractShuffle) s;
>> AbstractShuffle ts = s.cast(vspecies());
>> return VectorSupport.rearrangeOp(...);
>> }
>>
>> What do you think?
>
>> Apart from the mask implementation, shuffle implementation definitely has to
>> take into consideration the element type.
>
> Yes, the way you have implemented shuffle is tightly connected, that looks ok.
>
> I am wondering if we can make the mask implementation more loosely coupled
> and modified such that it does not have to take into consideration the
> element type (or species) of the vector it operates on, and instead
> compatibility is based solely on the lane count.
>
> Ideally it would be good to change the `VectorMask::check` method to just
> compare the lanes counts and not require a cast in the implementation, which
> i presume requires some deeper changes in C2?
>
> What you propose seems a possible a interim step towards a more preferable
> API, if the performance is good.
@PaulSandoz As some hardware does differentiate masks based on element type, at
some point we have to differentiate between them. From a design point of view,
they are both implementation details so there might be no consideration
regarding the API. On the other hand, having more in the Java side seems to be
more desirable, as it does illustrate the operations more intuitively compared
to the graph management in C2. Another important point I can think of is that
having a constant shape for a Java class would help us in implementing the
vector calling convention, as we can rely on the class information instead of
some side channels. As a result, I think I do prefer the current class
hierarchy.
-
PR Comment: https://git.openjdk.org/jdk/pull/13093#issuecomment-1478374992
Re: RFR: 8297605: DelayQueue javadoc is confusing [v2]
> Inviting @DougLea and @viktorklang-ora to review. > > As usual, I couldn't resist more fiddling. > - Added missing tests for take, remove(), remove(Object). > - Improved DelayQueueTest's testing infrastructure > - added more test assertions > - linkified new javadoc definitions Martin Buchholz has updated the pull request incrementally with one additional commit since the last revision: fix punctuation - Changes: - all: https://git.openjdk.org/jdk/pull/12838/files - new: https://git.openjdk.org/jdk/pull/12838/files/fb14b2f3..4d02235a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12838&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12838&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12838.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12838/head:pull/12838 PR: https://git.openjdk.org/jdk/pull/12838
Re: RFR: 8300979: Lazily initialize (byte, char)arr in java.io.DataInputStream
On Tue, 21 Mar 2023 15:41:13 GMT, Per Minborg wrote: > This PR proposed to lazily initialize the scratch arrays only if/when needed. Looks like `DataInputStream.readUTF` could benefit from using `JavaLangAccess.countPositives` and return early for the common case that all bytes are ASCII. Food for another PR? - PR Comment: https://git.openjdk.org/jdk/pull/13121#issuecomment-1478310588
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Mar 2023 15:23:53 GMT, Claes Redestad wrote: > An alternative design which would avoid adding more classes could be to add > package-private accessors to the existing `Unmodifiable/Synchronized` wrapper > classes so that `EnumSet/-Map` can retrieve the underlying set of an > unmodifiable or synchronized `Set` or `Map` and then use it directly for > these bulk operations. Then you'd contain the additional overhead to > `EnumSet/-Map`. Another alternative, in cases where `arg.getClass() == X.class` can be safely substituted with `requireNonNull(arg) instanceof X`, is a marker interface. - PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478253385
Re: RFR: 8304450: [vectorapi] Refactor VectorShuffle implementation [v2]
On Tue, 21 Mar 2023 16:11:50 GMT, Quan Anh Mai wrote: > Apart from the mask implementation, shuffle implementation definitely has to > take into consideration the element type. Yes, the way you have implemented shuffle is tightly connected, that looks ok. I am wondering if we can make the mask implementation more loosely coupled and modified such that it does not have to take into consideration the element type (or species) of the vector it operates on, and instead compatibility is based solely on the lane count. Ideally it would be good to change the `VectorMask::check` method to just compare the lanes counts and not require a cast in the implementation, which i presume requires some deeper changes in C2? What you propose seems a possible a interim step towards a more preferable API, if the performance is good. - PR Comment: https://git.openjdk.org/jdk/pull/13093#issuecomment-1478175761
Re: RFR: 8288730: Add type parameter to Lookup::accessClass and Lookup::ensureInitialized [v2]
On Tue, 21 Mar 2023 12:28:22 GMT, Chen Liang wrote: >> Parameterizes `Lookup::accessClass` and `Lookup::ensureInitialized`. Updated >> an applicable use-site within JDK to benefit from this patch. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Update return spec to indicate the same targetClass object is returned The spec adjustment looks good, make it explicit that the method returns the argument passed to the method. - PR Comment: https://git.openjdk.org/jdk/pull/12984#issuecomment-1478163654
Re: RFR: 8300979: Lazily initialize (byte, char)arr in java.io.DataInputStream
On Tue, 21 Mar 2023 15:41:13 GMT, Per Minborg wrote: > This PR proposed to lazily initialize the scratch arrays only if/when needed. src/java.base/share/classes/java/io/DataInputStream.java line 64: > 62: * working arrays initialized on demand by readUTF > 63: */ > 64: private byte[] bytearr; Could the `bytearr` and `chararr` instance variables be removed altogether? - PR Review Comment: https://git.openjdk.org/jdk/pull/13121#discussion_r1143664354
Re: RFR: 8304450: [vectorapi] Refactor VectorShuffle implementation [v2]
On Tue, 21 Mar 2023 16:11:50 GMT, Quan Anh Mai wrote: > I have moved most of the methods to `AbstractVector` and `AbstractShuffle`, I > have to resort to raw types, though, since there seems to be no way to do the > same with wild cards, and the generics mechanism is not powerful enough for > things like `Vector`. The remaining failure seems to be related > to [JDK-8304676](https://bugs.openjdk.org/projects/JDK/issues/JDK-8304676), > so I think this patch is ready for review now. The Java changes look good to me. I need to have another look, but will not be able to do so until next week. - PR Comment: https://git.openjdk.org/jdk/pull/13093#issuecomment-1478148846
Re: RFR: 8304450: [vectorapi] Refactor VectorShuffle implementation [v2]
On Tue, 21 Mar 2023 10:18:19 GMT, Quan Anh Mai wrote: > Note that generics are erased, so from the VM point of view, a > `VectorMask` and a `VectorMask` is indifferent. Yes, that's the easy bit :-) The mask implementation is specialized by the species of vectors it operates on, but does it have to be and can we make it independent of the species and bind to the lane count? Then the user does not need to explicitly cast from and to species that have the same lane count, which means we can remove the VectorMask::cast method (since it already throws if the lane counts are not equal). - PR Comment: https://git.openjdk.org/jdk/pull/13093#issuecomment-1478069401
Integrated: 8304139: Add and method constants to ConstantDescs
On Tue, 14 Mar 2023 14:30:32 GMT, Chen Liang wrote: > Add String constants INIT_NAME, CLASS_INIT_NAME, MTD_void for the names and > method type of instance and class initializers, as they are frequently used > in the Classfile API. In addition, they appear frequently in the JDK itself > as well. > > Update occurrences of and in core libraries API specification > to refer to these constants. The occurrences in code elsewhere will be > converted separately for there are too many. > > See > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000233.html This pull request has now been integrated. Changeset: 019fcd81 Author:Chen Liang Committer: Mandy Chung URL: https://git.openjdk.org/jdk/commit/019fcd819c4f24e6c9de9d4f9fc64b8db6bc6cfa Stats: 56 lines in 7 files changed: 35 ins; 0 del; 21 mod 8304139: Add and method constants to ConstantDescs Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/13020
Re: RFR: 8304450: [vectorapi] Refactor VectorShuffle implementation [v2]
On Sun, 19 Mar 2023 19:38:04 GMT, Quan Anh Mai wrote:
>> Hi,
>>
>> This patch reimplements `VectorShuffle` implementations to be a vector of
>> the bit type. Currently, VectorShuffle is stored as a byte array, and would
>> be expanded upon usage. This poses several drawbacks:
>>
>> 1. Inefficient conversions between a shuffle and its corresponding vector.
>> This hinders the performance when the shuffle indices are not constant and
>> are loaded or computed dynamically.
>> 2. Redundant expansions in `rearrange` operations. On all platforms, it
>> seems that a shuffle index vector is always expanded to the correct type
>> before executing the `rearrange` operations.
>> 3. Some redundant intrinsics are needed to support this handling as well as
>> special considerations in the C2 compiler.
>> 4. Range checks are performed using `VectorShuffle::toVector`, which is
>> inefficient for FP types since both FP conversions and FP comparisons are
>> more expensive than the integral ones.
>>
>> Upon these changes, a `rearrange` can emit more efficient code:
>>
>> var species = IntVector.SPECIES_128;
>> var v1 = IntVector.fromArray(species, SRC1, 0);
>> var v2 = IntVector.fromArray(species, SRC2, 0);
>> v1.rearrange(v2.toShuffle()).intoArray(DST, 0);
>>
>> Before:
>> movabs $0x751589fa8,%r10; {oop([I{0x000751589fa8})}
>> vmovdqu 0x10(%r10),%xmm2
>> movabs $0x7515a0d08,%r10; {oop([I{0x0007515a0d08})}
>> vmovdqu 0x10(%r10),%xmm1
>> movabs $0x75158afb8,%r10; {oop([I{0x00075158afb8})}
>> vmovdqu 0x10(%r10),%xmm0
>> vpand -0xddc12(%rip),%xmm0,%xmm0# Stub::vector_int_to_byte_mask
>> ;
>> {external_word}
>> vpackusdw %xmm0,%xmm0,%xmm0
>> vpackuswb %xmm0,%xmm0,%xmm0
>> vpmovsxbd %xmm0,%xmm3
>> vpcmpgtd %xmm3,%xmm1,%xmm3
>> vtestps %xmm3,%xmm3
>> jne0x7fc2acb4e0d8
>> vpmovzxbd %xmm0,%xmm0
>> vpermd %ymm2,%ymm0,%ymm0
>> movabs $0x751588f98,%r10; {oop([I{0x000751588f98})}
>> vmovdqu %xmm0,0x10(%r10)
>>
>> After:
>> movabs $0x751589c78,%r10; {oop([I{0x000751589c78})}
>> vmovdqu 0x10(%r10),%xmm1
>> movabs $0x75158ac88,%r10; {oop([I{0x00075158ac88})}
>> vmovdqu 0x10(%r10),%xmm2
>> vpxor %xmm0,%xmm0,%xmm0
>> vpcmpgtd %xmm2,%xmm0,%xmm3
>> vtestps %xmm3,%xmm3
>> jne0x7fa818b27cb1
>> vpermd %ymm1,%ymm2,%ymm0
>> movabs $0x751588c68,%r10; {oop([I{0x000751588c68})}
>> vmovdqu %xmm0,0x10(%r10)
>>
>> Please take a look and leave reviews. Thanks a lot.
>
> Quan Anh Mai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix Matcher::vector_needs_load_shuffle
Yes I will try to polish the patch more after finding the cause of the failure
in x86_32. The failure is strange, though, it does not occur on x86_64 for some
reasons.
> One annoyance in the API which propagates down into the implementation is
> `VectorShuffle` and `VectorMask` have `E` that is the lane element type.
Yes I agree, a shuffle merely contains the lane indices while a mask is an
array of boolean, it would be a good cleanup to remove `E` from the interface.
> However, i don't have a good sense of the implications this has to the
> current HotSpot implementation and whether it is feasible.
Note that generics are erased, so from the VM point of view, a `VectorMask`
and a `VectorMask` is indifferent. As a result, removing the type parameter
should not have any impact on the VM. Some details may have to change though,
as element types are removed, a mask or shuffle would only be validated in
accordance to its length, and we need to insert a cast at use sites. The cast
will be removed if it is actually the same species so there is little concern
regarding the machine code emitted.
Thanks a lot.
I have moved most of the methods to `AbstractVector` and `AbstractShuffle`, I
have to resort to raw types, though, since there seems to be no way to do the
same with wild cards, and the generics mechanism is not powerful enough for
things like `Vector`. The remaining failure seems to be related to
[JDK-8304676](https://bugs.openjdk.org/projects/JDK/issues/JDK-8304676), so I
think this patch is ready for review now.
> The mask implementation is specialized by the species of vectors it operates
> on, but does it have to be
Apart from the mask implementation, shuffle implementation definitely has to
take into consideration the element type. However, this information does not
have to be visible to the API, similar to how we currently handle the vector
length, we can have `class AbstractMask implements VectorMask`. As a result,
the cast method would be useless and can be removed in the API, but our
implementation details would still use it, for exa
Re: RFR: 8304450: [vectorapi] Refactor VectorShuffle implementation [v2]
On Sun, 19 Mar 2023 19:38:04 GMT, Quan Anh Mai wrote:
>> Hi,
>>
>> This patch reimplements `VectorShuffle` implementations to be a vector of
>> the bit type. Currently, VectorShuffle is stored as a byte array, and would
>> be expanded upon usage. This poses several drawbacks:
>>
>> 1. Inefficient conversions between a shuffle and its corresponding vector.
>> This hinders the performance when the shuffle indices are not constant and
>> are loaded or computed dynamically.
>> 2. Redundant expansions in `rearrange` operations. On all platforms, it
>> seems that a shuffle index vector is always expanded to the correct type
>> before executing the `rearrange` operations.
>> 3. Some redundant intrinsics are needed to support this handling as well as
>> special considerations in the C2 compiler.
>> 4. Range checks are performed using `VectorShuffle::toVector`, which is
>> inefficient for FP types since both FP conversions and FP comparisons are
>> more expensive than the integral ones.
>>
>> Upon these changes, a `rearrange` can emit more efficient code:
>>
>> var species = IntVector.SPECIES_128;
>> var v1 = IntVector.fromArray(species, SRC1, 0);
>> var v2 = IntVector.fromArray(species, SRC2, 0);
>> v1.rearrange(v2.toShuffle()).intoArray(DST, 0);
>>
>> Before:
>> movabs $0x751589fa8,%r10; {oop([I{0x000751589fa8})}
>> vmovdqu 0x10(%r10),%xmm2
>> movabs $0x7515a0d08,%r10; {oop([I{0x0007515a0d08})}
>> vmovdqu 0x10(%r10),%xmm1
>> movabs $0x75158afb8,%r10; {oop([I{0x00075158afb8})}
>> vmovdqu 0x10(%r10),%xmm0
>> vpand -0xddc12(%rip),%xmm0,%xmm0# Stub::vector_int_to_byte_mask
>> ;
>> {external_word}
>> vpackusdw %xmm0,%xmm0,%xmm0
>> vpackuswb %xmm0,%xmm0,%xmm0
>> vpmovsxbd %xmm0,%xmm3
>> vpcmpgtd %xmm3,%xmm1,%xmm3
>> vtestps %xmm3,%xmm3
>> jne0x7fc2acb4e0d8
>> vpmovzxbd %xmm0,%xmm0
>> vpermd %ymm2,%ymm0,%ymm0
>> movabs $0x751588f98,%r10; {oop([I{0x000751588f98})}
>> vmovdqu %xmm0,0x10(%r10)
>>
>> After:
>> movabs $0x751589c78,%r10; {oop([I{0x000751589c78})}
>> vmovdqu 0x10(%r10),%xmm1
>> movabs $0x75158ac88,%r10; {oop([I{0x00075158ac88})}
>> vmovdqu 0x10(%r10),%xmm2
>> vpxor %xmm0,%xmm0,%xmm0
>> vpcmpgtd %xmm2,%xmm0,%xmm3
>> vtestps %xmm3,%xmm3
>> jne0x7fa818b27cb1
>> vpermd %ymm1,%ymm2,%ymm0
>> movabs $0x751588c68,%r10; {oop([I{0x000751588c68})}
>> vmovdqu %xmm0,0x10(%r10)
>>
>> Please take a look and leave reviews. Thanks a lot.
>
> Quan Anh Mai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix Matcher::vector_needs_load_shuffle
That looks like a very good simplification and performance enhancement, and
removing a limitation, the byte[] representation. This should likely also help
with Valhalla integration.
IIUC it has the same upper bound limitation for vector lengths greater than the
maximum index size that can be represented as a lane element (although in
practice there may not be any hardware where this can occur). Which is fine, i
am not suggesting we try and fix this.
Perhaps it may be possible to move some methods on the concrete implementations
to the abstract implementations as helper methods or template methods, thereby
reducing the amount of generated code? It seems so in some cases, but i did not
look very closely. It may require the introduction of an an element type
specific abstract shuffle, and if that's the case it may not be worth it.
--
Relatedly, i would be interested in your opinion on the following. One
annoyance in the API which propagates down into the implementation is
`VectorShuffle` and `VectorMask` have `E` that is the lane element type.
But, in theory they should not need E, and any shuffle or mask with the same
lanes as the vector being operated on should be compatible, and it's an
implementation detail of the shuffle/mask how its state represented as a
hardware register. However, i don't have a good sense of the implications this
has to the current HotSpot implementation and whether it is feasible.
-
PR Review: https://git.openjdk.org/jdk/pull/13093#pullrequestreview-1349108003
Re: RFR: 8304450: [vectorapi] Refactor VectorShuffle implementation [v3]
> Hi,
>
> This patch reimplements `VectorShuffle` implementations to be a vector of the
> bit type. Currently, VectorShuffle is stored as a byte array, and would be
> expanded upon usage. This poses several drawbacks:
>
> 1. Inefficient conversions between a shuffle and its corresponding vector.
> This hinders the performance when the shuffle indices are not constant and
> are loaded or computed dynamically.
> 2. Redundant expansions in `rearrange` operations. On all platforms, it seems
> that a shuffle index vector is always expanded to the correct type before
> executing the `rearrange` operations.
> 3. Some redundant intrinsics are needed to support this handling as well as
> special considerations in the C2 compiler.
> 4. Range checks are performed using `VectorShuffle::toVector`, which is
> inefficient for FP types since both FP conversions and FP comparisons are
> more expensive than the integral ones.
>
> Upon these changes, a `rearrange` can emit more efficient code:
>
> var species = IntVector.SPECIES_128;
> var v1 = IntVector.fromArray(species, SRC1, 0);
> var v2 = IntVector.fromArray(species, SRC2, 0);
> v1.rearrange(v2.toShuffle()).intoArray(DST, 0);
>
> Before:
> movabs $0x751589fa8,%r10; {oop([I{0x000751589fa8})}
> vmovdqu 0x10(%r10),%xmm2
> movabs $0x7515a0d08,%r10; {oop([I{0x0007515a0d08})}
> vmovdqu 0x10(%r10),%xmm1
> movabs $0x75158afb8,%r10; {oop([I{0x00075158afb8})}
> vmovdqu 0x10(%r10),%xmm0
> vpand -0xddc12(%rip),%xmm0,%xmm0# Stub::vector_int_to_byte_mask
> ;
> {external_word}
> vpackusdw %xmm0,%xmm0,%xmm0
> vpackuswb %xmm0,%xmm0,%xmm0
> vpmovsxbd %xmm0,%xmm3
> vpcmpgtd %xmm3,%xmm1,%xmm3
> vtestps %xmm3,%xmm3
> jne0x7fc2acb4e0d8
> vpmovzxbd %xmm0,%xmm0
> vpermd %ymm2,%ymm0,%ymm0
> movabs $0x751588f98,%r10; {oop([I{0x000751588f98})}
> vmovdqu %xmm0,0x10(%r10)
>
> After:
> movabs $0x751589c78,%r10; {oop([I{0x000751589c78})}
> vmovdqu 0x10(%r10),%xmm1
> movabs $0x75158ac88,%r10; {oop([I{0x00075158ac88})}
> vmovdqu 0x10(%r10),%xmm2
> vpxor %xmm0,%xmm0,%xmm0
> vpcmpgtd %xmm2,%xmm0,%xmm3
> vtestps %xmm3,%xmm3
> jne0x7fa818b27cb1
> vpermd %ymm1,%ymm2,%ymm0
> movabs $0x751588c68,%r10; {oop([I{0x000751588c68})}
> vmovdqu %xmm0,0x10(%r10)
>
> Please take a look and leave reviews. Thanks a lot.
Quan Anh Mai has updated the pull request incrementally with two additional
commits since the last revision:
- missing casts
- clean up
-
Changes:
- all: https://git.openjdk.org/jdk/pull/13093/files
- new: https://git.openjdk.org/jdk/pull/13093/files/060554a9..4caa9d10
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=13093&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=13093&range=01-02
Stats: 1396 lines in 36 files changed: 44 ins; 1339 del; 13 mod
Patch: https://git.openjdk.org/jdk/pull/13093.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/13093/head:pull/13093
PR: https://git.openjdk.org/jdk/pull/13093
Re: RFR: 8300979: Lazily initialize (byte, char)arr in java.io.DataInputStream
On Tue, 21 Mar 2023 15:41:13 GMT, Per Minborg wrote:
> This PR proposed to lazily initialize the scratch arrays only if/when needed.
Looks good. Many DataInputStreams probably never read UTF. (Not a reviewer)
src/java.base/share/classes/java/io/DataInputStream.java line 576:
> 574: char[] chararr = null;
> 575: if (in instanceof DataInputStream dis) {
> 576: if (dis.bytearr == null || dis.bytearr.length < utflen){
Add a space between ')' and '{'?
Suggestion:
if (dis.bytearr == null || dis.bytearr.length < utflen) {
-
Marked as reviewed by eir...@github.com (no known OpenJDK username).
PR Review: https://git.openjdk.org/jdk/pull/13121#pullrequestreview-1350859010
PR Review Comment: https://git.openjdk.org/jdk/pull/13121#discussion_r1143638420
RFR: 8300979: Lazily initialize (byte, char)arr in java.io.DataInputStream
This PR proposed to lazily initialize the scratch arrays only if/when needed. - Commit messages: - Remove redundant initilizers - Lazily initialize (byte, char)arr in java.io.DataInputStream Changes: https://git.openjdk.org/jdk/pull/13121/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13121&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300979 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13121.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13121/head:pull/13121 PR: https://git.openjdk.org/jdk/pull/13121
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote: >> Currently, the two subclasses of `java.util.EnumSet` optimize bulk >> operations when the argument is also a `EnumSet`, but there is no such >> optimization for wrapper sets (returned by `Collections.unmodifiableSet`, >> `Collections.synchronizedSet`, etc.) and immutable sets (returned by >> `Set.of` methods) of `Enum`s. >> >> This PR introduces optimization classes for these situations. No public >> APIs are changed. > > Tingjun Yuan has updated the pull request incrementally with one additional > commit since the last revision: > > Set.copyOf: need defensive copy I'm wary of the impact of adding new wrapper classes. It may make the factory methods slightly slower due additional checks, but also risks increasing the number of classes at various call-sites which might upset call site inlining. An alternative design which would avoid adding more classes could be to add package-private accessors to the existing `Unmodifiable/Synchronized` wrapper classes so that `EnumSet/-Map` can retrieve the underlying set of an unmodifiable or synchronized `Set` or `Map` and then use it directly for these bulk operations. Then you'd contain the additional overhead to `EnumSet/-Map`. - PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478035549
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
On Tue, 21 Mar 2023 11:56:57 GMT, Alan Bateman wrote: >> test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java >> line 28: >> >>> 26: * @bug 8303742 >>> 27: * @summary CompletableFuture.orTimeout can leak memory if completed >>> exceptionally >>> 28: * @run junit/othervm/timeout=1000 -Xmx128m >>> CompletableFutureOrTimeoutExceptionallyTest >> >> Hello Viktor, a timeout of 1000 seconds is 16 minutes. This timeout value is >> then multiplied by the timeout factor (which is a jtreg option) to decide >> the final timeout. The CI on which this was reported use a timeout factor of >> 4. So this would translate to a timeout of 64 minutes, which I think is >> extremely high (although it's just the upper bound). >> >> Perhaps you could increase this timeout to maybe 5 minutes (300 seconds) and >> run some tests on our CI to see if that is sufficient? > > If there isn't any value running this test with a debug build then you could > have it skip (`@requires vm.debug == true`) to save resources. @AlanBateman Wouldn't that be `@requires vm.debug == false` ? - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1143541565
Integrated: 8304180: Constant Descriptors for MethodHandles::classData and classDataAt
On Wed, 15 Mar 2023 02:18:37 GMT, Chen Liang wrote: > Add Constant Descriptors for DirectMethodHandleDesc of > MethodHandles::classData and classDataAt in ConstantDescs. This facilitates > easier access of class data via condy in Classfile API-generated bytecode. > Most other constant bootstrap methods provided in the JDK, notably from > `ConstantBootstraps`, already have constant descriptors in ConstantDescs. > > Context: > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000235.html This pull request has now been integrated. Changeset: d788a1bb Author:Chen Liang Committer: Jorn Vernee URL: https://git.openjdk.org/jdk/commit/d788a1bb808da73ef17aee0b773b7e3ea682426f Stats: 16 lines in 1 file changed: 16 ins; 0 del; 0 mod 8304180: Constant Descriptors for MethodHandles::classData and classDataAt Reviewed-by: jvernee, mchung - PR: https://git.openjdk.org/jdk/pull/13033
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
On Tue, 21 Mar 2023 11:02:00 GMT, Jaikiran Pai wrote: >> Improves the stability of the memory leak test for CompletableFuture timeout >> cancellation by both reducing the count by 50% (which should still be above >> threshold to trigger given the ample margin set initially) as well as >> extending the default timeout of the test run. > > test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java > line 28: > >> 26: * @bug 8303742 >> 27: * @summary CompletableFuture.orTimeout can leak memory if completed >> exceptionally >> 28: * @run junit/othervm/timeout=1000 -Xmx128m >> CompletableFutureOrTimeoutExceptionallyTest > > Hello Viktor, a timeout of 1000 seconds is 16 minutes. This timeout value is > then multiplied by the timeout factor (which is a jtreg option) to decide the > final timeout. The CI on which this was reported use a timeout factor of 4. > So this would translate to a timeout of 64 minutes, which I think is > extremely high (although it's just the upper bound). > > Perhaps you could increase this timeout to maybe 5 minutes (300 seconds) and > run some tests on our CI to see if that is sufficient? @jaikiran Having a long timeout doesn't seem like a problem given that it just needs enough time to run through the iterations (i.e. that's the max duration of the test before giving up). @AlanBateman That's a great observation—do you see anything about this which would impact whether it makes sense to run on top of a debug-build (I don't see any). If it doesn't need to run on a debug build then I'll add the requirement you suggested. Let me know. - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1143522447
Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v46]
On Tue, 21 Mar 2023 13:31:37 GMT, Maurizio Cimadamore
wrote:
>> Jim Laskey has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Tidy javadoc
>> - Rename StringTemplate classes
>
> src/java.base/share/classes/java/lang/StringTemplate.java line 69:
>
>> 67: * List values = st.values();
>> 68: * }
>> 69: * The value of {@code fragments} will be equivalent to {@code
>> List.of("", " + ", " = ", "")},
>
> This is a bit hard to parse, due to the use of `the value of` (which then
> becomes problematic in the next para, as we are talking about `values`).
> Either change the name of the variable `values` in the snippet, or use some
> other locution, like "the list {@code fragments} is equivalent to ..." etc.
The above comment also applies to the javadoc of the `fragments` and `values`
methods, which use a similar way to describe their results.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143387999
Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v46]
On Mon, 20 Mar 2023 20:31:46 GMT, Jim Laskey wrote:
>> Enhance the Java programming language with string templates, which are
>> similar to string literals but contain embedded expressions. A string
>> template is interpreted at run time by replacing each expression with the
>> result of evaluating that expression, possibly after further validation and
>> transformation. This is a [preview language feature and
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Tidy javadoc
> - Rename StringTemplate classes
I like the new naming scheme!
src/java.base/share/classes/java/lang/StringTemplate.java line 28:
> 26: package java.lang;
> 27:
> 28: import java.lang.Object;
You might want to do another pass to check for unused imports
src/java.base/share/classes/java/lang/StringTemplate.java line 44:
> 42: * {@link StringTemplate} is the run-time representation of a string
> template or
> 43: * text block template in a template expression.
> 44: *
Extra newline?
src/java.base/share/classes/java/lang/StringTemplate.java line 56:
> 54: * {@link StringTemplate} is primarily used in conjunction with a
> template processor
> 55: * to produce a string or other meaningful value. Evaluation of a
> template expression
> 56: * first produces an instance of {@link StringTemplate}, representing the
> template
The "template of the template expression" - is this nomenclature official (e.g.
backed by any text in the JLS?). I must admit I find it a tad jarring.
src/java.base/share/classes/java/lang/StringTemplate.java line 69:
> 67: * List values = st.values();
> 68: * }
> 69: * The value of {@code fragments} will be equivalent to {@code
> List.of("", " + ", " = ", "")},
This is a bit hard to parse, due to the use of `the value of` (which then
becomes problematic in the next para, as we are talking about `values`). Either
change the name of the variable `values` in the snippet, or use some other
locution, like "the list {@code fragments} is equivalent to ..." etc.
src/java.base/share/classes/java/lang/StringTemplate.java line 324:
> 322: * assert Objects.equals(sc, "abcxyz");
> 323: * }
> 324: * the last character {@code "c"} from the first string is
> juxtaposed with the first
I was playing with this example in jshell:
jshell> var st1 = RAW."{1}"
st1 ==> StringTemplate{ fragments = [ "", "" ], values = [1] }
jshell> var st2 = RAW."{2}"
st2 ==> StringTemplate{ fragments = [ "", "" ], values = [2] }
jshell> StringTemplate.combine(st1, st2);
$7 ==> StringTemplate{ fragments = [ "", "", "" ], values = [1, 2] }
The result is obviously well-formed - but I'm not sure I can derive what the
method does just by reading the javadoc. The javadoc says:
Fragment lists from the StringTemplates are combined end to end with the last
fragment from each StringTemplate concatenated with the first fragment of the
next
I think I see what you want to say - (e.g. the end fragment of string template
`N` is concatenated with the starting fragment of string template `N + 1`).
src/java.base/share/classes/java/lang/StringTemplate.java line 431:
> 429: * Java compilation unit.The result of interpolation is not
> interned.
> 430: */
> 431: static final StringProcessor STR = StringTemplate::interpolate;
`static final` is redundant here (we are in an interface)
src/java.base/share/classes/java/lang/StringTemplate.java line 454:
> 452: * This interface describes the methods provided by a generalized
> string template processor. The
> 453: * primary method {@link Processor#process(StringTemplate)} is used
> to validate
> 454: * and compose a result using a {@link StringTemplate
> StringTemplate's} fragments and values lists.
nit: `{@link StringTemplate StringTemplate's}` will probably not render as
you'd expect (as it will all be preformat, including the `'s`).
src/java.base/share/classes/java/lang/runtime/ReferenceKey.java line 47:
> 45: */
> 46: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
> 47: interface ReferenceKey {
This (and other) class are package-private. Do we still need @PreviewFeature
for stuff like this, since it's not meant to be used publicly?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line
226:
> 224: List.nil(), expressions);
> 225: valuesArray.type = new ArrayType(syms.objectType,
> syms.arrayClass);
> 226: return bsmCall(names.process,
> names.newLargeStringTemplate, syms.stringTemplateType,
nit: the indy name is irrelevant here - that said, `process` is a tad
confusing, since it's also the name of a StringTemplate method.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line
252:
> 250: staticArgsTypes =
> staticArgsTypes.append(syms.stringType);
> 251:
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
Hello Stuart Marks, would you be able to take a look at this patch, improving performance of enum-only immutable collections, as you are the primary engineer in this area? On Mon, Feb 20, 2023 at 9:40 PM Tingjun Yuan wrote: > > > Currently, the two subclasses of `java.util.EnumSet` optimize bulk > > operations when the argument is also a `EnumSet`, but there is no such > > optimization for wrapper sets (returned by `Collections.unmodifiableSet`, > > `Collections.synchronizedSet`, etc.) and immutable sets (returned by > > `Set.of` methods) of `Enum`s. > > > > This PR introduces optimization classes for these situations. No public > > APIs are changed. > > Tingjun Yuan has updated the pull request incrementally with one additional > commit since the last revision: > > Set.copyOf: need defensive copy > > - > > Changes: > - all: https://git.openjdk.org/jdk/pull/12498/files > - new: https://git.openjdk.org/jdk/pull/12498/files/b9699bfe..0e0b2bf1 > > Webrevs: > - full: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=04 > - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=03-04 > > Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod > Patch: https://git.openjdk.org/jdk/pull/12498.diff > Fetch: git fetch https://git.openjdk.org/jdk pull/12498/head:pull/12498 > > PR: https://git.openjdk.org/jdk/pull/12498
Re: RFR: 8301552: Use AtomicReferenceArray for caching instead of CHM in ZoneOffset [v8]
On Tue, 21 Mar 2023 07:25:38 GMT, Per Minborg wrote:
>> src/java.base/share/classes/java/time/ZoneOffset.java line 430:
>>
>>> 428: public static ZoneOffset ofTotalSeconds(int totalSeconds) {
>>> 429: final class Holder {
>>> 430: private static final IntFunction
>>> ZONE_OFFSET_MAPPER = new ZoneOffsetMapper();
>>
>> Can't the ZoneOffsetMapper itself serve as a holder class? so we move this
>> singleton into ZoneOffsetMapper itself.
>
> It is possible but this keeps the mapper more local and only accessible where
> it is supposed to be used. For testing purposed, it might be better to have
> the class as you propose.
If we want it local, I suppose we can convert the whole `ZoneOffsetMapper`
local with this singleton in `ofTotalSeconds` static method as well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12346#discussion_r1143415419
Re: RFR: 8301552: Use AtomicReferenceArray for caching instead of CHM in ZoneOffset [v8]
On Tue, 21 Mar 2023 07:29:49 GMT, Per Minborg wrote: >> It can. I think either works fine. Maybe a bit nicer with a method reference >> as you propose. > > Perhaps you mean the method could be *replaced* with `Integer::valueOf"? Good > suggestion. Yes, `Integer::valueOf` is a better replacement of individual `intIdentity` calls. Thought you may want this method so only one lambda callsite would be generated. - PR Review Comment: https://git.openjdk.org/jdk/pull/12346#discussion_r1143417225
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 09:02:29 GMT, Per Minborg wrote: >> API changes for the FFM API (third preview) >> >> Specdiff: >> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html >> >> Javadoc: >> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Add example for Option::captureStateLayout A review of all the copyright years shall be made in this PR. - PR Comment: https://git.openjdk.org/jdk/pull/13079#issuecomment-1477849414
Re: RFR: 8266571: Sequenced Collections
On Tue, 8 Feb 2022 17:23:38 GMT, Stuart Marks wrote: > PR for Sequenced Collections implementation. I am interested in the optimization potential of `arraylist.reversed().addAll()` and similar batch operations. Though reverse list/deque views are a good start, I still wish to see customized implementations for `reversed` to perform `addAll` `removeIf` efficiently. - PR Comment: https://git.openjdk.org/jdk/pull/7387#issuecomment-1477768549
Re: RFR: 8288730: Add type parameter to Lookup::accessClass and Lookup::ensureInitialized
On Sat, 11 Mar 2023 03:28:16 GMT, Chen Liang wrote: > Parameterizes `Lookup::accessClass` and `Lookup::ensureInitialized`. Updated > an applicable use-site within JDK to benefit from this patch. Based on the feedback in the CSR, updated the return spec for `accessClass` to indicate the input `targetClass` is returned, in line with the specification for `ensureInitialized`. - PR Comment: https://git.openjdk.org/jdk/pull/12984#issuecomment-1477748354
Re: RFR: 8288730: Add type parameter to Lookup::accessClass and Lookup::ensureInitialized [v2]
> Parameterizes `Lookup::accessClass` and `Lookup::ensureInitialized`. Updated > an applicable use-site within JDK to benefit from this patch. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Update return spec to indicate the same targetClass object is returned - Changes: - all: https://git.openjdk.org/jdk/pull/12984/files - new: https://git.openjdk.org/jdk/pull/12984/files/e4dc34cb..3b7d8055 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12984&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12984&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12984.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12984/head:pull/12984 PR: https://git.openjdk.org/jdk/pull/12984
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 00:54:10 GMT, Paul Sandoz wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add example for Option::captureStateLayout
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 621:
>
>> 619: * to a downcall handle linked with {@link
>> #captureCallState(String...)}}
>> 620: *
>> 621: * @see #captureCallState(String...)
>
> How does a caller know what the structure may contain? Should we document the
> platform specific structures?
Back to @PaulSandoz question - "how does the caller know what the structure
contains?". The caller typically doesn't care too much about what the returned
struct is. But it might care about which "values" can be captured. That said,
the set of such interesting values, is not too surprising. As demonstrated in
the example in the `Linker.capturedCallState` method, once the client knows the
name (and "errno" is likely to be 90% case), everything else follows from there
- as the layout can be used to obtain var handles for the required values.
But, perhaps, now that I write this, I realize that what @PaulSandoz might
_really_ be asking is: how do I know that e.g. the returned struct will not
contain e.g. nested structs, sequences, or other non-sense. So we might specify
(in a normative way) that the returned layout is a struct layout, whose member
layouts are one or more value layouts (possibly with some added padding
layouts). The names of the value layouts reflect the names of the values that
can be captured.
And _then_ we show an example of the layout we return for Windows/x64 (as
that's more interesting).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143281164
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 11:58:28 GMT, Per Minborg wrote: >> I'm not sure about this. Honestly, the example probably doesn't help much if >> somebody didn't get the idea of what the layout might be. I think it might >> be better to say something like, `on Windows/x64 the returned layout might >> be as follows...` and then you write the code to create the layout instance >> corresponding to the returned layout. But we have to be careful to make the >> text non-normative (as the set of values might change). > > What about adding something like this? > > > Here is a list of valid names for some platforms: > Linux: > "errno" > macOS: > "errno" > Windows: > "GetLastError", "WSAGetLastError" and "errno" I'd prefer something more informal: "Examples of valid names are "errno" on Linux/x64, or "GetLastError" on Windows/x64". And maybe add that "The precise set of platform-dependent supported names can be queried using the returned group layout". - PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143272764
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 09:54:16 GMT, Maurizio Cimadamore wrote: >> I've added an example of how to print the platform-dependent structure. >> Should we document anyhow? > > I'm not sure about this. Honestly, the example probably doesn't help much if > somebody didn't get the idea of what the layout might be. I think it might be > better to say something like, `on Windows/x64 the returned layout might be as > follows...` and then you write the code to create the layout instance > corresponding to the returned layout. But we have to be careful to make the > text non-normative (as the set of values might change). What about adding something like this? Here is a list of valid names for some platforms: Linux: "errno" macOS: "errno" Windows: "GetLastError", "WSAGetLastError" and "errno" - PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143265595
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
On Tue, 21 Mar 2023 11:02:00 GMT, Jaikiran Pai wrote: >> Improves the stability of the memory leak test for CompletableFuture timeout >> cancellation by both reducing the count by 50% (which should still be above >> threshold to trigger given the ample margin set initially) as well as >> extending the default timeout of the test run. > > test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java > line 28: > >> 26: * @bug 8303742 >> 27: * @summary CompletableFuture.orTimeout can leak memory if completed >> exceptionally >> 28: * @run junit/othervm/timeout=1000 -Xmx128m >> CompletableFutureOrTimeoutExceptionallyTest > > Hello Viktor, a timeout of 1000 seconds is 16 minutes. This timeout value is > then multiplied by the timeout factor (which is a jtreg option) to decide the > final timeout. The CI on which this was reported use a timeout factor of 4. > So this would translate to a timeout of 64 minutes, which I think is > extremely high (although it's just the upper bound). > > Perhaps you could increase this timeout to maybe 5 minutes (300 seconds) and > run some tests on our CI to see if that is sufficient? If there isn't any value running this test with a debug build then you could have it skip (`@requires vm.debug == true`) to save resources. - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1143263858
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v14]
On Tue, 21 Mar 2023 11:31:54 GMT, Alan Bateman wrote: >> It seems to be redundant as the following code does not expect it on stack >> and loads it again as needed. > > Yes, I think so. If `java -XX:+UnlockDiagnosticVMOptions > -XX:+BytecodeVerificationLocal -version` doesn't fail and you've run all the > tests then it should be okay to drop it. I can confirm `java -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal -version` successful execution and passing all jdk/tools/jlink tests. Now waiting for all tier1 tests results. - PR Review Comment: https://git.openjdk.org/jdk/pull/12944#discussion_r1143261554
Re: RFR: JDK-8304028: Port fdlibm IEEEremainder to Java
On Tue, 21 Mar 2023 06:11:57 GMT, Joe Darcy wrote:
> Last but not least, a port of fdlibm IEEEremainder from C to Java. I plan to
> write some more implementation-specific tests around decision points in the
> FDLIBM algorithm, but I wanted to get the bulk of the changes out for review
> first.
>
> Note that since IEEEremainder was the last native method in StrictMath.java,
> the StrictMath.c file needed to be deleted (or modified) since StrictMath.h
> was no longer generated as part of the build. (StrictMath.c was one of the
> file deleted as part of JDK-8302801).
>
> For testing, Mach 5 tier 1 through 3 were successful (other than an unrelated
> test failure that was problem listed) and the exhaustive test was locally run
> and passed with "16, 16" to increase the testing density.
src/java.base/share/classes/java/lang/FdLibm.java line 3399:
> 3397: }
> 3398: }
> 3399: if(iy >= -1022)
Suggestion:
if (iy >= -1022)
src/java.base/share/classes/java/lang/FdLibm.java line 3414:
> 3412: // fix point fmod
> 3413: n = ix - iy;
> 3414: while(n-- != 0) {
Suggestion:
while (n-- != 0) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13113#discussion_r1143247870
PR Review Comment: https://git.openjdk.org/jdk/pull/13113#discussion_r1143249143
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v14]
On Tue, 21 Mar 2023 09:10:05 GMT, Adam Sotona wrote: >> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java >> line 1021: >> >>> 1019: MethodTypeDesc.of(CD_void, >>> CD_String)) >>> 1020:.astore(BUILDER_VAR) >>> 1021:.aload(BUILDER_VAR); >> >> The methods to generate the MD all load the Builder onto the operand stack >> so the aload at L1021 is catching my attention, is that needed? > > It seems to be redundant as the following code does not expect it on stack > and loads it again as needed. Yes, I think so. If `java -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal -version` doesn't fail and you've run all the tests then it should be okay to drop it. - PR Review Comment: https://git.openjdk.org/jdk/pull/12944#discussion_r1143238336
Re: RFR: JDK-8304028: Port fdlibm IEEEremainder to Java
On Tue, 21 Mar 2023 06:11:57 GMT, Joe Darcy wrote: > Last but not least, a port of fdlibm IEEEremainder from C to Java. I plan to > write some more implementation-specific tests around decision points in the > FDLIBM algorithm, but I wanted to get the bulk of the changes out for review > first. > > Note that since IEEEremainder was the last native method in StrictMath.java, > the StrictMath.c file needed to be deleted (or modified) since StrictMath.h > was no longer generated as part of the build. (StrictMath.c was one of the > file deleted as part of JDK-8302801). > > For testing, Mach 5 tier 1 through 3 were successful (other than an unrelated > test failure that was problem listed) and the exhaustive test was locally run > and passed with "16, 16" to increase the testing density. src/java.base/share/classes/java/lang/FdLibm.java line 3337: > 3335: } > 3336: x = Math.abs(x); > 3337: p = Math.abs(p); Suggestion: x = Math.abs(x); p = Math.abs(p); src/java.base/share/classes/java/lang/FdLibm.java line 3371: > 3369: > 3370: // purge off exception values > 3371: if((hy | ly) == 0 || (hx >= 0x7ff0_)|| // y = 0, > or x not finite Suggestion: if ((hy | ly) == 0 || (hx >= 0x7ff0_)|| // y = 0, or x not finite - PR Review Comment: https://git.openjdk.org/jdk/pull/13113#discussion_r1143236600 PR Review Comment: https://git.openjdk.org/jdk/pull/13113#discussion_r1143236820
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
On Tue, 21 Mar 2023 10:42:55 GMT, Viktor Klang wrote: > Improves the stability of the memory leak test for CompletableFuture timeout > cancellation by both reducing the count by 50% (which should still be above > threshold to trigger given the ample margin set initially) as well as > extending the default timeout of the test run. The PR seems to be using an incorrect JBS issue id/summary. This PR should be linked with the new https://bugs.openjdk.org/browse/JDK-8304557 issue. test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java line 28: > 26: * @bug 8303742 > 27: * @summary CompletableFuture.orTimeout can leak memory if completed > exceptionally > 28: * @run junit/othervm/timeout=1000 -Xmx128m > CompletableFutureOrTimeoutExceptionallyTest Hello Viktor, a timeout of 1000 seconds is 16 minutes. This timeout value is then multiplied by the timeout factor (which is a jtreg option) to decide the final timeout. The CI on which this was reported use a timeout factor of 4. So this would translate to a timeout of 64 minutes, which I think is extremely high (although it's just the upper bound). Perhaps you could increase this timeout to maybe 5 minutes (300 seconds) and run some tests on our CI to see if that is sufficient? - PR Comment: https://git.openjdk.org/jdk/pull/13116#issuecomment-1477639233 PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1143207328
RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally
Improves the stability of the memory leak test for CompletableFuture timeout cancellation by both reducing the count by 50% (which should still be above threshold to trigger given the ample margin set initially) as well as extending the default timeout of the test run. - Commit messages: - Hardening the memory leak test for CompletableFuture timeout cancellation Changes: https://git.openjdk.org/jdk/pull/13116/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13116&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303742 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13116.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13116/head:pull/13116 PR: https://git.openjdk.org/jdk/pull/13116
Re: RFR: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved [v2]
On Tue, 21 Mar 2023 09:45:56 GMT, Jaikiran Pai wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update
>> src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
>>
>> Co-authored-by: liach <7806504+li...@users.noreply.github.com>
>
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
> line 172:
>
>> 170: public StaticClassHierarchyResolver(Collection
>> interfaceNames, Map classToSuperClass) {
>> 171: map = HashMap.newHashMap(interfaceNames.size() +
>> classToSuperClass.size() + 1);
>> 172: map.put(ConstantDescs.CD_Object, new
>> ClassHierarchyInfo(ConstantDescs.CD_Object, false, null));
>
> Hello Adam, for this `Object` class representation, perhaps you could just
> have a `static final` field which is instantiated to this value and reused
> instead of creating a new instance each time?
fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13099#discussion_r1143179081
Re: RFR: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved [v3]
On Tue, 21 Mar 2023 10:40:35 GMT, Adam Sotona wrote: >> Classfile API class hierarchy makes assumptions when class is not resolved >> and that may lead to silent generation of invalid stack maps. Only >> debug-level log information of case is actually provided. >> >> Proposed patch throws IllegalArgumentException when the class is not >> resolved instead. >> >> Thanks for review. >> >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > avoid repeated ClassHierarchyInfo of CD_Object instantiation in > StaticClassHierarchyResolver Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13099#pullrequestreview-1350139685
Re: RFR: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved [v3]
> Classfile API class hierarchy makes assumptions when class is not resolved > and that may lead to silent generation of invalid stack maps. Only > debug-level log information of case is actually provided. > > Proposed patch throws IllegalArgumentException when the class is not resolved > instead. > > Thanks for review. > > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: avoid repeated ClassHierarchyInfo of CD_Object instantiation in StaticClassHierarchyResolver - Changes: - all: https://git.openjdk.org/jdk/pull/13099/files - new: https://git.openjdk.org/jdk/pull/13099/files/8f166bdd..39f46722 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13099&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13099&range=01-02 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13099.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13099/head:pull/13099 PR: https://git.openjdk.org/jdk/pull/13099
Re: RFR: JDK-8295859 Update Manual Test Groups [v2]
On Mon, 20 Mar 2023 23:29:25 GMT, Bill Huang wrote: >> The purpose of this task is to add the difference between -manual jdk_core >> and jdk_core_manual to the jdk_core_manual test goal. Furthermore, in order >> to streamline the manual test execution process, a new test group called >> jdk_core_manual_human has been created for manual tests that demand extra >> preparation or test input. > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Implemented review comments. LGTM - PR Comment: https://git.openjdk.org/jdk/pull/12840#issuecomment-1477607865
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v16]
> jdk.jlink internal plugins are heavily using ASM > > This patch converts ASM calls to Classfile API. > > Please review. > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: removed obsolete instruction from SystemModulesPlugin - Changes: - all: https://git.openjdk.org/jdk/pull/12944/files - new: https://git.openjdk.org/jdk/pull/12944/files/71ed9a43..3c12e88c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12944&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12944&range=14-15 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12944.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12944/head:pull/12944 PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 09:02:29 GMT, Per Minborg wrote:
>> API changes for the FFM API (third preview)
>>
>> Specdiff:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html
>>
>> Javadoc:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add example for Option::captureStateLayout
src/java.base/share/classes/java/lang/foreign/Linker.java line 480:
> 478: * Otherwise, the invocation throws {@link
> WrongThreadException}; and
> 479: * {@code A} is kept alive during the invocation. For
> instance, if {@code A} has been obtained using a
> 480: * {@linkplain Arena#ofShared()} confined arena}, any attempt to
> {@linkplain Arena#close() close}
the description of the link still says "confined"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143131392
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 08:57:42 GMT, Per Minborg wrote:
>> src/java.base/share/classes/java/lang/foreign/Linker.java line 621:
>>
>>> 619: * to a downcall handle linked with {@link
>>> #captureCallState(String...)}}
>>> 620: *
>>> 621: * @see #captureCallState(String...)
>>
>> How does a caller know what the structure may contain? Should we document
>> the platform specific structures?
>
> I've added an example of how to print the platform-dependent structure.
> Should we document anyhow?
I'm not sure about this. Honestly, the example probably doesn't help much if
somebody didn't get the idea of what the layout might be. I think it might be
better to say something like, `on Windows/x64 the returned layout might be as
follows...` and then you write the code to create the layout instance
corresponding to the returned layout. But we have to be careful to make the
text non-normative (as the set of values might change).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143125997
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 00:35:40 GMT, Paul Sandoz wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add example for Option::captureStateLayout
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 479:
>
>> 477: * Otherwise, the invocation throws {@link
>> WrongThreadException}; and
>> 478: * {@code A} is kept alive during the invocation. For
>> instance, if {@code A} has been obtained using a
>> 479: * {@linkplain Arena#ofConfined() confined arena}, any attempt
>> to {@linkplain Arena#close() close}
>
> Is that correct? Do you mean a shared arena?
The text is correct (you can still close a confined arena from inside an
upcall), but I agree that perhaps using a shared arena might be a bit more
intuitive.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143118512
Re: RFR: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved [v2]
On Mon, 20 Mar 2023 14:06:43 GMT, Adam Sotona wrote:
>> Classfile API class hierarchy makes assumptions when class is not resolved
>> and that may lead to silent generation of invalid stack maps. Only
>> debug-level log information of case is actually provided.
>>
>> Proposed patch throws IllegalArgumentException when the class is not
>> resolved instead.
>>
>> Thanks for review.
>>
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
>
> Co-authored-by: liach <7806504+li...@users.noreply.github.com>
Looks OK to me. Just a minor comment, which I've added inline.
src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
line 172:
> 170: public StaticClassHierarchyResolver(Collection
> interfaceNames, Map classToSuperClass) {
> 171: map = HashMap.newHashMap(interfaceNames.size() +
> classToSuperClass.size() + 1);
> 172: map.put(ConstantDescs.CD_Object, new
> ClassHierarchyInfo(ConstantDescs.CD_Object, false, null));
Hello Adam, for this `Object` class representation, perhaps you could just have
a `static final` field which is instantiated to this value and reused instead
of creating a new instance each time?
-
Marked as reviewed by jpai (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13099#pullrequestreview-1350037496
PR Review Comment: https://git.openjdk.org/jdk/pull/13099#discussion_r1143113268
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v14]
On Mon, 20 Mar 2023 19:53:29 GMT, Alan Bateman wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fixed SystemModulesClassGenerator.moduleInfos comment > > src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java > line 1021: > >> 1019: MethodTypeDesc.of(CD_void, >> CD_String)) >> 1020:.astore(BUILDER_VAR) >> 1021:.aload(BUILDER_VAR); > > The methods to generate the MD all load the Builder onto the operand stack so > the aload at L1021 is catching my attention, is that needed? It seems to be redundant as the following code does not expect it on stack and loads it again as needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/12944#discussion_r1143070062
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
> API changes for the FFM API (third preview) > > Specdiff: > https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html > > Javadoc: > https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Add example for Option::captureStateLayout - Changes: - all: https://git.openjdk.org/jdk/pull/13079/files - new: https://git.openjdk.org/jdk/pull/13079/files/4626a54e..21ef0607 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13079&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13079&range=03-04 Stats: 12 lines in 1 file changed: 12 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13079.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13079/head:pull/13079 PR: https://git.openjdk.org/jdk/pull/13079
Re: RFR: 8294972: Convert jdk.jlink internal plugins to use the Classfile API [v15]
> jdk.jlink internal plugins are heavily using ASM > > This patch converts ASM calls to Classfile API. > > Please review. > Thanks, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 223 commits: - Merge branch 'master' into JDK-8294972-jlink-plugins # Conflicts: # src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java - fixed SystemModulesClassGenerator.moduleInfos comment - added default to thrown for unknown CP tag in IncludeLocalesPlugin - Merge branch 'master' into JDK-8294972-jlink-plugins - SystemModulesPlugin::genClassBytes rename - SystemModulesPlugin::getClassBytes rename - Revert "implementation of custom ResourceHelper in IncludeLocalesPlugin" - remaining cleanup in SystemModulesPlugin - implementation of custom ResourceHelper in IncludeLocalesPlugin - fixed SystemModulesPlugin - ... and 213 more: https://git.openjdk.org/jdk/compare/c4df9b5f...71ed9a43 - Changes: https://git.openjdk.org/jdk/pull/12944/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12944&range=14 Stats: 1035 lines in 6 files changed: 217 ins; 278 del; 540 mod Patch: https://git.openjdk.org/jdk/pull/12944.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12944/head:pull/12944 PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 00:54:10 GMT, Paul Sandoz wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add example for Option::captureStateLayout
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 621:
>
>> 619: * to a downcall handle linked with {@link
>> #captureCallState(String...)}}
>> 620: *
>> 621: * @see #captureCallState(String...)
>
> How does a caller know what the structure may contain? Should we document the
> platform specific structures?
I've added an example of how to print the platform-dependent structure. Should
we document anyhow?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143055969
Integrated: 8303648: Add String.indexOf(String str, int beginIndex, int endIndex)
On Tue, 7 Mar 2023 14:44:32 GMT, Raffaello Giulietti wrote: > As a followup of [JDK-8302590](https://bugs.openjdk.org/browse/JDK-8302590), > this issue covers the analogous case for a search of a string rather than a > character. This pull request has now been integrated. Changeset: 4bf1fbb0 Author:Raffaello Giulietti URL: https://git.openjdk.org/jdk/commit/4bf1fbb06d63b4c52bfd3922beb2adf069e25b09 Stats: 252 lines in 2 files changed: 237 ins; 5 del; 10 mod 8303648: Add String.indexOf(String str, int beginIndex, int endIndex) Reviewed-by: rriggs - PR: https://git.openjdk.org/jdk/pull/12903
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v4]
> API changes for the FFM API (third preview) > > Specdiff: > https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html > > Javadoc: > https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Improve Linker javadocs - Changes: - all: https://git.openjdk.org/jdk/pull/13079/files - new: https://git.openjdk.org/jdk/pull/13079/files/a71518c4..4626a54e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13079&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13079&range=02-03 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13079.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13079/head:pull/13079 PR: https://git.openjdk.org/jdk/pull/13079
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v4]
On Tue, 21 Mar 2023 00:52:01 GMT, Paul Sandoz wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Improve Linker javadocs
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 609:
>
>> 607: * @see #captureStateLayout()
>> 608: */
>> 609: static Option captureCallState(String... capturedState) {
>
> What if a name is not recognized? Throw IAE?
Added
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143024389
Re: RFR: 8205129: Remove java.lang.Compiler
On Sun, 19 Mar 2023 09:01:26 GMT, Eirik Bjorsnos wrote: > Please help review this PR which suggests we remove the class > `java.lang.Compiler`. The class seems non-functional for a decade, and was > terminally deprecated in Java 9. Time has come to let it go. The CSR for the removal of `java.lang.Compiler` is now looking for non-vacationing reviewers: https://bugs.openjdk.org/browse/JDK-8304458 - PR Comment: https://git.openjdk.org/jdk/pull/13092#issuecomment-1477425915
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v3]
> API changes for the FFM API (third preview) > > Specdiff: > https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html > > Javadoc: > https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Remove MemoryInspection classes - Changes: - all: https://git.openjdk.org/jdk/pull/13079/files - new: https://git.openjdk.org/jdk/pull/13079/files/7a78948a..a71518c4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13079&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13079&range=01-02 Stats: 684 lines in 3 files changed: 0 ins; 684 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13079.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13079/head:pull/13079 PR: https://git.openjdk.org/jdk/pull/13079
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v2]
> API changes for the FFM API (third preview) > > Specdiff: > https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html > > Javadoc: > https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html Per Minborg has updated the pull request incrementally with two additional commits since the last revision: - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/java/lang/foreign/AddressLayout.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/13079/files - new: https://git.openjdk.org/jdk/pull/13079/files/bb2f4438..7a78948a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13079&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13079&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13079.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13079/head:pull/13079 PR: https://git.openjdk.org/jdk/pull/13079
Re: RFR: 8301552: Use AtomicReferenceArray for caching instead of CHM in ZoneOffset [v8]
On Mon, 20 Mar 2023 18:12:01 GMT, Chen Liang wrote:
>> Per Minborg has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Remove unused setup method
>> - Rename method in test
>> - Add copyright header
>
> src/java.base/share/classes/java/time/ZoneOffset.java line 430:
>
>> 428: public static ZoneOffset ofTotalSeconds(int totalSeconds) {
>> 429: final class Holder {
>> 430: private static final IntFunction
>> ZONE_OFFSET_MAPPER = new ZoneOffsetMapper();
>
> Can't the ZoneOffsetMapper itself serve as a holder class? so we move this
> singleton into ZoneOffsetMapper itself.
It is possible but this keeps the mapper more local and only accessible where
it is supposed to be used. For testing purposed, it might be better to have the
class as you propose.
> test/jdk/jdk/internal/util/LazyReferenceArray/BasicLazyReferenceArrayTest.java
> line 107:
>
>> 105:
>> 106: private static IntFunction intIdentity() {
>> 107: return i -> i;
>
> Can't this be `Integer::valueOf`?
It can. I think either works fine. Maybe a bit nicer with a method reference as
you propose.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12346#discussion_r1142972042
PR Review Comment: https://git.openjdk.org/jdk/pull/12346#discussion_r1142973744
Re: RFR: 8301552: Use AtomicReferenceArray for caching instead of CHM in ZoneOffset [v8]
On Tue, 21 Mar 2023 07:27:59 GMT, Per Minborg wrote:
>> test/jdk/jdk/internal/util/LazyReferenceArray/BasicLazyReferenceArrayTest.java
>> line 107:
>>
>>> 105:
>>> 106: private static IntFunction intIdentity() {
>>> 107: return i -> i;
>>
>> Can't this be `Integer::valueOf`?
>
> It can. I think either works fine. Maybe a bit nicer with a method reference
> as you propose.
Perhaps you mean the method could be *replaced* with `Integer::valueOf"? Good
suggestion.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12346#discussion_r1142974996
