RFR: 8281268: Resolve duplication of test ClassTransformer class
ClassTransformer.java is duplicated because ClassTransformer.java was copied from test/jdk/com/sun/jdi/lib/jdb to test/lib/jdk/test/lib/util at [JDK-8240908](https://bugs.openjdk.java.net/browse/JDK-8240908). test/lib/jdk/test/lib/util/ClassTransformer.java should be deleted and test/jdk/com/sun/jdi/lib/jdb/ClassTransformer.java should be used. This is because ClassTransformer is used for testing jdi, and it is not appropriate to exist in test/lib/jdk/test/lib/util. Would you please review this fix? - Commit messages: - 8281268: Resolve duplication of test ClassTransformer class Changes: https://git.openjdk.java.net/jdk/pull/8672/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8672&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281268 Stats: 165 lines in 2 files changed: 0 ins; 163 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8672.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8672/head:pull/8672 PR: https://git.openjdk.java.net/jdk/pull/8672
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v7]
On Wed, 11 May 2022 05:22:22 GMT, Shruthi wrote: >> Removing the Duplicate keys present in XSLTErrorResources.java and >> XPATHErrorResources.java >> >> The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 > > Shruthi has updated the pull request incrementally with one additional commit > since the last revision: > > Modify copyright year `/integrate` - PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems
On Tue, 10 May 2022 12:29:10 GMT, Severin Gehwolf wrote:
> Please review this change to the cgroup v1 subsystem which makes it more
> resilient on some of the stranger systems. Unfortunately, I wasn't able to
> re-create a similar system as the reporter. The idea of using the longest
> substring match for the cgroupv1 file paths was based on the fact that on
> systemd systems processes run in separate scopes and the maven forked test
> runner might exhibit this property. For that it makes sense to use the common
> ancestor path. Nothing changes in the common cases where the `cgroup_path`
> matches `_root` and when the `_root` is `/` (container the former, host
> system the latter).
>
> In addition, the code paths which are susceptible to throw NPE have been
> hardened to catch those situations. Should it happen in the future it makes
> more sense (to me) to not have accurate container detection in favor of
> continuing to keep running.
>
> Finally, with the added unit-tests a bug was uncovered on the "substring"
> match case of cgroup paths in hotspot. `p` returned from `strstr` can never
> point to `_root` as it's used as the "needle" to find in "haystack"
> `cgroup_path` (not the other way round).
>
> Testing:
> - [x] Added unit tests
> - [x] GHA
> - [x] Container tests on cgroups v1 Linux. Continue to pass
I just started to look at the code so just one comment for now.
src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1SubsystemController.java
line 65:
> 63: path = mountPoint + cgroupSubstr;
> 64: }
> 65: } else {
Looks like `path` is still not set if the condition at line 61 `if
(cgroupPath.length() > root.length()) {` is false.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
Alexander > On May 11, 2022, at 11:38 PM, Alexander Matveev > wrote: > > Hi Michael, > >> On May 11, 2022, at 3:17 PM, Michael Hall wrote: >> >> Is this restricted somehow to Mac App Store applications? > Yes, helper tools (in our case JDK native commands) in Mac App Store > applications cannot use same bundle ID as another application. Since we have > bundle ID embedded in native commands multiple applications cannot use it > without updating each native command with unique bundle ID and since they > embedded inside executable I do not see how it would be possible to change > during packaging. Possibly not during packaging but during the jdk build of the native commands? Somehow the Info.plist must be getting embedded and a bundle id provided. I’m not sure where or how. But one way to get uniqueness if the id is provided to embed the Info.plist in the native command executable compile/link would be to include the command as part of the identifier. Something like CFBundleID = java.native.cmd or CFBundleID = javac.native.cmd. >> >> Is a warning issued as stripping native commands may break application >> functionality? > No, error will be issues and jpackage will exit with error if —mac-app-store > arguments is used and provided runtime has native tools or user did not > specified --strip-native-commands to jlink. A warning might be sufficient if the commands are included accidentally by the user unintentionally omitting the —strip-native-commands with the jlink options. Then just force the —strip-native-commands option. It might be the application will just lose some non-critical functionality even if they meant to have the native command included. >> Is it not possible for the user to provide their own Info.plist with a >> different bundle identifier that doesn’t collide? > Not possible due to native commands have embedded Info.plist. Maybe I’m misunderstanding the conflict. Are the commands conflicting with the application level Info.plist or with each other? I believe jpackage usually has a mechanism where a substitute Info.plist can be used if the conflict is with the application level. If multiple commands are conflicting with each other this won’t work. >> >> I’m not real familiar with the build process. But would it be possible for >> the user to build their own jdk that substitutes something else for the >> colliding identifier that gets embedded? > Yes, it should be possible and in theory such JDK with native commands can be > used. However, current fix will not allow such JDK build, since we checking > for presence of “bin” folder and not if ID is actually unique. To work around > this limitation user can package without native commands first, then add > native commands and re-sign application. Signing is currently an integrated part of the process. I thought about making an enhancement request to have it possible to do it stepped. First build the application, then allow the user to post-process that. Maybe run a tool/script to modify the application level Info.plist file. Then allow a second final step that does the signing. I don’t remember if I actually made that enhancement request. But currently there is no way with jpackage to standalone sign the application is there? I think this is somewhat involved with the application bundle being iterated and for one thing each jar being separately code signed? I think it took a release or two of the command to get this correct. So I doubt Xcode could manage it. Is there a way to do the signing standalone with jpackage that I am not aware of? If I didn’t do an enhancement request on this I could, if you think jpackage could manage it? > >> Or just change it in the current build so it doesn’t collide? With the >> application or whatever one it is colliding with. > Not possible, since ID should be unique per application. Possibly you are misunderstanding me here I think you already indicated the could be done ‘in theory’. It still seems possibly the correct ‘fix’. Change the build so uniqueness is not an issue. > > Thanks, > Alexander Thanks, Mike
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
Hi Michael, > On May 11, 2022, at 3:17 PM, Michael Hall wrote: > > Is this restricted somehow to Mac App Store applications? Yes, helper tools (in our case JDK native commands) in Mac App Store applications cannot use same bundle ID as another application. Since we have bundle ID embedded in native commands multiple applications cannot use it without updating each native command with unique bundle ID and since they embedded inside executable I do not see how it would be possible to change during packaging. > > Is a warning issued as stripping native commands may break application > functionality? No, error will be issues and jpackage will exit with error if —mac-app-store arguments is used and provided runtime has native tools or user did not specified --strip-native-commands to jlink. > > Is it not possible for the user to provide their own Info.plist with a > different bundle identifier that doesn’t collide? Not possible due to native commands have embedded Info.plist. > > I’m not real familiar with the build process. But would it be possible for > the user to build their own jdk that substitutes something else for the > colliding identifier that gets embedded? Yes, it should be possible and in theory such JDK with native commands can be used. However, current fix will not allow such JDK build, since we checking for presence of “bin” folder and not if ID is actually unique. To work around this limitation user can package without native commands first, then add native commands and re-sign application. > Or just change it in the current build so it doesn’t collide? With the > application or whatever one it is colliding with. Not possible, since ID should be unique per application. Thanks, Alexander
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]
On Wed, 11 May 2022 16:00:32 GMT, Maxim Kartashev wrote: >> Matthias Baesken has updated the pull request incrementally with one >> additional commit since the last revision: >> >> adjust API level to Windows 8 for security.cpp and do some cleanup > > This change seem to have made this group of declarations obsolete: > `src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h:157` (follow > the [link]( > https://github.com/openjdk/jdk/blob/89de756ffbefac452c7df559e2a4eb50bf71368b/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L157)). > Does anyone have plans to drop those? Have any bugs been filed for that? If > not, I'll attend to that myself. @mkartashev all references to WINVER in the AWT code seem obsolete. Possibly most of the IS_WINxxx usages could also be replaced. - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe [v2]
On Wed, 11 May 2022 21:58:46 GMT, Alexey Semenyuk wrote:
>> Alexander Matveev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8286122: [macos]: App bundle cannot upload to Mac App Store due to
>> info.plist embedded in java exe [v2]
>
> test/jdk/tools/jpackage/macosx/MacAppStoreJlinkOptionsTest.java line 48:
>
>> 46:
>> 47: @Test
>> 48: public static void test() throws Exception {
>
> I'd give some more descriptive names to test functions than `test` and
> `test2`. Something like `testWithStripNativeCommands` and
> `testWithoutStripNativeCommands` maybe?
Fixed.
> test/jdk/tools/jpackage/macosx/MacAppStoreRuntimeTest.java line 102:
>
>> 100:
>> 101: cmd.execute(1);
>> 102: }
>
> @Test
> @Parameter("true")
> @Parameter("false")
> public static void test(boolean stripNativeCommands) throws Exception {
> JPackageCommand cmd = JPackageCommand.helloAppImage();
> cmd.addArguments("--mac-app-store", "--runtime-image",
> getRuntimeImage(stripNativeCommands));
>
> if (stripNativeCommands) {
> cmd.executeAndAssertHelloAppImageCreated();
> } else {
> cmd.execute(1);
> }
> }
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/8666
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe [v2]
> - It is not possible to support native JDK commands such as "java" inside Mac > App Store bundles due to embedded info.plist. Workarounds suggested in > JDK-8286122 does not seems to be visible. > - With proposed fix we will enforce "--strip-native-commands" option for > jlink, so native JDK commands are not included when generating Mac App Store > bundles. > - Custom runtime provided via --runtime-image should not contain native > commands as well, otherwise jpackage will throw error. > - Added two tests to validate fix. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe [v2] - Changes: - all: https://git.openjdk.java.net/jdk/pull/8666/files - new: https://git.openjdk.java.net/jdk/pull/8666/files/4b3c8754..c9ebcee1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8666&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8666&range=00-01 Stats: 16 lines in 2 files changed: 3 ins; 4 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/8666.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8666/head:pull/8666 PR: https://git.openjdk.java.net/jdk/pull/8666
Re: RFR: 8286560: Remove user parameter from jdk.internal.perf.Perf.attach()
On Wed, 11 May 2022 23:08:32 GMT, Ioi Lam wrote: > The API `jdk.internal.perf.Perf.::attach(String user, int lvmid)` is never > used. It should be removed, and all the handling of a specified user name > should be removed. Nice cleanup! I checked back in JDK 7 and couldn't find any use of this particular API. Thanks. - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8669
Re: RFR: 8286279: [vectorapi] Only check index of masked lanes if offset is out of array boundary for masked store
On Thu, 12 May 2022 03:36:31 GMT, Paul Sandoz wrote: >> Thanks for the review @PaulSandoz ! For the >> `VectorIntrinsics.checkFromIndexSize`, I'm afraid it's not suitable to be >> used here because the `outOfBounds` exception will be thrown if the offset >> is not inside of the valid array boundary. And for the masked operations, >> this is not needed since we only need to check the masked lanes. Please >> correct me if I didn't understand correctly. Thanks! > > Silly me! i commented too quickly, `checkFromIndexSize` cannot be used. My > intent was that we could use a method rather than repeating the expression in > numerous places (including masked loads IIRC). Good idea! I will try. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8620
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v3]
On Wed, 11 May 2022 19:45:55 GMT, Paul Sandoz wrote:
> I tried your test code with the patch and logged compilation
> (`-XX:-TieredCompilation -XX:+PrintCompilation -XX:+PrintInlining
> -XX:+PrintIntrinsics -Xbatch`)
>
> For `func` the first call to `VectorSupport::loadMasked` is intrinsic and
> inlined:
>
> ```
> @ 45 jdk.internal.vm.vector.VectorSupport::loadMasked (40 bytes)
> (intrinsic)
> ```
>
> But the second call (for the last loop iteration) fails to inline:
>
> ```
> @ 45 jdk.internal.vm.vector.VectorSupport::loadMasked (40 bytes)
> failed to inline (intrinsic)
> ```
>
> Since i am running on an mac book this looks right and aligns with the
> `-XX:+PrintIntrinsics` output:
>
> ```
> ** Rejected vector op (LoadVectorMasked,int,8) because architecture does
> not support it
> ** Rejected vector op (LoadVectorMasked,int,8) because architecture does
> not support it
> ** not supported: op=loadMasked vlen=8 etype=int using_byte_array=0
> ```
>
> ?
>
> I have not looked at the code gen nor measured performance comparing the case
> when never out of bounds and only out of bounds for the last loop iteration.
Yeah, it looks right from the log. Did you try to find whether there is the log
`** missing constant: offsetInRange=Parm` with `XX:+PrintIntrinsics` ? Or
insert an assertion in `vectorIntrinsics.cpp` like:
--- a/src/hotspot/share/opto/vectorIntrinsics.cpp
+++ b/src/hotspot/share/opto/vectorIntrinsics.cpp
@@ -1236,6 +1236,7 @@ bool
LibraryCallKit::inline_vector_mem_masked_operation(bool is_store) {
} else {
// Masked vector load with IOOBE always uses the predicated load.
const TypeInt* offset_in_range = gvn().type(argument(8))->isa_int();
+ assert(offset_in_range->is_con(), "must be a constant");
if (!offset_in_range->is_con()) {
if (C->print_intrinsics()) {
tty->print_cr(" ** missing constant: offsetInRange=%s",
And run the tests with debug mode. Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8286279: [vectorapi] Only check index of masked lanes if offset is out of array boundary for masked store
On Thu, 12 May 2022 01:56:25 GMT, Xiaohong Gong wrote:
>> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-Vector.java.template
>> line 4086:
>>
>>> 4084: } else {
>>> 4085: $Type$Species vsp = vspecies();
>>> 4086: if (offset < 0 || offset > (a.length - vsp.length())) {
>>
>> Can we use `VectorIntrinsics.checkFromIndexSize`? e.g.
>>
>> if (!VectorIntrinsics.checkFromIndexSize(offset, vsp.length(), a.length)) {
>> ...
>
> Thanks for the review @PaulSandoz ! For the
> `VectorIntrinsics.checkFromIndexSize`, I'm afraid it's not suitable to be
> used here because the `outOfBounds` exception will be thrown if the offset is
> not inside of the valid array boundary. And for the masked operations, this
> is not needed since we only need to check the masked lanes. Please correct me
> if I didn't understand correctly. Thanks!
Silly me! i commented too quickly, `checkFromIndexSize` cannot be used. My
intent was that we could use a method rather than repeating the expression in
numerous places (including masked loads IIRC).
-
PR: https://git.openjdk.java.net/jdk/pull/8620
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Wed, 11 May 2022 20:27:42 GMT, Patricio Chilano Mateo wrote: >> I went ahead and implemented this suggestion. Now we block async exceptions >> in on_entry, and unblock in on_exit. > > Is it possible for these upcalls to be nested? If yes, we could add a boolean > to context to avoid unsetting the flag in those nested cases. And now that I > think we should probably add that check in NoAsyncExceptionDeliveryMark too > if we allow broader use of this flag. David added the > NoAsyncExceptionDeliveryMark code with that assert about nesting so maybe he > might have more insights about that. NoAsyncExceptionDeliveryMark is not for general use! There is no provision for blocking async exceptions when running user-defined Java code. NoAsyncExceptionDeliveryMark was purely for protecting "system Java code". - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v10]
On Wed, 11 May 2022 17:51:31 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR updates the VM implementation of the foreign linker, by bringing >> over commits from the panama-foreign repo. >> >> This is split off from the main JEP integration for 19, since we have >> limited resources to handle this. As such, this PR might fall over to 20, >> but it would be nice if we could get it into 19. >> >> I've written up an overview of the Linker architecture here: >> http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful >> to read that first. >> >> This patch moves from the "legacy" implementation, to what is currently >> implemented in the panama-foreign repo, except for replacing the use of >> method handle combinators with ASM. That will come in a later path. To >> recap. This PR contains the following changes: >> >> 1. VM stubs for downcalls are now generated up front, instead of lazily by >> C2 [1]. >> 2. the VM support for upcalls/downcalls now support all possible call >> shapes. And VM stubs and Java code implementing the buffered invocation >> strategy has been removed [2], [3], [4], [5]. >> 3. The existing C2 intrinsification support for the `linkToNative` method >> handle linker was no longer needed and has been removed [6] (support might >> be re-added in another form later). >> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now >> implements RuntimeBlob directly. Binding to java classes has been rewritten >> to use javaClasses.h/cpp (this wasn't previously possible due to these java >> classes being in an incubator module) [7], [8], [9]. >> >> While the patch mostly consists of VM changes, there are also some Java >> changes to support (2). >> >> The original commit structure has been mostly retained, so it might be >> useful to look at a specific commit, or the corresponding patch in the >> [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) >> repo as well. I've also left some inline comments to explain some of the >> changes, which will hopefully make reviewing easier. >> >> Testing: Tier1-4 >> >> Thanks, >> Jorn >> >> [1]: >> https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 >> [2]: >> https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 >> [3]: >> https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 >> [4]: >> https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 >> [5]: >> https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 >> [6]: >> https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a >> [7]: >> https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 >> [8]: >> https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f >> [9]: >> https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 26 commits: > > - Block async exceptions during upcalls > - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 > - Fix use of rt_call > - Migrate to GrowableArray > - Address some review comments > - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 > - Remove unneeded ComputeMoveOrder > - Remove comment about native calls in lcm.cpp > - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 > >Reviewed-by: jvernee, mcimadamore > - Update riscv and arm stubs > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/cdd006e7...b29ad8f4 src/hotspot/cpu/aarch64/foreign_globals_aarch64.cpp line 3: > 1: /* > 2: * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights > reserved. > 3: * Copyright (c) 2019, 2021, Arm Limited. All rights reserved. Only update third-party copyrights under direction from that copyright holder. This may not be the correct format for example. Also it is 2022. :) - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:24:38 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which fixes build failures on macos >> when using `--with-zlib=bundled`? >> >> With this change the build now passes (tested both with bundled and system >> zlib variants). >> >> tier1, tier2 and tier3 testing has been done and no related failures have >> been noticed. > > Jaikiran Pai has updated the pull request incrementally with four additional > commits since the last revision: > > - copyright years > - disable format-nonliteral warning when building LIBSPLASHSCREEN with > bundled zlib > - Magnus' suggestion - make the LIBZ_CFLAGS more readable in the build file > - Magnus' suggestion - Disable format-nonliteral in build section of zlib > instead of source code Thank you Lance and Magnus for the reviews and inputs. I've triggered various build and test runs with this state of the PR. Once those complete satisfactorily, I'll go ahead and integrate this. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: JDK-8286615: Small refactor to SerializedLambda
On Thu, 12 May 2022 00:10:28 GMT, Joe Darcy wrote: > Noticed by inspection during a CSR review, small refactoring to use a > message-cause exception constructor when one is available. > > Will update the copyright before a push. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8670
Re: RFR: 8286279: [vectorapi] Only check index of masked lanes if offset is out of array boundary for masked store
On Wed, 11 May 2022 15:10:55 GMT, Paul Sandoz wrote:
>> Checking whether the indexes of masked lanes are inside of the valid memory
>> boundary is necessary for masked vector memory access. However, this could
>> be saved if the given offset is inside of the vector range that could make
>> sure no IOOBE (IndexOutOfBoundaryException) happens. The masked load APIs
>> have saved this kind of check for common cases. And this patch did the
>> similar optimization for the masked vector store.
>>
>> The performance for the new added store masked benchmarks improves about
>> `1.83x ~ 2.62x` on a x86 system:
>>
>> Benchmark BeforeAfter Gain Units
>> StoreMaskedBenchmark.byteStoreArrayMask 12757.936 23291.118 1.826 ops/ms
>> StoreMaskedBenchmark.doubleStoreArrayMask 1520.932 3921.616 2.578 ops/ms
>> StoreMaskedBenchmark.floatStoreArrayMask 2713.031 7122.535 2.625 ops/ms
>> StoreMaskedBenchmark.intStoreArrayMask 4113.772 8220.206 1.998 ops/ms
>> StoreMaskedBenchmark.longStoreArrayMask1993.986 4874.148 2.444 ops/ms
>> StoreMaskedBenchmark.shortStoreArrayMask 8543.593 17821.086 2.086 ops/ms
>>
>> Similar performane gain can also be observed on ARM hardware.
>
> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-Vector.java.template
> line 4086:
>
>> 4084: } else {
>> 4085: $Type$Species vsp = vspecies();
>> 4086: if (offset < 0 || offset > (a.length - vsp.length())) {
>
> Can we use `VectorIntrinsics.checkFromIndexSize`? e.g.
>
> if (!VectorIntrinsics.checkFromIndexSize(offset, vsp.length(), a.length)) {
> ...
Thanks for the review @PaulSandoz ! For the
`VectorIntrinsics.checkFromIndexSize`, I'm afraid it's not suitable to be used
here because the `outOfBounds` exception will be thrown if the offset is not
inside of the valid array boundary. And for the masked operations, this is not
needed since we only need to check the masked lanes. Please correct me if I
didn't understand correctly. Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/8620
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]
On Thu, 12 May 2022 01:27:30 GMT, Yasumasa Suenaga wrote: >> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 >> on Fedora 36. >> As you can see, the warnings spreads several areas. Let me know if I should >> separate them by area. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Calculate char offset before realloc() Thanks for all to review this PR! I think we should separate this issue as following: * Suppress warnings * make/modules/java.desktop/lib/Awt2dLibraries.gmk * src/hotspot/share/classfile/bytecodeAssembler.cpp * src/hotspot/share/classfile/classFileParser.cpp * src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp * src/hotspot/share/opto/memnode.cpp * src/hotspot/share/opto/type.cpp * src/hotspot/share/utilities/compilerWarnings.hpp * src/hotspot/share/utilities/compilerWarnings_gcc.hpp * src/java.base/unix/native/libjli/java_md_common.c * Bug fixes * src/java.base/share/native/libjli/java.c * src/java.base/share/native/libjli/parse_manifest.c * src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c I want to include the change of Awt2dLibraries.gmk (HarfBuzz) in this PR because it is 3rd party library. I will separate in above if I do not hear any objections, and this issue (PR) handles "suppress warnings" only. - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:47:43 GMT, Kim Barrett wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid pragma error in before GCC 12 > > src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c line 193: > >> 191: #if defined(__GNUC__) && __GNUC__ >= 12 >> 192: #pragma GCC diagnostic pop >> 193: #endif > > Rather than all this warning suppression cruft and the comment explaining why > it's okay, just compute the `(strBufNextChar - strBufBegin)` offset a few > lines earlier, before the realloc. I did do that in new commit, and the warning has gone! - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]
> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on > Fedora 36. > As you can see, the warnings spreads several areas. Let me know if I should > separate them by area. > > * -Wstringop-overflow > * src/hotspot/share/oops/array.hpp > * > src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp > > In member function 'void Array::at_put(int, const T&) [with T = unsigned > char]', > inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, > inlined from 'void ConstantPool::method_at_put(int, int, int)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, > inlined from 'ConstantPool* > BytecodeConstantPool::create_constant_pool(JavaThread*) const' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision: Calculate char offset before realloc() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8646/files - new: https://git.openjdk.java.net/jdk/pull/8646/files/8d608414..b3afa3e0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8646&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8646&range=02-03 Stats: 18 lines in 1 file changed: 3 ins; 14 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8646.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646 PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:43:55 GMT, Kim Barrett wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid pragma error in before GCC 12 > > src/java.base/unix/native/libjli/java_md_common.c line 135: > >> 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name)) >> return 0; >> 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, >> cmd); >> 135: #pragma GCC diagnostic pop > > Wouldn't it be better to just call JLI_Snprintf without the precheck and > check the result to determine whether it was successful or was truncated? I > think that kind of check is supposed to make gcc's truncation checker happy. The warning has gone when using return value from `JLI_Snprintf()` in new commit! - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v3]
> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on > Fedora 36. > As you can see, the warnings spreads several areas. Let me know if I should > separate them by area. > > * -Wstringop-overflow > * src/hotspot/share/oops/array.hpp > * > src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp > > In member function 'void Array::at_put(int, const T&) [with T = unsigned > char]', > inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, > inlined from 'void ConstantPool::method_at_put(int, int, int)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, > inlined from 'ConstantPool* > BytecodeConstantPool::create_constant_pool(JavaThread*) const' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision: Use return value from JLI_Snprintf - Changes: - all: https://git.openjdk.java.net/jdk/pull/8646/files - new: https://git.openjdk.java.net/jdk/pull/8646/files/7f155256..8d608414 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8646&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8646&range=01-02 Stats: 11 lines in 1 file changed: 4 ins; 5 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8646.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646 PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 14:27:27 GMT, Kim Barrett wrote:
>> src/java.base/share/native/libjli/java.c line 1629:
>>
>>> 1627: const char *arg = jargv[i];
>>> 1628: if (arg[0] == '-' && arg[1] == 'J') {
>>> 1629: *nargv++ = (arg[2] == '\0') ? NULL : JLI_StringDup(arg +
>>> 2);
>>
>> Wow!
>
> I wonder if the client expects NULL strings in the result, or if the NULL
> value should be an empty string? If empty strings are okay, this would be
> simpler without the conditional, just dup from arg + 2 to the terminating
> byte (which might be immediate).
`NULL` affects as a loop stopper in `ParseArguments()` as following:
static jboolean
ParseArguments(int *pargc, char ***pargv,
int *pmode, char **pwhat,
int *pret, const char *jrepath)
{
int argc = *pargc;
char **argv = *pargv;
int mode = LM_UNKNOWN;
char *arg;
*pret = 0;
while ((arg = *argv) != 0 && *arg == '-') {
But I'm not sure it is valid, I think it might be discussed as another issue.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: JDK-8286615: Small refactor to SerializedLambda
On Thu, 12 May 2022 00:10:28 GMT, Joe Darcy wrote: > Noticed by inspection during a CSR review, small refactoring to use a > message-cause exception constructor when one is available. > > Will update the copyright before a push. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8670
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:35:43 GMT, Kim Barrett wrote:
>> Yasumasa Suenaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid pragma error in before GCC 12
>
> src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 51:
>
>> 49:
>> 50: #define PRAGMA_STRINGOP_OVERFLOW_IGNORED \
>> 51: PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow")
>
> Are the reported problems with format/stringop-overflow real? Or are they
> false positives? If they are real then I'm not going to approve ignoring them,
> unless there is some urgent reason and there are followup bug reports for
> fixing them.
I think all of warnings in HotSpot are false-positives, so I added new pragmas
to compilerWarnings.hpp, and use it in HotSpot code.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8277493: [REDO] Quarantined jpackage apps are labeled as "damaged"
On Wed, 4 May 2022 03:56:10 GMT, Alexander Matveev wrote: > - No changes to code provided by original fix. > - Added ad hoc signing on macOS aarch64, since macOS aarch64 cannot execute > unsigned code and code should be at least ad hoc signed. > - Signing of app bundle produced by AppContentTest.java fails, since it has > invalid app bundle structure with added files into Content folder. Thus test > is executed but failure is always expected on macOS aarch64. Any chance logic traversing runtime and adjusting the permissions of its files in `unsignAppBundle()` and ` signAdHocAppBundle()` can be moved to a single function instead of being duplicated in both of them? - PR: https://git.openjdk.java.net/jdk/pull/8527
Re: RFR: 8277493: [REDO] Quarantined jpackage apps are labeled as "damaged"
On Wed, 4 May 2022 03:56:10 GMT, Alexander Matveev wrote:
> - No changes to code provided by original fix.
> - Added ad hoc signing on macOS aarch64, since macOS aarch64 cannot execute
> unsigned code and code should be at least ad hoc signed.
> - Signing of app bundle produced by AppContentTest.java fails, since it has
> invalid app bundle structure with added files into Content folder. Thus test
> is executed but failure is always expected on macOS aarch64.
test/jdk/tools/jpackage/macosx/SigningAppImageTest.java line 72:
> 70: @Parameters
> 71: public static List data() {
> 72: return List.of(new Object[][] {{"true"}, {"false"}});
I'd replace use of `@Parameters` with `@Parameter` as follows:
@Test
@Parameter("true")
@Parameter("false")
public static void test(boolean doSign) {...}
-
PR: https://git.openjdk.java.net/jdk/pull/8527
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 19:11:16 GMT, Phil Race wrote: >> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 462: >> >>> 460:HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits >>> missing-field-initializers strict-aliasing >>> 461:HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder >>> delete-non-virtual-dtor strict-overflow \ >>> 462: maybe-uninitialized class-memaccess unused-result extra >>> use-after-free >> >> Globally disabling use-after-free warnings for this package seems really >> questionable. If these are problems in the code, just suppressing the warning >> and leaving the problems to bite us seems like a bad idea. And the problems >> ought to be reported upstream to the HarfBuzz folks. > > I don't understand what the actual warning is getting at .. can anyone > explain it ? > > FWIW the code is still the same in upstream harfbuzz > https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-font.cc I've pasted a part of warning messages to description of this PR, all of messages are here: In function 'void trampoline_reference(hb_trampoline_closure_t*)', inlined from 'void hb_font_funcs_set_glyph_func(hb_font_funcs_t*, hb_font_get_glyph_func_t, void*, hb_destroy_func_t)' at /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2368:24: - PR: https://git.openjdk.java.net/jdk/pull/8646
RFR: JDK-8286615: Small refactor to SerializedLambda
Noticed by inspection during a CSR review, small refactoring to use a message-cause exception constructor when one is available. Will update the copyright before a push. - Commit messages: - JDK-8286615: Small refactor to SerializedLambda Changes: https://git.openjdk.java.net/jdk/pull/8670/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8670&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286615 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8670.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8670/head:pull/8670 PR: https://git.openjdk.java.net/jdk/pull/8670
RFR: 8286560: Remove user parameter from jdk.internal.perf.Perf.attach()
The API `jdk.internal.perf.Perf.::attach(String user, int lvmid)` is never used. It should be removed, and all the handling of a specified user name should be removed. - Commit messages: - more cleanup - 8286560: Remove user parameter from jdk.internal.perf.Perf.attach() Changes: https://git.openjdk.java.net/jdk/pull/8669/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8669&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286560 Stats: 120 lines in 5 files changed: 2 ins; 95 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/8669.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8669/head:pull/8669 PR: https://git.openjdk.java.net/jdk/pull/8669
Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec
On Wed, 11 May 2022 20:40:30 GMT, Joe Darcy wrote:
> While doing a CSR review of another issue, I noticed some cases in
> InputStream and OutputStream what would benefit from being upgraded to
> implSpec and related javadoc tags.
>
> The "A subclass must provide an implementation of this method." statements on
> several abstract methods don't add much value, but I chose to leave them in
> for this request.
>
> Please also review the corresponding CSR:
> https://bugs.openjdk.java.net/browse/JDK-8286605
src/java.base/share/classes/java/io/OutputStream.java line 151:
> 149: * The {@code write} method of {@code OutputStream} calls
> 150: * the write method of one argument on each of the bytes to be
> 151: * written out. Subclasses are encouraged to override this method and
Shouldn't the "subclasses" information be part of the API Note?
-
PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 22:03:38 GMT, Magnus Ihse Bursie wrote: > It would not make sense to set the disabled warning in the configure script, > no. The current code looks perfectly fine. Disabled warnings per module are > set in the makefiles. OK, that you for your feedback regarding the makefile changes vs. configure script. > > If you feel strongly that this needs to be documented more than in the JBS > issue and this PR review, updating the zlib ChangeLog file is probably the > way to go. I have no strong opinion on that. But from the build system PoV, > the current code is fine as it is to be committed. OK, I am probably overthinking the need to document this - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:24:38 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which fixes build failures on macos >> when using `--with-zlib=bundled`? >> >> With this change the build now passes (tested both with bundled and system >> zlib variants). >> >> tier1, tier2 and tier3 testing has been done and no related failures have >> been noticed. > > Jaikiran Pai has updated the pull request incrementally with four additional > commits since the last revision: > > - copyright years > - disable format-nonliteral warning when building LIBSPLASHSCREEN with > bundled zlib > - Magnus' suggestion - make the LIBZ_CFLAGS more readable in the build file > - Magnus' suggestion - Disable format-nonliteral in build section of zlib > instead of source code Thanks Jai for the latest tweaks to the our original patch. I think we should be good to go - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
> On May 11, 2022, at 5:17 PM, Michael Hall wrote: > > I’m not real familiar with the build process. But would it be possible for > the user to build their own jdk that substitutes something else for the > colliding identifier that gets embedded? Or just change it in the current build so it doesn’t collide? With the application or whatever one it is colliding with.
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
Is this restricted somehow to Mac App Store applications? Is a warning issued as stripping native commands may break application functionality? Is it not possible for the user to provide their own Info.plist with a different bundle identifier that doesn’t collide? I’m not real familiar with the build process. But would it be possible for the user to build their own jdk that substitutes something else for the colliding identifier that gets embedded?
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 19:14:54 GMT, Lance Andersen wrote: >> make/autoconf/lib-bundled.m4 line 220: >> >>> 218: if test "x$USE_EXTERNAL_LIBZ" = "xfalse"; then >>> 219: LIBZ_CFLAGS="$LIBZ_CFLAGS >>> -I$TOPDIR/src/java.base/share/native/libzip/zlib" >>> 220: if test "x$OPENJDK_TARGET_OS" = xmacosx; then >> >> Please add a comment here as to why we are doing this > >> @LanceAndersen Is that really needed? We normally don't comment on the >> reason why certain code needs certain defines. We tried to document some >> compiler flags in the beginning, but it turned out that most command lines >> ended up as essays, and this were not helpful. I think it's quite obvious >> what this code does: libz requires the define HAVE_UNISTD_H on macOS. I'm >> not even sure how you'd formulate a "why" -- "otherwise it does not work >> properly"? > > The zlib configure script, which needs to be run prior running make to > build the upstream repository, will determine if HAVE_UNISTD_H is needed and > generate zconf.h replacing the macro with "1".My reason for adding a > comment as this is a variant from how zlib is built upstream. Perhaps just > updating open/src/java.base/share/native/libzip/zlib/ChangeLog is enough. I > was just trying to document why we are doing this. > > > Another question would it make sense to set LIBZ_DISABLED_WARNINGS_CLANG in > make/autoconf/lib-bundled.m4 so that the value in the case of zlib is set in > one location? In addition, I can log a request to address this in the > upstream project so we do not need to worry about this warning going forward. It would not make sense to set the disabled warning in the configure script, no. The current code looks perfectly fine. Disabled warnings per module are set in the makefiles. If you feel strongly that this needs to be documented more than in the JBS issue and this PR review, updating the zlib ChangeLog file is probably the way to go. I have no strong opinion on that. But from the build system PoV, the current code is fine as it is to be committed. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
On Wed, 11 May 2022 21:31:44 GMT, Alexander Matveev
wrote:
> - It is not possible to support native JDK commands such as "java" inside Mac
> App Store bundles due to embedded info.plist. Workarounds suggested in
> JDK-8286122 does not seems to be visible.
> - With proposed fix we will enforce "--strip-native-commands" option for
> jlink, so native JDK commands are not included when generating Mac App Store
> bundles.
> - Custom runtime provided via --runtime-image should not contain native
> commands as well, otherwise jpackage will throw error.
> - Added two tests to validate fix.
test/jdk/tools/jpackage/macosx/MacAppStoreJlinkOptionsTest.java line 48:
> 46:
> 47: @Test
> 48: public static void test() throws Exception {
I'd give some more descriptive names to test functions than `test` and `test2`.
Something like `testWithStripNativeCommands` and
`testWithoutStripNativeCommands` maybe?
-
PR: https://git.openjdk.java.net/jdk/pull/8666
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
On Wed, 11 May 2022 21:31:44 GMT, Alexander Matveev
wrote:
> - It is not possible to support native JDK commands such as "java" inside Mac
> App Store bundles due to embedded info.plist. Workarounds suggested in
> JDK-8286122 does not seems to be visible.
> - With proposed fix we will enforce "--strip-native-commands" option for
> jlink, so native JDK commands are not included when generating Mac App Store
> bundles.
> - Custom runtime provided via --runtime-image should not contain native
> commands as well, otherwise jpackage will throw error.
> - Added two tests to validate fix.
Changes requested by asemenyuk (Reviewer).
test/jdk/tools/jpackage/macosx/MacAppStoreRuntimeTest.java line 102:
> 100:
> 101: cmd.execute(1);
> 102: }
@Test
@Parameter("true")
@Parameter("false")
public static void test(boolean stripNativeCommands) throws Exception {
JPackageCommand cmd = JPackageCommand.helloAppImage();
cmd.addArguments("--mac-app-store", "--runtime-image",
getRuntimeImage(stripNativeCommands));
if (stripNativeCommands) {
cmd.executeAndAssertHelloAppImageCreated();
} else {
cmd.execute(1);
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/8666
RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
- It is not possible to support native JDK commands such as "java" inside Mac App Store bundles due to embedded info.plist. Workarounds suggested in JDK-8286122 does not seems to be visible. - With proposed fix we will enforce "--strip-native-commands" option for jlink, so native JDK commands are not included when generating Mac App Store bundles. - Custom runtime provided via --runtime-image should not contain native commands as well, otherwise jpackage will throw error. - Added two tests to validate fix. - Commit messages: - 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe Changes: https://git.openjdk.java.net/jdk/pull/8666/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8666&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286122 Stats: 223 lines in 7 files changed: 217 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8666.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8666/head:pull/8666 PR: https://git.openjdk.java.net/jdk/pull/8666
Re: Unused method sun.reflect.misc.MethodUtil.getPublicMethods
They are unused and leftover since JDK 7. It's good to remove them. Thanks Mandy On 4/30/22 7:48 AM, Andrey Turbanov wrote: Hello. I found a few methods in MethodUtil class which seem unused to me: getPublicMethods, getInterfaceMethods, getInternalPublicMethods ,addMethod. Are they somehow used by VM, or is it just leftovers from some refactorings? I wonder if we can drop them. Andrey Turbanov
Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec
On Wed, 11 May 2022 20:40:30 GMT, Joe Darcy wrote: > While doing a CSR review of another issue, I noticed some cases in > InputStream and OutputStream what would benefit from being upgraded to > implSpec and related javadoc tags. > > The "A subclass must provide an implementation of this method." statements on > several abstract methods don't add much value, but I chose to leave them in > for this request. > > Please also review the corresponding CSR: > https://bugs.openjdk.java.net/browse/JDK-8286605 Associated CSR also reviewed. - Marked as reviewed by iris (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec
On Wed, 11 May 2022 20:40:30 GMT, Joe Darcy wrote: > While doing a CSR review of another issue, I noticed some cases in > InputStream and OutputStream what would benefit from being upgraded to > implSpec and related javadoc tags. > > The "A subclass must provide an implementation of this method." statements on > several abstract methods don't add much value, but I chose to leave them in > for this request. > > Please also review the corresponding CSR: > https://bugs.openjdk.java.net/browse/JDK-8286605 Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec
On Wed, 11 May 2022 20:40:30 GMT, Joe Darcy wrote: > While doing a CSR review of another issue, I noticed some cases in > InputStream and OutputStream what would benefit from being upgraded to > implSpec and related javadoc tags. > > The "A subclass must provide an implementation of this method." statements on > several abstract methods don't add much value, but I chose to leave them in > for this request. > > Please also review the corresponding CSR: > https://bugs.openjdk.java.net/browse/JDK-8286605 Looks fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF
On Wed, 11 May 2022 20:47:52 GMT, Brian Burkhalter wrote: > Modify the specification of `SequenceInputStream.read(byte[],int,int)` to > indicate that `-1` is returned at the EOF of the last stream even if `len` is > zero. The `InputStream.read(byte[],int,int)` specification indicates If len is zero, then no bytes are read and 0 is returned; otherwise, there is an attempt to read at least one byte. If no byte is available because the stream is at end of file, the value -1 is returned; otherwise, at least one byte is read and stored into b. so that zero should be returned if `len` is zero regardless of anything else. `SequenceInputStream` does not follow this so its specification should be modified to document the existing, longstanding behavior. A CSR will be filed once there is consensus here. - PR: https://git.openjdk.java.net/jdk/pull/8664
RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF
Modify the specification of `SequenceInputStream.read(byte[],int,int)` to indicate that `-1` is returned at the EOF of the last stream even if `len` is zero. - Commit messages: - 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF Changes: https://git.openjdk.java.net/jdk/pull/8664/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8664&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286200 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8664.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8664/head:pull/8664 PR: https://git.openjdk.java.net/jdk/pull/8664
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]
On Wed, 11 May 2022 20:04:39 GMT, Naoto Sato wrote: >> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support >> second-level resolution, enabling round trips with `java.time.ZoneOffset`s. >> Corresponding CSR is also being drafted. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - Disallow `UTC` -> `GMT` > - Merge branch 'master' into JDK-8285844 > - Minor fixup > - Allowed round-trips for offset-style ZoneIds. > - Fixed offsets in milliseconds, added test variations, refined Custom ID > definitions > - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all > ZoneOffsets and returns GMT unexpected Marked as reviewed by uschindler (Author). - PR: https://git.openjdk.java.net/jdk/pull/8606
RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec
While doing a CSR review of another issue, I noticed some cases in InputStream and OutputStream what would benefit from being upgraded to implSpec and related javadoc tags. The "A subclass must provide an implementation of this method." statements on several abstract methods don't add much value, but I chose to leave them in for this request. Please also review the corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8286605 - Commit messages: - JDK-8286604: Update InputStream and OutputStream to use @implSpec Changes: https://git.openjdk.java.net/jdk/pull/8663/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8663&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286604 Stats: 36 lines in 2 files changed: 18 ins; 4 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8663.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8663/head:pull/8663 PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]
On Wed, 11 May 2022 20:04:39 GMT, Naoto Sato wrote: >> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support >> second-level resolution, enabling round trips with `java.time.ZoneOffset`s. >> Corresponding CSR is also being drafted. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - Disallow `UTC` -> `GMT` > - Merge branch 'master' into JDK-8285844 > - Minor fixup > - Allowed round-trips for offset-style ZoneIds. > - Fixed offsets in milliseconds, added test variations, refined Custom ID > definitions > - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all > ZoneOffsets and returns GMT unexpected Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Wed, 11 May 2022 17:55:16 GMT, Jorn Vernee wrote: >> Oh nice! I was just thinking that the only possible way out of this >> conundrum would be to somehow block the delivery of async exceptions (at >> least outside of the user's exception handler). So, that seems to be exactly >> what we need :) > > I went ahead and implemented this suggestion. Now we block async exceptions > in on_entry, and unblock in on_exit. Is it possible for these upcalls to be nested? If yes, we could add a boolean to context to avoid unsetting the flag in those nested cases. And now that I think we should probably add that check in NoAsyncExceptionDeliveryMark too if we allow broader use of this flag. David added the NoAsyncExceptionDeliveryMark code with that assert about nesting so maybe he might have more insights about that. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]
On Wed, 11 May 2022 20:04:39 GMT, Naoto Sato wrote: >> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support >> second-level resolution, enabling round trips with `java.time.ZoneOffset`s. >> Corresponding CSR is also being drafted. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - Disallow `UTC` -> `GMT` > - Merge branch 'master' into JDK-8285844 > - Minor fixup > - Allowed round-trips for offset-style ZoneIds. > - Fixed offsets in milliseconds, added test variations, refined Custom ID > definitions > - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all > ZoneOffsets and returns GMT unexpected Marked as reviewed by scolebourne (Author). - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support > second-level resolution, enabling round trips with `java.time.ZoneOffset`s. > Corresponding CSR is also being drafted. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Disallow `UTC` -> `GMT` - Merge branch 'master' into JDK-8285844 - Minor fixup - Allowed round-trips for offset-style ZoneIds. - Fixed offsets in milliseconds, added test variations, refined Custom ID definitions - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected - Changes: - all: https://git.openjdk.java.net/jdk/pull/8606/files - new: https://git.openjdk.java.net/jdk/pull/8606/files/9722decd..11150ac5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8606&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8606&range=03-04 Stats: 8626 lines in 248 files changed: 4866 ins; 2323 del; 1437 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v4]
On Wed, 11 May 2022 18:30:32 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Minor fixup
>
> src/java.base/share/classes/java/util/TimeZone.java line 80:
>
>> 78: * {@code GMT} Sign Hours {@code :} Minutes
>> {@code :} Seconds
>> 79: * {@code GMT} Sign Hours {@code :} Minutes
>> 80: * {@code GMT} Sign Hours Minutes
>
> For hours and minutes, the format can be hh:mm or hhmm. Is it worth it to
> support hhmmss as well?
It would be consistent, but I don't see much requirement for it. I'll consider
if someone needs it.
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v3]
On Thu, 5 May 2022 08:56:07 GMT, Xiaohong Gong wrote:
>> Currently the vector load with mask when the given index happens out of the
>> array boundary is implemented with pure java scalar code to avoid the IOOBE
>> (IndexOutOfBoundaryException). This is necessary for architectures that do
>> not support the predicate feature. Because the masked load is implemented
>> with a full vector load and a vector blend applied on it. And a full vector
>> load will definitely cause the IOOBE which is not valid. However, for
>> architectures that support the predicate feature like SVE/AVX-512/RVV, it
>> can be vectorized with the predicated load instruction as long as the
>> indexes of the masked lanes are within the bounds of the array. For these
>> architectures, loading with unmasked lanes does not raise exception.
>>
>> This patch adds the vectorization support for the masked load with IOOBE
>> part. Please see the original java implementation (FIXME: optimize):
>>
>>
>> @ForceInline
>> public static
>> ByteVector fromArray(VectorSpecies species,
>>byte[] a, int offset,
>>VectorMask m) {
>> ByteSpecies vsp = (ByteSpecies) species;
>> if (offset >= 0 && offset <= (a.length - species.length())) {
>> return vsp.dummyVector().fromArray0(a, offset, m);
>> }
>>
>> // FIXME: optimize
>> checkMaskFromIndexSize(offset, vsp, m, 1, a.length);
>> return vsp.vOp(m, i -> a[offset + i]);
>> }
>>
>> Since it can only be vectorized with the predicate load, the hotspot must
>> check whether the current backend supports it and falls back to the java
>> scalar version if not. This is different from the normal masked vector load
>> that the compiler will generate a full vector load and a vector blend if the
>> predicate load is not supported. So to let the compiler make the expected
>> action, an additional flag (i.e. `usePred`) is added to the existing
>> "loadMasked" intrinsic, with the value "true" for the IOOBE part while
>> "false" for the normal load. And the compiler will fail to intrinsify if the
>> flag is "true" and the predicate load is not supported by the backend, which
>> means that normal java path will be executed.
>>
>> Also adds the same vectorization support for masked:
>> - fromByteArray/fromByteBuffer
>> - fromBooleanArray
>> - fromCharArray
>>
>> The performance for the new added benchmarks improve about `1.88x ~ 30.26x`
>> on the x86 AVX-512 system:
>>
>> Benchmark before After Units
>> LoadMaskedIOOBEBenchmark.byteLoadArrayMaskIOOBE 737.542 1387.069 ops/ms
>> LoadMaskedIOOBEBenchmark.doubleLoadArrayMaskIOOBE 118.366 330.776 ops/ms
>> LoadMaskedIOOBEBenchmark.floatLoadArrayMaskIOOBE 233.832 6125.026 ops/ms
>> LoadMaskedIOOBEBenchmark.intLoadArrayMaskIOOBE233.816 7075.923 ops/ms
>> LoadMaskedIOOBEBenchmark.longLoadArrayMaskIOOBE 119.771 330.587 ops/ms
>> LoadMaskedIOOBEBenchmark.shortLoadArrayMaskIOOBE 431.961 939.301 ops/ms
>>
>> Similar performance gain can also be observed on 512-bit SVE system.
>
> Xiaohong Gong has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rename "use_predicate" to "needs_predicate"
I tried your test code with the patch and logged compilation
(`-XX:-TieredCompilation -XX:+PrintCompilation -XX:+PrintInlining
-XX:+PrintIntrinsics -Xbatch`)
For `func` the first call to `VectorSupport::loadMasked` is intrinsic and
inlined:
@ 45 jdk.internal.vm.vector.VectorSupport::loadMasked (40 bytes)
(intrinsic)
But the second call (for the last loop iteration) fails to inline:
@ 45 jdk.internal.vm.vector.VectorSupport::loadMasked (40 bytes) failed
to inline (intrinsic)
Since i am running on an mac book this looks right and aligns with the
`-XX:+PrintIntrinsics` output:
** Rejected vector op (LoadVectorMasked,int,8) because architecture does not
support it
** Rejected vector op (LoadVectorMasked,int,8) because architecture does not
support it
** not supported: op=loadMasked vlen=8 etype=int using_byte_array=0
?
I have not looked at the code gen nor measured performance comparing the case
when never out of bounds and only out of bounds for the last loop iteration.
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Integrated: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam wrote: > The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never > supported the value `"rw"` since the source code was imported to the openjdk > repo more than 15 years ago. In fact HotSpot throws > `IllegalArgumentException` when such a mode is specified. > > It's unlikely such a mode will be required for future enhancements. Support > for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` > is the only supported mode. > > I also cleaned up related code in the JDK and HotSpot. > > Testing: > - Passed tiers 1 ~ 5 This pull request has now been integrated. Changeset: fcf49f42 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/fcf49f42cef4ac3e50b3b480aecf6fa38cf5be00 Stats: 205 lines in 15 files changed: 3 ins; 153 del; 49 mod 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() Reviewed-by: redestad, alanb - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 20:03:45 GMT, Alan Bateman wrote: >> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never >> supported the value `"rw"` since the source code was imported to the openjdk >> repo more than 15 years ago. In fact HotSpot throws >> `IllegalArgumentException` when such a mode is specified. >> >> It's unlikely such a mode will be required for future enhancements. Support >> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` >> is the only supported mode. >> >> I also cleaned up related code in the JDK and HotSpot. >> >> Testing: >> - Passed tiers 1 ~ 5 > > I skimmed through the changes and I think they look okay. In the distant past > there were tools outside of the JDK that used the jvmstat API directly. It's > possible that VisualVM still does but it would only compile/run if > --add-exports is used to export the sun.jvmstat.* packages. So it might be > that dropping the parameter from a method in RemoteHost is noticed and I > think that is okay because this package is not exported and is not meant to > be used by code outside of the JDK. Thanks to @AlanBateman and @cl4es for the review. - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286594: (zipfs) Improvements after JDK-8251329
On Wed, 11 May 2022 15:32:38 GMT, Christoph Langer wrote: > This augments the ZipException upon rejecting a Zip File containing entry > names with "." or ".." elements. > > It furthermore tries to clean up & compact the logic for detecting "." and > ".." and it adds a method IndexNode::nameAsString to return the node name as > a String. Looks good Christoph. Thank you for separating this out from the other PR - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8655
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v4]
On Wed, 11 May 2022 17:04:41 GMT, Naoto Sato wrote:
>> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support
>> second-level resolution, enabling round trips with `java.time.ZoneOffset`s.
>> Corresponding CSR is also being drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Minor fixup
src/java.base/share/classes/java/util/TimeZone.java line 80:
> 78: * {@code GMT} Sign Hours {@code :} Minutes
> {@code :} Seconds
> 79: * {@code GMT} Sign Hours {@code :} Minutes
> 80: * {@code GMT} Sign Hours Minutes
For hours and minutes, the format can be hh:mm or hhmm. Is it worth it to
support hhmmss as well?
-
PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 15:03:56 GMT, Lance Andersen wrote: >> Jaikiran Pai has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - copyright years >> - disable format-nonliteral warning when building LIBSPLASHSCREEN with >> bundled zlib >> - Magnus' suggestion - make the LIBZ_CFLAGS more readable in the build file >> - Magnus' suggestion - Disable format-nonliteral in build section of zlib >> instead of source code > > make/autoconf/lib-bundled.m4 line 220: > >> 218: if test "x$USE_EXTERNAL_LIBZ" = "xfalse"; then >> 219: LIBZ_CFLAGS="$LIBZ_CFLAGS >> -I$TOPDIR/src/java.base/share/native/libzip/zlib" >> 220: if test "x$OPENJDK_TARGET_OS" = xmacosx; then > > Please add a comment here as to why we are doing this > @LanceAndersen Is that really needed? We normally don't comment on the reason > why certain code needs certain defines. We tried to document some compiler > flags in the beginning, but it turned out that most command lines ended up as > essays, and this were not helpful. I think it's quite obvious what this code > does: libz requires the define HAVE_UNISTD_H on macOS. I'm not even sure how > you'd formulate a "why" -- "otherwise it does not work properly"? The zlib configure script, which needs to be run prior running make to build the upstream repository, will determine if HAVE_UNISTD_H is needed and generate zconf.h replacing the macro with "1".My reason for adding a comment as this is a variant from how zlib is built upstream. Perhaps just updating open/src/java.base/share/native/libzip/zlib/ChangeLog is enough. I was just trying to document why we are doing this. Another question would it make sense to set LIBZ_DISABLED_WARNINGS_CLANG in make/autoconf/lib-bundled.m4 so that the value in the case of zlib is set in one location? In addition, I can log a request to address this in the upstream project so we do not need to worry about this warning going forward. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8282664: Unroll by hand StringUTF16, StringLatin1, and Arrays polynomial hash loops [v12]
On Tue, 10 May 2022 14:46:56 GMT, Ludovic Henry wrote: >> Despite the hash value being cached for Strings, computing the hash still >> represents a significant CPU usage for applications handling lots of text. >> >> Even though it would be generally better to do it through an enhancement to >> the autovectorizer, the complexity of doing it by hand is trivial and the >> gain is sizable (2x speedup) even without the Vector API. The algorithm has >> been proposed by Richard Startin and Paul Sandoz [1]. >> >> Speedup are as follows on a `Intel(R) Xeon(R) E-2276G CPU @ 3.80GHz` >> >> >> Benchmark(size) Mode Cnt Score >>Error Units >> StringHashCode.Algorithm.scalarLatin1 0 avgt 25 2.111 >> ± 0.210 ns/op >> StringHashCode.Algorithm.scalarLatin1 1 avgt 25 3.500 >> ± 0.127 ns/op >> StringHashCode.Algorithm.scalarLatin110 avgt 25 7.001 >> ± 0.099 ns/op >> StringHashCode.Algorithm.scalarLatin1 100 avgt 2561.285 >> ± 0.444 ns/op >> StringHashCode.Algorithm.scalarLatin1 1000 avgt 25 628.995 >> ± 0.846 ns/op >> StringHashCode.Algorithm.scalarLatin1 1 avgt 25 6307.990 >> ± 4.071 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 0 avgt 25 2.358 >> ± 0.092 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3.631 >> ± 0.159 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 10 avgt 25 7.049 >> ± 0.019 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 100 avgt 2533.626 >> ± 1.218 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled161000 avgt 25 317.811 >> ± 1.225 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3212.333 >> ± 14.621 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled80 avgt 25 2.356 >> ± 0.097 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3.630 >> ± 0.158 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 10 avgt 25 8.724 >> ± 0.065 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 100 avgt 2532.402 >> ± 0.019 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 1000 avgt 25 321.949 >> ± 0.251 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3202.083 >> ± 1.667 ns/op >> StringHashCode.Algorithm.scalarUTF16 0 avgt 25 2.135 >> ± 0.191 ns/op >> StringHashCode.Algorithm.scalarUTF16 1 avgt 25 5.202 >> ± 0.362 ns/op >> StringHashCode.Algorithm.scalarUTF16 10 avgt 2511.105 >> ± 0.112 ns/op >> StringHashCode.Algorithm.scalarUTF16100 avgt 2575.974 >> ± 0.702 ns/op >> StringHashCode.Algorithm.scalarUTF16 1000 avgt 25 716.429 >> ± 3.290 ns/op >> StringHashCode.Algorithm.scalarUTF16 1 avgt 25 7095.459 >> ± 43.847 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled160 avgt 25 2.381 >> ± 0.038 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 5.268 >> ± 0.422 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 10 avgt 2511.248 >> ± 0.178 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 100 avgt 2552.966 >> ± 0.089 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 1000 avgt 25 450.912 >> ± 1.834 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 4403.988 >> ± 2.927 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 0 avgt 25 2.401 >> ± 0.032 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 5.091 >> ± 0.396 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled810 avgt 2512.801 >> ± 0.189 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 100 avgt 2552.068 >> ± 0.032 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1000 avgt 25 453.270 >> ± 0.340 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 4433.112 >> ± 2.699 ns/op >> >> >> At Datadog, we handle a great amount of text (through logs management for >> example), and hashing String represents a large part of our CPU usage. It's >> very unlikely that we are the only one as String.hashCode is such a core >> feature of the JVM-based languages with its use in HashMap for example. >> Having even only a 2x speedup would allow us to save thousands of CPU cores >> per month and improve correspondingly the energy/carbon impact. >> >> [1] >> https://static.rainfocus.com/oracle/oow18/sess/1525822677955001tLqU/PF/codeone18-vector-API-DEV5081_1540354883936001Q3Sv.pdf > > Ludovic Henry has updated the pull request with a new target base due to a > merge or a reba
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:35:00 GMT, Kim Barrett wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid pragma error in before GCC 12 > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 462: > >> 460:HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits >> missing-field-initializers strict-aliasing >> 461:HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder >> delete-non-virtual-dtor strict-overflow \ >> 462: maybe-uninitialized class-memaccess unused-result extra >> use-after-free > > Globally disabling use-after-free warnings for this package seems really > questionable. If these are problems in the code, just suppressing the warning > and leaving the problems to bite us seems like a bad idea. And the problems > ought to be reported upstream to the HarfBuzz folks. I don't understand what the actual warning is getting at .. can anyone explain it ? FWIW the code is still the same in upstream harfbuzz https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-font.cc - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Wed, 11 May 2022 16:30:41 GMT, Roger Riggs wrote: >> PR#8599 8244681: proposes to add compiler warnings for possible lossy >> conversions >> From the CSR: >> >> "If the type of the right-hand operand of a compound assignment is not >> assignment compatible with the type of the variable, a cast is implied and >> possible lossy conversion may silently occur. While similar situations are >> resolved as compilation errors for primitive assignments, there are no >> similar rules defined for compound assignments." >> >> This PR anticipates the warnings and adds explicit casts to replace the >> implicit casts. >> In most cases, the cast matches the type of the right-hand side to type of >> the compound assignment. >> Since these casts are truncating the value, there might be a different >> refactoring that avoids the cast >> but a direct replacement was chosen here. >> >> (Advise and suggestions will inform similar updates to other OpenJDK >> modules). > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Updated copyrights > Fixed cast style to add a space after cast, (where consistent with file > style) > Improved code per review comments in PollSelectors. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
On Wed, 11 May 2022 02:43:21 GMT, Ioi Lam wrote: >> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never >> supported the value `"rw"` since the source code was imported to the openjdk >> repo more than 15 years ago. In fact HotSpot throws >> `IllegalArgumentException` when such a mode is specified. >> >> It's unlikely such a mode will be required for future enhancements. Support >> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` >> is the only supported mode. >> >> I also cleaned up related code in the JDK and HotSpot. >> >> Testing: >> - Passed tiers 1 and 2 >> - Tiers 3, 4, 5 are in progress > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > review comments Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:24:38 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which fixes build failures on macos >> when using `--with-zlib=bundled`? >> >> With this change the build now passes (tested both with bundled and system >> zlib variants). >> >> tier1, tier2 and tier3 testing has been done and no related failures have >> been noticed. > > Jaikiran Pai has updated the pull request incrementally with four additional > commits since the last revision: > > - copyright years > - disable format-nonliteral warning when building LIBSPLASHSCREEN with > bundled zlib > - Magnus' suggestion - make the LIBZ_CFLAGS more readable in the build file > - Magnus' suggestion - Disable format-nonliteral in build section of zlib > instead of source code Looks good to me. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 15:03:56 GMT, Lance Andersen wrote: >> Jaikiran Pai has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - copyright years >> - disable format-nonliteral warning when building LIBSPLASHSCREEN with >> bundled zlib >> - Magnus' suggestion - make the LIBZ_CFLAGS more readable in the build file >> - Magnus' suggestion - Disable format-nonliteral in build section of zlib >> instead of source code > > make/autoconf/lib-bundled.m4 line 220: > >> 218: if test "x$USE_EXTERNAL_LIBZ" = "xfalse"; then >> 219: LIBZ_CFLAGS="$LIBZ_CFLAGS >> -I$TOPDIR/src/java.base/share/native/libzip/zlib" >> 220: if test "x$OPENJDK_TARGET_OS" = xmacosx; then > > Please add a comment here as to why we are doing this @LanceAndersen Is that really needed? We normally don't comment on the reason why certain code needs certain defines. We tried to document some compiler flags in the beginning, but it turned out that most command lines ended up as essays, and this were not helpful. I think it's quite obvious what this code does: libz requires the define HAVE_UNISTD_H on macOS. I'm not even sure how you'd formulate a "why" -- "otherwise it does not work properly"? > make/modules/java.base/lib/CoreLibraries.gmk line 139: > >> 137: DISABLED_WARNINGS_gcc := unused-function implicit-fallthrough, \ >> 138: DISABLED_WARNINGS_clang := format-nonliteral, \ >> 139: LDFLAGS := $(LDFLAGS_JDKLIB) \ > > A comment would be good here also as to the reasoning And once again, we never comment on why we disable warnings. The context is clear enough -- there is a compiler warning that is not applicable to this module. Especially for 3rd party code, which is not nearly as stringent as we are about enabling warnings. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v7]
On Mon, 9 May 2022 12:30:19 GMT, Ichiroh Takiguchi wrote: >> On JDK19 with Linux ja_JP.eucjp locale, >> System.getenv() returns unexpected value if environment variable has >> Japanese EUC characters. >> It seems this issue happens because of JEP 400. >> Arguments for ProcessBuilder have same kind of issue. > > Ichiroh Takiguchi has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains seven commits: > > - Merge branch 'master' into 8285517 > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character > - 8285517: System.getenv() returns unexpected value if environment variable > has non ASCII character src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 68: > 66: private static final Map theUnmodifiableEnvironment; > 67: static final int MIN_NAME_LENGTH = 0; > 68: private static final Charset jnuCharset = StaticProperty.jnuCharset(); Since the Charset is cached in StaticProperty, I don't think its worthwhile to make a local copy of the same ref. Just refer to `StaticProperty.jnuCharset()` where it is needed. (Here and in ProcessImpl) - PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Wed, 11 May 2022 16:38:54 GMT, Jorn Vernee wrote: >> If you want to avoid processing asynchronous exceptions during this upcall >> you could block them (check NoAsyncExceptionDeliveryMark in >> JavaThread::exit()). Seems you could set the flag in >> ProgrammableUpcallhandler::on_entry() and unset it back on >> ProgrammableUpcallhandler::on_exit(). While that flag is set any >> asynchronous exception in the handshake queue of this thread will be skipped >> from processing. Maybe we should add a public method in the JavaThread >> class, block_async_exceptions()/unblock_async_exceptions() so we hide the >> handshake implementation. > > Oh nice! I was just thinking that the only possible way out of this conundrum > would be to somehow block the delivery of async exceptions (at least outside > of the user's exception handler). So, that seems to be exactly what we need :) I went ahead and implemented this suggestion. Now we block async exceptions in on_entry, and unblock in on_exit. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v10]
> Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20, but it > would be nice if we could get it into 19. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 26 commits: - Block async exceptions during upcalls - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 - Fix use of rt_call - Migrate to GrowableArray - Address some review comments - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 - Remove unneeded ComputeMoveOrder - Remove comment about native calls in lcm.cpp - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 Reviewed-by: jvernee, mcimadamore - Update riscv and arm stubs - ... and 16 more: https://git.openjdk.java.net/jdk/compare/cdd006e7...b29ad8f4 - Changes: https://git.openjdk.java.net/jdk/pull/7959/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7959&range=09 Stats: 6870 lines in 157 files changed: 2596 ins; 3218 del; 1056 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Wed, 11 May 2022 16:30:41 GMT, Roger Riggs wrote: >> PR#8599 8244681: proposes to add compiler warnings for possible lossy >> conversions >> From the CSR: >> >> "If the type of the right-hand operand of a compound assignment is not >> assignment compatible with the type of the variable, a cast is implied and >> possible lossy conversion may silently occur. While similar situations are >> resolved as compilation errors for primitive assignments, there are no >> similar rules defined for compound assignments." >> >> This PR anticipates the warnings and adds explicit casts to replace the >> implicit casts. >> In most cases, the cast matches the type of the right-hand side to type of >> the compound assignment. >> Since these casts are truncating the value, there might be a different >> refactoring that avoids the cast >> but a direct replacement was chosen here. >> >> (Advise and suggestions will inform similar updates to other OpenJDK >> modules). > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Updated copyrights > Fixed cast style to add a space after cast, (where consistent with file > style) > Improved code per review comments in PollSelectors. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v4]
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support > second-level resolution, enabling round trips with `java.time.ZoneOffset`s. > Corresponding CSR is also being drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Minor fixup - Changes: - all: https://git.openjdk.java.net/jdk/pull/8606/files - new: https://git.openjdk.java.net/jdk/pull/8606/files/fcdaf512..9722decd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8606&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8606&range=02-03 Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v3]
I checked you pr look good to me @Roger Le mer. 11 mai 2022 à 15:35, Roger Riggs a écrit : > On Wed, 11 May 2022 13:27:38 GMT, Adam Sotona wrote: > > >> That's good to know. I think the tricky part is mostly about keeping > track of all these disabled warnings, so they are not kept around longer > than necessary. And that needs coordination with all the subtasks of the > umbrella issue. > > > > Thanks for quick reaction. > > I'll keep my eyes on this race of patches and update this pull request > accordingly or create a new PR. > > I put out a PR for java.base, but thought I'd wait until the javac fixe > were pushed before integrating and would re-enable the warnings at the same > time. > > - > > PR: https://git.openjdk.java.net/jdk/pull/8599 >
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Wed, 11 May 2022 16:20:32 GMT, Patricio Chilano Mateo wrote: >> Discussed this with Maurizio as well. >> >> My understanding is as follows: >> 1. Async exceptions are installed into a thread's `pending_exception` field >> by handshake at a safepoint >> 2. From there they are "thrown" (I guess during the same safepoint?), in >> which case we either end up in a user-defined exception handler (higher up >> the stack), in our Java fallback exception handler (print stack trace and >> terminate), or in this fallback exception handler (print and terminate). >> 3. If we end up in this exception handler it means the async exception was >> installed somewhere outside of our Java exception handler and "thrown" from >> there. However, it also means that the Java code we were calling into >> completed abruptly. >> 4. We've previously established that we have no way of signalling to the >> native code that is calling us that something went wrong, and so the only >> safe option is to terminate, as to not leave the application in an >> inconsistent state. >> >> As a consequence, I don't think we have much choice in the case of async >> exceptions if we get here. Silently clearing them seems like it will leave >> the program in an inconsistent state (since we unwound some frames), so we >> have to terminate I think. >> >> (@dholmes-ora is my understanding of async exceptions in point 1. and 2. >> correct here?) > > If you want to avoid processing asynchronous exceptions during this upcall > you could block them (check NoAsyncExceptionDeliveryMark in > JavaThread::exit()). Seems you could set the flag in > ProgrammableUpcallhandler::on_entry() and unset it back on > ProgrammableUpcallhandler::on_exit(). While that flag is set any asynchronous > exception in the handshake queue of this thread will be skipped from > processing. Maybe we should add a public method in the JavaThread class, > block_async_exceptions()/unblock_async_exceptions() so we hide the handshake > implementation. Oh nice! I was just thinking that the only possible way out of this conundrum would be to somehow block the delivery of async exceptions (at least outside of the user's exception handler). So, that seems to be exactly what we need :) - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v3]
On Wed, 11 May 2022 09:00:45 GMT, Uwe Schindler wrote: >> I tried it out: If you create a ZoneId with prefixes "UT" and "UTC", they >> fail to convert to TimeZone. Same happens if you use this as String name in >> `TimeZone#getTimeZone(String)`. This is another bug / missing feature! It >> does not work with or without this PR. > > In short, we can only test what works. The test was mainly added to allow > roundtrips of `ZoneOffset` instances. The code intended to allow only offset-style ids that start with `GMT` (which is compatible with TimeZone's CustomID). Now I made changes to allow all offset-style zone ids. - PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v3]
> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support > second-level resolution, enabling round trips with `java.time.ZoneOffset`s. > Corresponding CSR is also being drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Allowed round-trips for offset-style ZoneIds. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8606/files - new: https://git.openjdk.java.net/jdk/pull/8606/files/81a806e5..fcdaf512 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8606&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8606&range=01-02 Stats: 133 lines in 3 files changed: 70 ins; 61 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8606.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8606/head:pull/8606 PR: https://git.openjdk.java.net/jdk/pull/8606
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
> PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Updated copyrights Fixed cast style to add a space after cast, (where consistent with file style) Improved code per review comments in PollSelectors. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8642/files - new: https://git.openjdk.java.net/jdk/pull/8642/files/7ff806a5..a6679ce7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8642&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8642&range=01-02 Stats: 41 lines in 24 files changed: 0 ins; 0 del; 41 mod Patch: https://git.openjdk.java.net/jdk/pull/8642.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642 PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286287: Reading file as UTF-16 causes Error which "shouldn't happen" [v2]
On Wed, 11 May 2022 15:52:59 GMT, Naoto Sato wrote: >> `String.decodeWithDecoder()` method requires the `CharsetDecoder` parameter >> replaces on malformed/unmappable characters with replacements. However, >> there was a code path that lacked to set the `CodingErrorAction.REPLACE` on >> the decoder. Unrelated, one typo in a test was also fixed. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reverted the typo fix Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8640
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Wed, 11 May 2022 15:44:19 GMT, Jorn Vernee wrote: >> We have an exception handler in Java as well, so this code is only a fail >> safe. But, I think in the case of asynchronous exceptions this might be >> problematic if the exception is discovered by the current thread outside of >> the Java exception handler, turned into a synchronous exception and then we >> get here and call `ProgrammableUpcallhandler::handle_uncaught_exception` and >> then crash. Or if the asynchronous exception is discovered in >> `ProgrammableUpcallHandler::on_exit` (where there is currently an assert for >> no exceptions). >> >> I think you're right that, in both of those cases, if the exception is >> asynchronous, we should just ignore it. > > Discussed this with Maurizio as well. > > My understanding is as follows: > 1. Async exceptions are installed into a thread's `pending_exception` field > by handshake at a safepoint > 2. From there they are "thrown" (I guess during the same safepoint?), in > which case we either end up in a user-defined exception handler (higher up > the stack), in our Java fallback exception handler (print stack trace and > terminate), or in this fallback exception handler (print and terminate). > 3. If we end up in this exception handler it means the async exception was > installed somewhere outside of our Java exception handler and "thrown" from > there. However, it also means that the Java code we were calling into > completed abruptly. > 4. We've previously established that we have no way of signalling to the > native code that is calling us that something went wrong, and so the only > safe option is to terminate, as to not leave the application in an > inconsistent state. > > As a consequence, I don't think we have much choice in the case of async > exceptions if we get here. Silently clearing them seems like it will leave > the program in an inconsistent state (since we unwound some frames), so we > have to terminate I think. > > (@dholmes-ora is my understanding of async exceptions in point 1. and 2. > correct here?) If you want to avoid processing asynchronous exceptions during this upcall you could block them (check NoAsyncExceptionDeliveryMark in JavaThread::exit()). Seems you could set the flag in ProgrammableUpcallhandler::on_entry() and unset it back on ProgrammableUpcallhandler::on_exit(). While that flag is set any asynchronous exception in the handshake queue of this thread will be skipped from processing. Maybe we should add a public method in the JavaThread class, block_async_exceptions()/unblock_async_exceptions() so we hide the handshake implementation. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]
On Wed, 4 May 2022 08:00:08 GMT, Matthias Baesken wrote: >> Currently we set _WIN32_WINNT at various places in the codebase; this is >> used to target a minimum Windows version we want to support. See also for >> more detailled information : >> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt >> Macros for Conditional Declarations >> "Certain functions that depend on a particular version of Windows are >> declared using conditional code. This enables you to use the compiler to >> detect whether your application uses functions that are not supported on its >> target version(s) of Windows." >> >> However currently we have quite a lot of differing settings of _WIN32_WINNT >> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible >> would make sense because we have this setting already at java_props_md.c >> (so targeting older Windows versions at other places is most likely not >> useful). > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust API level to Windows 8 for security.cpp and do some cleanup This change seem to have made this group of declarations obsolete: `src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h:157` (follow the [link]( https://github.com/openjdk/jdk/blob/89de756ffbefac452c7df559e2a4eb50bf71368b/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L157)). Does anyone have plans to drop those? Have any bugs been filed for that? If not, I'll attend to that myself. - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: 8286287: Reading file as UTF-16 causes Error which "shouldn't happen" [v2]
> `String.decodeWithDecoder()` method requires the `CharsetDecoder` parameter > replaces on malformed/unmappable characters with replacements. However, there > was a code path that lacked to set the `CodingErrorAction.REPLACE` on the > decoder. Unrelated, one typo in a test was also fixed. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reverted the typo fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/8640/files - new: https://git.openjdk.java.net/jdk/pull/8640/files/0fa7cd38..d5a29e2b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8640&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8640&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8640.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8640/head:pull/8640 PR: https://git.openjdk.java.net/jdk/pull/8640
Re: RFR: 8283689: Update the foreign linker VM implementation [v9]
On Wed, 11 May 2022 12:20:43 GMT, Jorn Vernee wrote: >> Any problems with migrating `CallConv` and `RegSpiller`away from ` VMReg* + >> int` to `GrowableArray`? > > I'll try migrating to `GrowableArray` Done. I've removed these accessors as well. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Wed, 11 May 2022 12:05:29 GMT, Jorn Vernee wrote: >> src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 306: >> >>> 304: intptr_t exception_handler_offset = __ pc() - start; >>> 305: >>> 306: // Native caller has no idea how to handle exceptions, >> >> Can you elaborate, please, how it is expected to work in presence of >> asynchronous exceptions? I'd expect to see a code which unconditionally >> clears pending exception with an assertion that verifies that the exception >> is of expected type. > > We have an exception handler in Java as well, so this code is only a fail > safe. But, I think in the case of asynchronous exceptions this might be > problematic if the exception is discovered by the current thread outside of > the Java exception handler, turned into a synchronous exception and then we > get here and call `ProgrammableUpcallhandler::handle_uncaught_exception` and > then crash. Or if the asynchronous exception is discovered in > `ProgrammableUpcallHandler::on_exit` (where there is currently an assert for > no exceptions). > > I think you're right that, in both of those cases, if the exception is > asynchronous, we should just ignore it. Discussed this with Maurizio as well. My understanding is as follows: 1. Async exceptions are installed into a thread's `pending_exception` field by handshake at a safepoint 2. From there they are "thrown" (I guess during the same safepoint?), in which case we either end up in a user-defined exception handler (higher up the stack), in our Java fallback exception handler (print stack trace and terminate), or in this fallback exception handler (print and terminate). 3. If we end up in this exception handler it means the async exception was installed somewhere outside of our Java exception handler and "thrown" from there. However, it also means that the Java code we were calling into completed abruptly. 4. We've previously established that we have no way of signalling to the native code that is calling us that something went wrong, and so the only safe option is to terminate, as to not leave the application in an inconsistent state. As a consequence, I don't think we have much choice in the case of async exceptions if we get here. Silently clearing them seems like it will leave the program in an inconsistent state (since we unwound some frames), so we have to terminate I think. (@dholmes-ora is my understanding of async exceptions in point 1. and 2. correct here?) - PR: https://git.openjdk.java.net/jdk/pull/7959
RFR: 8286594: (zipfs) Improvements after JDK-8251329
This augments the ZipException upon rejecting a Zip File containing entry names with "." or ".." elements. It furthermore tries to clean up & compact the logic for detecting "." and ".." and it adds a method IndexNode:nameAsString() to return the node name as a String. - Commit messages: - JDK-8286594 Changes: https://git.openjdk.java.net/jdk/pull/8655/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8655&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286594 Stats: 73 lines in 1 file changed: 31 ins; 38 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8655.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8655/head:pull/8655 PR: https://git.openjdk.java.net/jdk/pull/8655
Re: Can JDK-8190546 be re-opened or "how do I delete a file ending with a space on Windows"
Redirect discussion to nio-dev. Brian > On May 11, 2022, at 7:29 AM, Maxim Kartashev > wrote: > > Win32 documentation [1] kind of discourages the use of space at the very > end of a file name. Based on that, JDK-8190546 (File.toPath() reject > directory names with trailing space) had been closed a long time ago. I > would like to poll the public on the matter of re-opening the bug. > > There isn't anything new that I can throw in support of allowing JDK to > work with such file names. But the old points can perhaps be re-visited. > AFAIU, the only reason to explicitly forbid that (see > WindowsPathParser.normalize()) was that the parsing is based on Windows > documentation like [1]. That documentation says the following: > "Do not end a file or directory name with a space or a period. Although the > underlying file system may support such names, the Windows shell and user > interface does not" > Indeed, it's hard or even impossible to create such a file using "normal" > windows GUI, but you can use that GUI to delete such a file. In Java, you > can't do either. Working in a cygwin terminal, you can fairly easily create > a file name ending with a space. And when you turn to your Java-based IDE > to delete it, you get an error from the very basic level of NIO that you > can't overrule, which seems to be quite unfortunate. > > If there's any interest in resolving this - or at least not enough > opposition to let it be resolved - I am willing to make the necessary > changes and open a PR. > > References > [1] > https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions > [2] https://bugs.openjdk.java.net/browse/JDK-8190546
Re: RFR: 8286287: Reading file as UTF-16 causes Error which "shouldn't happen"
On Tue, 10 May 2022 20:22:39 GMT, Naoto Sato wrote: > `String.decodeWithDecoder()` method requires the `CharsetDecoder` parameter > replaces on malformed/unmappable characters with replacements. However, there > was a code path that lacked to set the `CodingErrorAction.REPLACE` on the > decoder. Unrelated, one typo in a test was also fixed. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8640
Re: RFR: 8283689: Update the foreign linker VM implementation [v9]
> Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Fix use of rt_call - Changes: - all: https://git.openjdk.java.net/jdk/pull/7959/files - new: https://git.openjdk.java.net/jdk/pull/7959/files/abd2b6ca..c6754a1c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7959&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7959&range=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8286279: [vectorapi] Only check index of masked lanes if offset is out of array boundary for masked store
On Tue, 10 May 2022 01:23:55 GMT, Xiaohong Gong wrote:
> Checking whether the indexes of masked lanes are inside of the valid memory
> boundary is necessary for masked vector memory access. However, this could be
> saved if the given offset is inside of the vector range that could make sure
> no IOOBE (IndexOutOfBoundaryException) happens. The masked load APIs have
> saved this kind of check for common cases. And this patch did the similar
> optimization for the masked vector store.
>
> The performance for the new added store masked benchmarks improves about
> `1.83x ~ 2.62x` on a x86 system:
>
> Benchmark BeforeAfter Gain Units
> StoreMaskedBenchmark.byteStoreArrayMask 12757.936 23291.118 1.826 ops/ms
> StoreMaskedBenchmark.doubleStoreArrayMask 1520.932 3921.616 2.578 ops/ms
> StoreMaskedBenchmark.floatStoreArrayMask 2713.031 7122.535 2.625 ops/ms
> StoreMaskedBenchmark.intStoreArrayMask 4113.772 8220.206 1.998 ops/ms
> StoreMaskedBenchmark.longStoreArrayMask1993.986 4874.148 2.444 ops/ms
> StoreMaskedBenchmark.shortStoreArrayMask 8543.593 17821.086 2.086 ops/ms
>
> Similar performane gain can also be observed on ARM hardware.
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-Vector.java.template
line 4086:
> 4084: } else {
> 4085: $Type$Species vsp = vspecies();
> 4086: if (offset < 0 || offset > (a.length - vsp.length())) {
Can we use `VectorIntrinsics.checkFromIndexSize`? e.g.
if (!VectorIntrinsics.checkFromIndexSize(offset, vsp.length(), a.length)) { ...
-
PR: https://git.openjdk.java.net/jdk/pull/8620
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:24:38 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which fixes build failures on macos >> when using `--with-zlib=bundled`? >> >> With this change the build now passes (tested both with bundled and system >> zlib variants). >> >> tier1, tier2 and tier3 testing has been done and no related failures have >> been noticed. > > Jaikiran Pai has updated the pull request incrementally with four additional > commits since the last revision: > > - copyright years > - disable format-nonliteral warning when building LIBSPLASHSCREEN with > bundled zlib > - Magnus' suggestion - make the LIBZ_CFLAGS more readable in the build file > - Magnus' suggestion - Disable format-nonliteral in build section of zlib > instead of source code Hi Jai, thank you for continuing the work to allow us to build/use the bundled zlib on macOS we should also update: open/src/java.base/share/native/libzip/zlib/ChangeLog to add a comment regarding why the build changes were required make/autoconf/lib-bundled.m4 line 220: > 218: if test "x$USE_EXTERNAL_LIBZ" = "xfalse"; then > 219: LIBZ_CFLAGS="$LIBZ_CFLAGS > -I$TOPDIR/src/java.base/share/native/libzip/zlib" > 220: if test "x$OPENJDK_TARGET_OS" = xmacosx; then Please add a comment here as to why we are doing this make/modules/java.base/lib/CoreLibraries.gmk line 139: > 137: DISABLED_WARNINGS_gcc := unused-function implicit-fallthrough, \ > 138: DISABLED_WARNINGS_clang := format-nonliteral, \ > 139: LDFLAGS := $(LDFLAGS_JDKLIB) \ A comment would be good here also as to the reasoning make/modules/java.desktop/lib/Awt2dLibraries.gmk line 683: > 681: ifeq ($(USE_EXTERNAL_LIBZ), false) > 682: LIBSPLASHSCREEN_EXTRA_SRC += java.base:libzip/zlib > 683: LIBZ_DISABLED_WARNINGS_CLANG := format-nonliteral Same here for a comment - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v3]
On Wed, 11 May 2022 03:23:13 GMT, Xiaohong Gong wrote:
>> I modified the code of this PR to avoid the conversion of `boolean` to
>> `int`, so a constant integer value is passed all the way through, and the
>> masked load is made intrinsic from the method at which the constants are
>> passed as arguments i.e. the public `fromArray` mask accepting method.
>
> Hi @PaulSandoz , thanks for the patch for the constant int parameter. I think
> the main change is:
>
> -ByteVector fromArray0Template(Class maskClass, C base, long offset,
> int index, M m, boolean offsetInRange,
> +ByteVector fromArray0Template(Class maskClass, C base, long offset,
> int index, M m, int offsetInRange,
> VectorSupport.LoadVectorMaskedOperation ByteVector, ByteSpecies, M> defaultImpl) {
> m.check(species());
> ByteSpecies vsp = vspecies();
> -if (offsetInRange) {
> -return VectorSupport.loadMasked(
> -vsp.vectorType(), maskClass, vsp.elementType(),
> vsp.laneCount(),
> -base, offset, m, /* offsetInRange */ 1,
> -base, index, vsp, defaultImpl);
> -} else {
> -return VectorSupport.loadMasked(
> -vsp.vectorType(), maskClass, vsp.elementType(),
> vsp.laneCount(),
> -base, offset, m, /* offsetInRange */ 0,
> -base, index, vsp, defaultImpl);
> -}
> +return VectorSupport.loadMasked(
> +vsp.vectorType(), maskClass, vsp.elementType(), vsp.laneCount(),
> +base, offset, m, offsetInRange == 1 ? 1 : 0,
> +base, index, vsp, defaultImpl);
> }
>
> which uses `offsetInRange == 1 ? 1 : 0`. Unfortunately this could not always
> make sure the `offsetInRange` a constant a the compiler time. Again, this
> change could also make the assertion fail randomly:
>
> --- a/src/hotspot/share/opto/vectorIntrinsics.cpp
> +++ b/src/hotspot/share/opto/vectorIntrinsics.cpp
> @@ -1236,6 +1236,7 @@ bool
> LibraryCallKit::inline_vector_mem_masked_operation(bool is_store) {
> } else {
>// Masked vector load with IOOBE always uses the predicated load.
>const TypeInt* offset_in_range = gvn().type(argument(8))->isa_int();
> + assert(offset_in_range->is_con(), "must be a constant");
>if (!offset_in_range->is_con()) {
> if (C->print_intrinsics()) {
>tty->print_cr(" ** missing constant: offsetInRange=%s",
>
> Sometimes, the compiler can parse it a constant. I think this depends on the
> compiler OSR and speculative optimization. Did you try an example with IOOBE
> on a non predicated hardware?
>
> Here is the main code of my unittest to reproduce the issue:
>
> static final VectorSpecies I_SPECIES = IntVector.SPECIES_128;
> static final int LENGTH = 1026;
> public static int[] ia;
> public static int[] ib;
>
> private static void init() {
> for (int i = 0; i < LENGTH; i++) {
> ia[i] = i;
> ib[i] = 0;
> }
>
> for (int i = 0; i < 2; i++) {
> m[i] = i % 2 == 0;
> }
> }
>
> private static void func() {
> VectorMask mask = VectorMask.fromArray(I_SPECIES, m, 0);
> for (int i = 0; i < LENGTH; i += vl) {
> IntVector av = IntVector.fromArray(I_SPECIES, ia, i, mask);
> av.lanewise(VectorOperators.ABS).intoArray(ic, i, mask);
> }
> }
>
> public static void main(String[] args) {
> init();
> for (int i = 0; i < 1; i++) {
> func();
> }
> }
@XiaohongGong Doh! The ternary was an experiment, and I forgot to re-run the
code gen script before sending your the patch. See `IntVector`, which does not
have that. I presume when the offset is not in range and the other code path is
taken then it might be problematic unless all code paths are inlined. I will
experiment further with tests.
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 14:25:39 GMT, Jaikiran Pai wrote: >> I agree with Magnus and try to avoid changing the imported zlib code. > >> I did these changes locally but for some reason this format-nonliteral is >> not getting picked up while building that library. > > Turns out that was slightly inaccurate. What was actually happening was that, > that setting you suggested in that build file did indeed work and got picked > up. But it ran into an error which looked like: > > > src/java.base/share/native/libzip/zlib/gzwrite.c:452:40: error: format string > is not a string literal [-Werror,-Wformat-nonliteral] > len = vsnprintf(next, state->size, format, va); >^~ > I agree with Magnus and try to avoid changing the imported zlib code. We already had modified gzwrite.c in our port so I thought keeping the changes narrowed to apple made sense here. - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v5]
> Please review this patch adding new lint option, **lossy-conversions**, to > javac to warn about type casts in compound assignments with possible lossy > conversions. > > The new lint warning is shown if the type of the right-hand operand of a > compound assignment is not assignment compatible with the type of the > variable. > > The implementation of the warning is based on similar check performed to emit > "possible lossy conversion" compilation error for simple assignments. > > Proposed patch also include complex matrix-style test with positive and > negative test cases of lossy conversions in compound assignments. > > Proposed patch also disables this new lint option in all affected JDK modules > and libraries to allow smooth JDK build. Individual cases to address possibly > lossy conversions warnings in JDK are already addressed in a separate > umbrella issue and its sub-tasks. > > Thanks for your review, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: enabled lossy-conversions warnings for jdk.jfr and jdk.management.jfr - Changes: - all: https://git.openjdk.java.net/jdk/pull/8599/files - new: https://git.openjdk.java.net/jdk/pull/8599/files/a59dfa4f..32282515 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8599&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8599&range=03-04 Stats: 2 lines in 2 files changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8599.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8599/head:pull/8599 PR: https://git.openjdk.java.net/jdk/pull/8599
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v4]
> Please review this patch adding new lint option, **lossy-conversions**, to > javac to warn about type casts in compound assignments with possible lossy > conversions. > > The new lint warning is shown if the type of the right-hand operand of a > compound assignment is not assignment compatible with the type of the > variable. > > The implementation of the warning is based on similar check performed to emit > "possible lossy conversion" compilation error for simple assignments. > > Proposed patch also include complex matrix-style test with positive and > negative test cases of lossy conversions in compound assignments. > > Proposed patch also disables this new lint option in all affected JDK modules > and libraries to allow smooth JDK build. Individual cases to address possibly > lossy conversions warnings in JDK are already addressed in a separate > umbrella issue and its sub-tasks. > > Thanks for your review, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8244681 - 8244681: Add a warning for possibly lossy conversion in compound assignments recommended correction of the warning description - 8244681: Add a warning for possibly lossy conversion in compound assignments recommended correction of the warning wording fixed typo in test method name - Merge branch 'openjdk:master' into JDK-8244681 - 8244681: Add a warning for possibly lossy conversion in compound assignments jdk.internal.le make patch to disable warnings - 8244681: Add a warning for possibly lossy conversion in compound assignments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8599/files - new: https://git.openjdk.java.net/jdk/pull/8599/files/f0729396..a59dfa4f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8599&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8599&range=02-03 Stats: 9179 lines in 255 files changed: 5253 ins; 2422 del; 1504 mod Patch: https://git.openjdk.java.net/jdk/pull/8599.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8599/head:pull/8599 PR: https://git.openjdk.java.net/jdk/pull/8599
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v3]
On Wed, 11 May 2022 13:31:16 GMT, Roger Riggs wrote: >> Thanks for quick reaction. >> I'll keep my eyes on this race of patches and update this pull request >> accordingly or create a new PR. > > I put out a PR for java.base, but thought I'd wait until the javac fixe were > pushed before integrating and would re-enable the warnings at the same time. Feel free to go ahead with the java.base PR as this one still needs CSR. - PR: https://git.openjdk.java.net/jdk/pull/8599
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Tue, 10 May 2022 20:38:05 GMT, Vladimir Ivanov wrote: >> Jorn Vernee has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 21 commits: >> >> - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 >> - Remove unneeded ComputeMoveOrder >> - Remove comment about native calls in lcm.cpp >> - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 >> >>Reviewed-by: jvernee, mcimadamore >> - Update riscv and arm stubs >> - Remove spurious ProblemList change >> - Pass pointer to LogStream >> - Polish >> - Replace TraceNativeInvokers flag with unified logging >> - Fix other platforms, take 2 >> - ... and 11 more: >> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91 > > src/hotspot/share/prims/foreign_globals.cpp line 217: > >> 215: >> 216: public: >> 217: ForeignCMO(int total_in_args, const VMRegPair* in_regs, int >> total_out_args, VMRegPair* out_regs, > > I propose to turn it into a trivial ctor and move all the logic into a helper > static function which returns the computed moves. Done. Moved constructor logic into a `compute` method, and added a static helper that constructs a ComputeMoveOrder, calls `compute`, and then returns the moves. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Wed, 11 May 2022 11:06:51 GMT, Jorn Vernee wrote: >> src/hotspot/share/opto/callGenerator.cpp line 1131: >> >>> 1129: >>> 1130: case vmIntrinsics::_linkToNative: >>> 1131: print_inlining_failure(C, callee, jvms->depth() - 1, jvms->bci(), >> >> Why is it unconditionally reported as inlining failure? > > The call that is being processed here is `linkToNative`, and that call is not > inlined, so reporting an inlining failure seems correct. We still go through > the method handle trampoline stub which loads the actual target from the > NativeEntryPoint appendix argument (`jump_to_native_invoker` in > methodHandles_x86.cpp). > > It's potentially faster here to generate a runtime call to the underlying > invoker/downcall stub if the NativeEntryPoint is constant (i.e. avoid the > lookup through NEP in the MH trampoline), but I hadn't gotten to > investigating that yet. > > From comparing the benchmark times between this and the old implementation > (which generated an inline call), they are not all that different. So it > seemed that doing something special here would not save that much time any > ways. (but, still would be good to investigate at some point) I've filed: https://bugs.openjdk.java.net/browse/JDK-8286588 - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 11:52:55 GMT, Magnus Ihse Bursie wrote: >> Jaikiran Pai has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - copyright years >> - disable format-nonliteral warning when building LIBSPLASHSCREEN with >> bundled zlib >> - Magnus' suggestion - make the LIBZ_CFLAGS more readable in the build file >> - Magnus' suggestion - Disable format-nonliteral in build section of zlib >> instead of source code > > make/autoconf/lib-bundled.m4 line 224: > >> 222: LIBZ_LIBS="" >> 223: if test "x$USE_EXTERNAL_LIBZ" = "xfalse"; then >> 224: LIBZ_CFLAGS="$LIBZ_CFLAGS $APPLE_LIBZ_CFLAGS >> -I$TOPDIR/src/java.base/share/native/libzip/zlib" > > Declaring APPLE_LIBZ_CFLAGS far away (and only conditionally) and then using > it once here just makes for hard-to-read code. > > Suggestion: > > LIBZ_CFLAGS="$LIBZ_CFLAGS > -I$TOPDIR/src/java.base/share/native/libzip/zlib" > if test "x$OPENJDK_TARGET_OS" = xmacosx; then > LIBZ_CFLAGS="$LIBZ_CFLAGS -DHAVE_UNISTD_H" > fi > > ... and remove the assignment above. Updated the PR to implement this suggestion - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
On Wed, 11 May 2022 12:50:39 GMT, Alan Bateman wrote: >> Thank you for these useful inputs Magnus. I did these changes locally but >> for some reason this format-nonliteral is not getting picked up while >> building that library. I will investigate and see what's going on. Will >> update the PR once I figure it out. > > I agree with Magnus and try to avoid changing the imported zlib code. > I did these changes locally but for some reason this format-nonliteral is > not getting picked up while building that library. Turns out that was slightly inaccurate. What was actually happening was that, that setting you suggested in that build file did indeed work and got picked up. But it ran into an error which looked like: src/java.base/share/native/libzip/zlib/gzwrite.c:452:40: error: format string is not a string literal [-Werror,-Wformat-nonliteral] len = vsnprintf(next, state->size, format, va); ^~ - PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:56:44 GMT, Kim Barrett wrote:
>> Yasumasa Suenaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid pragma error in before GCC 12
>
> src/java.base/share/native/libjli/java.c line 1629:
>
>> 1627: const char *arg = jargv[i];
>> 1628: if (arg[0] == '-' && arg[1] == 'J') {
>> 1629: *nargv++ = (arg[2] == '\0') ? NULL : JLI_StringDup(arg +
>> 2);
>
> Wow!
I wonder if the client expects NULL strings in the result, or if the NULL value
should be an empty string? If empty strings are okay, this would be simpler
without the conditional, just dup from arg + 2 to the terminating byte (which
might be immediate).
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Tue, 10 May 2022 21:02:39 GMT, Vladimir Ivanov wrote: >> Jorn Vernee has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 21 commits: >> >> - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 >> - Remove unneeded ComputeMoveOrder >> - Remove comment about native calls in lcm.cpp >> - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 >> >>Reviewed-by: jvernee, mcimadamore >> - Update riscv and arm stubs >> - Remove spurious ProblemList change >> - Pass pointer to LogStream >> - Polish >> - Replace TraceNativeInvokers flag with unified logging >> - Fix other platforms, take 2 >> - ... and 11 more: >> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91 > > Nice work! Looks good. Some minor comments/questions follow. @iwanowww Thanks for the review! I've addressed most comments already, but will have to think a bit on how to handle the asynchronous exceptions issue. > src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5531: > >> 5529: } >> 5530: >> 5531: // On 64 bit we will store integer like items to the stack as > > Time for a cleanup? `64 bit` vs `64bit`, `abi`, `Aarch64`. I've cleaned up the spaces and capitalization here a bit. > src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 36: > >> 34: #include "code/nativeInst.hpp" >> 35: #include "code/vtableStubs.hpp" >> 36: #include "compiler/disassembler.hpp" > > Redundant includes? No new code added in the file. Good catch. Seems like a merge artifact maybe. Removing them seems to be fine. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v8]
> Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision: - Migrate to GrowableArray - Address some review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7959/files - new: https://git.openjdk.java.net/jdk/pull/7959/files/43fd1b91..abd2b6ca Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7959&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7959&range=06-07 Stats: 254 lines in 23 files changed: 22 ins; 125 del; 107 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled [v2]
> Can I please get a review of this change which fixes build failures on macos > when using `--with-zlib=bundled`? > > With this change the build now passes (tested both with bundled and system > zlib variants). > > tier1, tier2 and tier3 testing has been done and no related failures have > been noticed. Jaikiran Pai has updated the pull request incrementally with four additional commits since the last revision: - copyright years - disable format-nonliteral warning when building LIBSPLASHSCREEN with bundled zlib - Magnus' suggestion - make the LIBZ_CFLAGS more readable in the build file - Magnus' suggestion - Disable format-nonliteral in build section of zlib instead of source code - Changes: - all: https://git.openjdk.java.net/jdk/pull/8651/files - new: https://git.openjdk.java.net/jdk/pull/8651/files/eee12c25..081a7d34 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8651&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8651&range=00-01 Stats: 41 lines in 5 files changed: 5 ins; 30 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8651.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8651/head:pull/8651 PR: https://git.openjdk.java.net/jdk/pull/8651
Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
On Wed, 11 May 2022 05:39:42 GMT, Alan Bateman wrote: > > > It's probably ok, but the bug report is either incomplete or I am missing > > > something. It says "This can be improved to something like: ..." but the > > > same text as is emitted now is used. Can you fix this so I have a better > > > example of what will be included in the message? > > > > > > The bug report also says "The error message does not give a clue which jar > > file is causing the problem" but the error message includes the name > > "invalid.jar" so I am also confused about that. > > There are two parts to it. In the case of initCEN method, the proposed change > is to include the name of the rejected entry in the exception message. This > is not the same thing as leaking a file path in the exception message so I > don't think we have a concern here. Ok, but @RealCLanger can you address the prior comments I had on the bug report? The error messages (before and after the fix) are the same. - PR: https://git.openjdk.java.net/jdk/pull/8616
