Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v2]
On Thu, 21 Mar 2024 14:40:37 GMT, Adam Sotona wrote: >> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only >> bytecode-level class verification. >> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with >> additional class checks inspired by >> `hotspot/share/classfile/classFileParser.cpp`. >> >> Also new `VerifierSelfTest::testParserVerifier` has been added. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 26 commits: > > - Merge remote-tracking branch 'openjdk/master' into > JDK-8320396-verifier-extension > - work in progress > - work in progress > - work in progress > - work in progress > - work in progress > - removed string templates from test > - work in progress > - work in progress > - work in progress > - ... and 16 more: https://git.openjdk.org/jdk/compare/e41bc42d...54c4e9b9 Verification is an optional feature of Class-File API, however it is critical for development and testing of the Class-File API itself and all frameworks and libraries built on top of it. `ClassFile::verify` is now focused on JVMS chapter 6 only, however the goal is to give user a bit more confidence that any class file passing the `ClassFile::verify` is correct. Loading of the generated or transformed classes to verify them is not feasible in many situations and `ClassFile::verify` tries to give user at least some information, similar to ASM `CheckClassAdapter`or BCEL `Verifier`. The work is also related to [JDK-8182774 Verify code in javap](https://bugs.openjdk.org/browse/JDK-8182774). I'll add references to the relevant JVMS chapters. Thank you for the review. - PR Comment: https://git.openjdk.org/jdk/pull/16809#issuecomment-2014600028
Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v3]
> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only > bytecode-level class verification. > This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with > additional class checks inspired by > `hotspot/share/classfile/classFileParser.cpp`. > > Also new `VerifierSelfTest::testParserVerifier` has been added. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: added references to jvms - Changes: - all: https://git.openjdk.org/jdk/pull/16809/files - new: https://git.openjdk.org/jdk/pull/16809/files/54c4e9b9..61a9d748 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16809&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16809&range=01-02 Stats: 8 lines in 2 files changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16809.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16809/head:pull/16809 PR: https://git.openjdk.org/jdk/pull/16809
Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v2]
On Fri, 22 Mar 2024 02:17:58 GMT, David Holmes wrote: >> Adam Sotona has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 26 commits: >> >> - Merge remote-tracking branch 'openjdk/master' into >> JDK-8320396-verifier-extension >> - work in progress >> - work in progress >> - work in progress >> - work in progress >> - work in progress >> - removed string templates from test >> - work in progress >> - work in progress >> - work in progress >> - ... and 16 more: https://git.openjdk.org/jdk/compare/e41bc42d...54c4e9b9 > > src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java > line 56: > >> 54: >> 55: /** >> 56: * @see > href="https://raw.githubusercontent.com/openjdk/jdk/master/src/hotspot/share/classfile/classFileParser.cpp";>hotspot/share/classfile/classFileParser.cpp > > A reference to JVMS Ch4(?) would be appropriate here as that is what any > checks should be compliant with. Thank you for the review, I've added references to JVMS. - PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1535265201
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Fri, 22 Mar 2024 00:21:56 GMT, Chen Liang wrote:
>> src/java.base/share/classes/java/util/ImmutableCollections.java line 924:
>>
>>> 922: action.accept(REVERSE ? (E)e1 : e0); // implicit null
>>> check
>>> 923: action.accept(REVERSE ? e0 : (E)e1);
>>> 924: }
>>
>> Out of curiosity, how does the following fare performance-wise?
>>
>> Suggestion:
>>
>> action.accept((!REVERSE || e1 == EMPTY) ? e0 : (E)e1); //
>> implicit null check
>> if (e1 != EMPTY)
>> action.accept(!REVERSE ? (E)e1 : e0);
>
> BenchmarkMode CntScore Error Units
> ImmutableColls.forEachOverList thrpt 15 361.423 ± 8.751 ops/us
> ImmutableColls.forEachOverSet thrpt 15 79.158 ± 5.064 ops/us
> ImmutableColls.getOrDefault thrpt 15 244.012 ± 0.943 ops/us
> ImmutableColls.iterateOverList thrpt 15 152.598 ± 3.687 ops/us
> ImmutableColls.iterateOverSet thrpt 15 61.969 ± 4.453 ops/us
>
> The 3 results are also available at
> https://gist.github.com/f0b4336e5b1cf9c5299ebdbcd82232bf, where baseline is
> the master this patch currently is based on (which has WhiteBoxResizeTest
> failures), patch-0 being the current code, and patch-1 being your proposal
> (uncommited patch below).
>
> diff --git a/src/java.base/share/classes/java/util/ImmutableCollections.java
> b/src/java.base/share/classes/java/util/ImmutableCollections.java
> index fc232a521fb..f38b093cf60 100644
> --- a/src/java.base/share/classes/java/util/ImmutableCollections.java
> +++ b/src/java.base/share/classes/java/util/ImmutableCollections.java
> @@ -916,12 +916,9 @@ public T[] toArray(T[] a) {
> @Override
> @SuppressWarnings("unchecked")
> public void forEach(Consumer action) {
> -if (e1 == EMPTY) {
> -action.accept(e0); // implicit null check
> -} else {
> -action.accept(REVERSE ? (E)e1 : e0); // implicit null check
> -action.accept(REVERSE ? e0 : (E)e1);
> -}
> +action.accept((!REVERSE || e1 == EMPTY) ? e0 : (E) e1); //
> implicit null check
> +if (e1 != EMPTY)
> +action.accept(!REVERSE ? (E) e1 : e0);
> }
>
> @Override
>
>
>
> My testing shows that the existing version I have is most likely faster than
> your proposed version.
>
> Also note that the test failures are from WhiteBoxResizeTest that's fixed in
> latest master; I decide not to pull as not to invalidate the existing
> benchmark baselines.
Thanks. I was mostly trying to gauge what the bottleneck might be.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15834#discussion_r1535286326
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v15]
On Thu, 21 Mar 2024 23:38:30 GMT, Brent Christian wrote:
>> Classes in the `java.lang.ref` package would benefit from an update to bring
>> the spec in line with how the VM already behaves. The changes would focus on
>> _happens-before_ edges at some key points during reference processing.
>>
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the
>> registered cleaning action.
>>
>> This will bring Cleaner in line (or close) with the memory visibility
>> guarantees made for finalizers in [JLS
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object
>> to the start of a finalizer (§12.6) for that object."_
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Elaborate on 'surprising and undesirable effects' in reachabilityFence()
src/java.base/share/classes/java/lang/ref/Reference.java line 632:
> 630: * object. This might be the case if, for example, a usage in a
> user program
> 631: * had the form {@code new Resource().action();} which retains no
> other
> 632: * reference to this {@code Resource}. The
Reformat this line?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1535291214
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Thu, 21 Mar 2024 20:29:16 GMT, Mandy Chung wrote: > As @jaikiran suggests, `loadLibraryOnlyIfPresent()` should return false for > AIX as the file does not exist. The library implementation may need to adjust > as the current implementation uses a file path name. Had kept it as true, as it was in J9. but changing it to false works. So that would mean we are letting the java Runtime to handle paths that don't exist ? - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2014728820
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v2]
On Thu, 21 Mar 2024 17:06:23 GMT, Jaikiran Pai wrote: >>> But there is no actual file named libclang.a(libclang.so.16) in the >>> filesystem. >> So when the check is done if file exists, it fails,i.e it checks for >> libclang.a(libclang.so.16). >> >> I'll let Mandy and others more knowledgable of this area to correct me. From >> what I know, there was a change done a few releases back in the JDK >> (https://bugs.openjdk.org/browse/JDK-8275703) which accomodated macos >> platform where it too doesn't actually have the file on the filesystem in >> recent versions of macos. That change allowed platform specific code to >> dictate whether the file existence check needs to be done here in this >> loadLibrary code. That's done through the call to >> `jdk.internal.loader.ClassLoaderHelper.loadLibraryOnlyIfPresent()`. For >> macos, that implementation of that method resides in macos specific class >> here >> https://github.com/openjdk/jdk/blob/master/src/java.base/macosx/classes/jdk/internal/loader/ClassLoaderHelper.java#L41. >> For AIX too then, perhaps you could add an implementation which returns >> `false` (either always or in specific cases) and experiment with that to see >> if that's enough? > >> For AIX too then, perhaps you could add an implementation which returns >> false (either always or in specific cases) and experiment with that to see >> if that's enough? > > I see that you actually have a ClassLoaderHelper in this PR for AIX, but that > implementation in `loadLibraryOnlyIfPresent()` is currently returning `true` > and thus enforcing the file existence check. Was it intentional to have it to > `true`? Given the context of this change, shouldn't it be returning `false`? Correct. Had kept it as true, as it was in J9. but changing it to false works. So that would mean we are letting the java Runtime to handle paths that don't exist ? - PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1535320320
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Thu, 21 Mar 2024 22:23:02 GMT, Maurizio Cimadamore wrote: > > (I'm pessimistic) > > To summarize: I think that allowing version-specific names (even if > surrounded by parenthesis) in `System::loadLibrary` would be very odd. After > all, `System::loadLibrary` doesn't support versioned names, even on other > systems (and adding support for versioned library names across different > platforms is a much bigger effort). > > For this reason, the only thing that would make sense for `loadLibrary` to > support is `clang` which will be expanded (by `mapLibraryName`) to > `clang(libclang.so)`. But, even assuming this works: wouldn't we still have > an issue? As far as I understand (and given the code in this patch), we don't > really know (before calling `dlopen`) whether the suffix is needed or not: > whether it's an archive with an `.so` inside, or whether it's a plain `.so`. > So how can the behavior of `mapLibraryName` be deterministic? yes, i think the member needs to be specified in full. AIX archives can be both static and shared, so to mention the shared object member, we need to put the braces. But we also have cases, where a .so file when it doesn't exists will map deterministically to .a file , in which we use the loadlibrary. Specifying member objects is in addition to it. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2014741652
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Thu, 21 Mar 2024 22:23:02 GMT, Maurizio Cimadamore wrote: > For this reason, the only thing that would make sense for `loadLibrary` to > support is `clang` which will be expanded (by `mapLibraryName`) to > `clang(libclang.so)`. But, even assuming this works: wouldn't we still have > an issue? As far as I understand (and given the code in this patch), we don't > really know (before calling `dlopen`) whether the suffix is needed or not: > whether it's an archive with an `.so` inside, or whether it's a plain `.so`. > So how can the behavior of `mapLibraryName` be deterministic? The behaviour is deterministic when we have a case where, a .so file maps to .a file without specifying any members. This was the original intention for mapAlternateName so that we can use loadLibrary to load shared objects with .so format, and on failure we fallback and check if .a exists. But to mention the member object, it looks to me that we must specify the full name and there is no direct mapping. So then we restrict that to only System.load and not System.loadLibrary ? - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2014770717
Re: CFV: New Core Libraries Group Member: Per-Ake Minborg
Vote: Yes On 19/03/2024 16:19, Daniel Fuchs wrote: Hi, I hereby nominate Per-Ake Minborg (pminborg) [1] to Membership in the Core Libraries Group [4]. Per-Ake is an OpenJDK Reviewer, a committer in the Leyden and Panama projects, and a member of Oracle’s Java Core Libraries team. Per-Ake has been actively participating in the development of the JDK and Panama projects for several years, and is one of the co-author of the Implementation of Foreign Function and Memory API (Third Preview) [2]. His contributions include more than 80 commits in the JDK [3] Votes are due by 16:00 UTC on April 2, 2024. Only current Members of the Core Libraries Group [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [5]. best regards, -- daniel [1] https://openjdk.org/census#pminborg [2] https://github.com/openjdk/jdk/commit/cbccc4c8172797ea2f1b7c301d00add3f517546d [3] https://github.com/search?q=repo%3Aopenjdk%2Fjdk+author%3Aminborg&type=commits [4] https://openjdk.org/census#core-libs [5] https://openjdk.org/groups/#member-vote
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Thu, 21 Mar 2024 22:04:28 GMT, Maurizio Cimadamore
wrote:
> This problem seems relatively similar to what happens for versioned library
> names on e.g. linux distributions - e.g. `libclang.so.2`. In such cases users
> are stuck between a rock and a hard place: using
> `System.loadLibrary("libclang")` is too little: it will be expanded to
> `libclang.so`, and will not be found. But there's no way to pass the version
> name to `loadLibrary`, as, to do that, you have to also pass the `.so`
> extension, and that doesn't work. So the only option is to use the _full_
> absolute name with `System::load` (not `loadLibrary`).
>
> My feeling is that it is not possible to "infer" the desired member name
> (because we don't know which version the member library has), in the same way
> as, in my system, it is not possible to infer `libc.so.6` from just the
> library name `c`. As @JornVernee mentioned, this is why
> `SymbolLookup::libraryLookup` exists, so that library names can be passed
> straight to dlopen, w/o further interpretation from the JDK. It seems that at
> least part of the issue here is that the `jextract` code loads its own
> library (libclang) using `System::loadLibrary`, which doesn't do the right
> thing on AIX when only given "clang" as the library name. But I'm not exactly
> sure there's a fix for that at the `System::loadLibrary` level if `dlopen`
> really expects `clang.a(libclang.so.16)` to be passed as parameter.
>
> In other words, a fix to `mapLibraryName` only works as long as `dlopen` on
> AIX is able to do something with a name that is mechanically inferred, such
> as `clang.a(libclang.so)`. Is that the case? (I'm pessimistic)
Yes, dlopen expects the full name.
If i just pass clang in loadLibrary() and not the member i get exec error.
System error: Exec format error
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2014780371
Re: RFR: JDK-8326853 Missing @since tags for Charset related methods added in Java 10 [v9]
On Wed, 20 Mar 2024 18:04:50 GMT, Nizar Benalla wrote: >> # Issue >> - JDK-8326853 Incorrect `@since` Tags for Charset Related Methods Added in >> JDK 10 >> >> I changed the `@since` tags to better accurately show when the methods and >> constructors were introduced. > > Nizar Benalla has updated the pull request incrementally with two additional > commits since the last revision: > > - Update full name > - Update full name I spent some time running some tests to learn the process. - PR Comment: https://git.openjdk.org/jdk/pull/18032#issuecomment-2014786660
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container
On Mon, 11 Mar 2024 16:55:36 GMT, Severin Gehwolf wrote: > Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? Anyone willing to review this? - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2015043712
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Mon, 18 Mar 2024 17:43:45 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > trraling spaces `dlopen` really expects `libclang.a(libclang.so.16)`. I don't think this can currently be passed by any of the functions `System.loadLibrary`, `System.load` or `SymbolLookup.libraryLookup`. The `.16` suffix is only for clang 16. Clang 15 uses `.15`. Note that AIX uses archive members quite often. E.g. `libc.a(shr_64.o)`, `libpthreads.a(shr_xpg5_64.o)`, `libz.a(libz.so.1)`, ... Only the Java developer can select the right member. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2015274997
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Fri, 22 Mar 2024 14:53:17 GMT, Martin Doerr wrote: > Only the Java developer can select the right member. Right, and I think this problem is isomporphic to the problem we have in Linux distros of selecting between libclang.so.1 and libclang.so.2. `System::loadLibrary` cannot do it. So, I think it would feel odd if, say AIX was able to load precise version of a library using the member syntax, but in all other platforms that would not be possible. For better or worse, the story for now is: if you want version, use `System::load`. In this case, the problem might be that `System::load` doesn't work out of the box, as the path doesn't really exist. But this doesn't seem different to loading non-existing framework paths in MacOS - e.g. it is a special case that can be added for AIX (when using `System::load`). All that said, if one is doing `System::load/loadLibrary` just because they want to use FFM and `Linker`, a better way to do that is to just use `SymbolLookup::libraryLookup` directly, which gives you direct access to whatever string `dlopen` accepts in your system. Now, I realize that `jextract` is currently using `SymbolLookup::loaderLookup`, but I believe we can change that, which in turn will make adding support for AIX a bit easier. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2015299960
RFR: 8328812: Update and move siphash license
Updated and moved the license file. - Commit messages: - 8328812: Update and move siphash license Changes: https://git.openjdk.org/jdk/pull/18455/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18455&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328812 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18455.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18455/head:pull/18455 PR: https://git.openjdk.org/jdk/pull/18455
Re: RFR: JDK-8326853 Missing @ since tags for Charset related methods added in Java 10 [v8]
On Thu, 21 Mar 2024 16:50:31 GMT, Justin Lu wrote: >> Thank you justin > > Hi @nizarbenalla , you can comment `/integrate` whenever you're ready, and we > can sponsor the change to have it integrated. @justin-curtis-lu I realized there is a small issue with the commit message 8326853: Missing @since tags for Charset related methods added in Java 10 Reviewed-by: jlu, naoto there is a github user with the username `@since` and he will be tagged in the commit message, so there should be a space here. I added a space in the PR title - PR Comment: https://git.openjdk.org/jdk/pull/18032#issuecomment-2015380584
Re: RFR: 8328812: Update and move siphash license
On Fri, 22 Mar 2024 15:18:29 GMT, Jesper Wilhelmsson wrote: > Updated and moved the license file. Looks good. Lois - Marked as reviewed by lfoltan (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18455#pullrequestreview-1955180960
Integrated: 8328812: Update and move siphash license
On Fri, 22 Mar 2024 15:18:29 GMT, Jesper Wilhelmsson wrote: > Updated and moved the license file. This pull request has now been integrated. Changeset: ce7ebaa6 Author:Jesper Wilhelmsson URL: https://git.openjdk.org/jdk/commit/ce7ebaa606f96fdfee66d300b56022d9903b5ae3 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod 8328812: Update and move siphash license Reviewed-by: lfoltan - PR: https://git.openjdk.org/jdk/pull/18455
Integrated: JDK-8326853 Missing `@since` tags for Charset related methods added in Java 10
On Tue, 27 Feb 2024 16:28:26 GMT, Nizar Benalla wrote: > # Issue > - JDK-8326853 Incorrect `@since` Tags for Charset Related Methods Added in > JDK 10 > > I changed the `@since` tags to better accurately show when the methods and > constructors were introduced. This pull request has now been integrated. Changeset: 4d932d61 Author:Nizar Benalla Committer: Justin Lu URL: https://git.openjdk.org/jdk/commit/4d932d615c78f45516a4f136398e7610546065a6 Stats: 12 lines in 2 files changed: 10 ins; 0 del; 2 mod 8326853: Missing `@since` tags for Charset related methods added in Java 10 Reviewed-by: jlu, naoto - PR: https://git.openjdk.org/jdk/pull/18032
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Mon, 18 Mar 2024 17:43:45 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> trraling spaces
In case of jextract (jdk22 branch), we would then need something like the
following if we want AIX to behave like the other platforms?
diff --git a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
index 14eba30..c069c3b 100644
--- a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
+++ b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
@@ -27,11 +27,13 @@
package org.openjdk.jextract.clang.libclang;
+import java.io.File;
import java.lang.invoke.*;
import java.lang.foreign.*;
import java.nio.ByteOrder;
import java.util.*;
import java.util.function.*;
+import java.util.StringTokenizer;
import java.util.stream.*;
import static java.lang.foreign.ValueLayout.*;
@@ -101,8 +103,21 @@ public class Index_h {
}
static {
-String libName = System.getProperty("os.name").startsWith("Windows") ?
"libclang" : "clang";
-System.loadLibrary(libName);
+String osName = System.getProperty("os.name");
+String libName = "";
+if (osName.startsWith("AIX")) {
+String libPath = System.getProperty("java.library.path");
+StringTokenizer parser = new StringTokenizer(libPath, ":");
+while (parser.hasMoreTokens()) {
+libName = parser.nextToken() + "/libclang.a";
+File f = new File(libName);
+if (f.exists() && !f.isDirectory()) break;
+}
+SymbolLookup.libraryLookup(libName + "(libclang.so.16)",
Arena.global());
+} else {
+libName = osName.startsWith("Windows") ? "libclang" : "clang";
+System.loadLibrary(libName);
+}
}
static final SymbolLookup SYMBOL_LOOKUP = SymbolLookup.loaderLookup()
If I try this, I get "UnsatisfiedLinkError: unresolved symbol:
clang_createIndex". So, seems like the lib gets found, but symbols not loaded.
Seems like there are more changes needed.
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2015506765
RFR: 8328700: Unused import and variable should be deleted in regex package
Make use of an unused local variable probably intended to replace later casts. - Commit messages: - 8328700: Unused import and variable should be deleted in regex package Changes: https://git.openjdk.org/jdk/pull/18460/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18460&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328700 Stats: 7 lines in 2 files changed: 0 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18460.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18460/head:pull/18460 PR: https://git.openjdk.org/jdk/pull/18460
Re: RFR: 8328700: Unused import and variable should be deleted in regex package
On Fri, 22 Mar 2024 17:58:25 GMT, Raffaello Giulietti wrote: > Make use of an unused local variable probably intended to replace later casts. Marked as reviewed by redestad (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18460#pullrequestreview-1955510870
Re: RFR: 8328700: Unused import and variable should be deleted in regex package
On Fri, 22 Mar 2024 17:58:25 GMT, Raffaello Giulietti wrote: > Make use of an unused local variable probably intended to replace later casts. Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18460#pullrequestreview-195551
Re: RFR: 8328700: Unused import and variable should be deleted in regex package
On Fri, 22 Mar 2024 17:58:25 GMT, Raffaello Giulietti wrote: > Make use of an unused local variable probably intended to replace later casts. Marked as reviewed by bpb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18460#pullrequestreview-1955519288
Re: RFR: 8328700: Unused import and variable should be deleted in regex package
On Fri, 22 Mar 2024 17:58:25 GMT, Raffaello Giulietti wrote: > Make use of an unused local variable probably intended to replace later casts. Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18460#pullrequestreview-1955528280
Re: RFR: 8328700: Unused import and variable should be deleted in regex package
On Fri, 22 Mar 2024 17:58:25 GMT, Raffaello Giulietti wrote: > Make use of an unused local variable probably intended to replace later casts. Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18460#pullrequestreview-1955535154
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Fri, 22 Mar 2024 16:53:58 GMT, Martin Doerr wrote:
> In case of jextract (jdk22 branch), we would then need something like the
> following if we want AIX to behave like the other platforms?
>
> ```
> diff --git a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
> b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
> index 14eba30..c069c3b 100644
> --- a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
> +++ b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
> @@ -27,11 +27,13 @@
>
> package org.openjdk.jextract.clang.libclang;
>
> +import java.io.File;
> import java.lang.invoke.*;
> import java.lang.foreign.*;
> import java.nio.ByteOrder;
> import java.util.*;
> import java.util.function.*;
> +import java.util.StringTokenizer;
> import java.util.stream.*;
>
> import static java.lang.foreign.ValueLayout.*;
> @@ -101,8 +103,21 @@ public class Index_h {
> }
>
> static {
> -String libName = System.getProperty("os.name").startsWith("Windows")
> ? "libclang" : "clang";
> -System.loadLibrary(libName);
> +String osName = System.getProperty("os.name");
> +String libName = "";
> +if (osName.startsWith("AIX")) {
> +String libPath = System.getProperty("java.library.path");
> +StringTokenizer parser = new StringTokenizer(libPath, ":");
> +while (parser.hasMoreTokens()) {
> +libName = parser.nextToken() + "/libclang.a";
> +File f = new File(libName);
> +if (f.exists() && !f.isDirectory()) break;
> +}
> +SymbolLookup.libraryLookup(libName + "(libclang.so.16)",
> Arena.global());
> +} else {
> +libName = osName.startsWith("Windows") ? "libclang" : "clang";
> +System.loadLibrary(libName);
> +}
> }
>
> static final SymbolLookup SYMBOL_LOOKUP = SymbolLookup.loaderLookup()
> ```
>
> If I try this, I get "UnsatisfiedLinkError: unresolved symbol:
> clang_createIndex". So, seems like the lib gets found, but symbols not
> loaded. Seems like there are more changes needed.
Something like that seems good. When you call "find" on that lookup, it should
just forward to dlsym... I suggest you try first to simply use the lookup in a
standalone file (w/o jextract) and try to lookup a symbol in clang (e.g.
`clang_getClangVersion`), and see what happens.
If that doesn't work (likely), I'd suggest to write the equivalent C program
with dlopen/dlsym, and make sure that works, and also note which DLOPEN/DLSYM
parameters are used (maybe those are different from the ones used by the JDK?).
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2015773691
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Mon, 18 Mar 2024 17:43:45 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> trraling spaces
Ah, right. Thanks! I should use that `SymbolLookup`:
diff --git a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
index 14eba30..4f92f43 100644
--- a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
+++ b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
@@ -27,11 +27,13 @@
package org.openjdk.jextract.clang.libclang;
+import java.io.File;
import java.lang.invoke.*;
import java.lang.foreign.*;
import java.nio.ByteOrder;
import java.util.*;
import java.util.function.*;
+import java.util.StringTokenizer;
import java.util.stream.*;
import static java.lang.foreign.ValueLayout.*;
@@ -100,14 +102,27 @@ public class Index_h {
throw new IllegalArgumentException("Invalid type for ABI: " +
c.getTypeName());
}
+static SymbolLookup SYMBOL_LOOKUP;
+
static {
-String libName = System.getProperty("os.name").startsWith("Windows") ?
"libclang" : "clang";
-System.loadLibrary(libName);
+String osName = System.getProperty("os.name");
+String libName = "";
+if (osName.startsWith("AIX")) {
+String libPath = System.getProperty("java.library.path");
+StringTokenizer parser = new StringTokenizer(libPath, ":");
+while (parser.hasMoreTokens()) {
+libName = parser.nextToken() + "/libclang.a";
+File f = new File(libName);
+if (f.exists() && !f.isDirectory()) break;
+}
+SYMBOL_LOOKUP = SymbolLookup.libraryLookup(libName +
"(libclang.so.16)", Arena.global());
+} else {
+libName = osName.startsWith("Windows") ? "libclang" : "clang";
+System.loadLibrary(libName);
+SYMBOL_LOOKUP =
SymbolLookup.loaderLookup().or(Linker.nativeLinker().defaultLookup());
+}
}
-static final SymbolLookup SYMBOL_LOOKUP = SymbolLookup.loaderLookup()
-.or(Linker.nativeLinker().defaultLookup());
-
public static final ValueLayout.OfBoolean C_BOOL =
ValueLayout.JAVA_BOOLEAN;
public static final ValueLayout.OfByte C_CHAR = ValueLayout.JAVA_BYTE;
public static final ValueLayout.OfShort C_SHORT = ValueLayout.JAVA_SHORT;
The symbols get found and the JVM can really call into the
`libclang.a(libclang.so.16)`. Impressive! (That doesn't mean that jextract is
working. clang crashes the way we are calling it. Maybe because of a thread
stack size or other memory management problem.) So, there is basically a valid
solution for loading such libraries. Only not very smooth for Java programmers.
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2015811096
Re: RFR: JDK-8327640: Allow NumberFormat strict parsing [v4]
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
Justin Lu has updated the pull request incrementally with three additional
commits since the last revision:
- replace protected field for private fields in subclasses for consistency
- drop Format implNote as well
- setStrict and isStrict should be optional in NumberFormat
- specify and throw UOE as default
- override and implement in dFmt and cmpctNFmt
parseStrict should be protected, and utilized by subclasses. The public
methods should just
serve as API.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18339/files
- new: https://git.openjdk.org/jdk/pull/18339/files/c09a34dd..4edb4802
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18339&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18339&range=02-03
Stats: 148 lines in 4 files changed: 87 ins; 24 del; 37 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Fri, 22 Mar 2024 19:56:30 GMT, Martin Doerr wrote: > The symbols get found and the JVM can really call into the > `libclang.a(libclang.so.16)`. Impressive! (That doesn't mean that jextract is > working. clang crashes the way we are calling it. Maybe because of a thread > stack size or other memory management problem.) So, there is basically a > valid solution for loading such libraries. Only not very smooth for Java > programmers. Note that the jextract code itself depends on bindings generated via jextract (!!). So, I wonder if there might be some incompatibility in the generated layouts/descriptors, which is then causing the crash.The generated bindings are here: https://github.com/openjdk/jextract/tree/master/src/main/java/org/openjdk/jextract/clang/libclang These bindings are effectively shared across Linux/x64, Macos/x64, Macos/arm64 and Win/x64 - that is, all the layouts in there are valid and portable on these platforms (except for `C_LONG` on Windows, but libclang is quite disciplined and only uses `long long`). I'd try few steps: 1. check that your libclang.so is not broken, by calling `clang_version` in a C program and making sure that works w/o crashing 2. then try to do the same (w/o jextract) using FFM, from Java, and see if that still works If even (1) fails, you might just have a bad libclang, or one that is not for your system (not all the binary downloads in the LLVM website worked on my machine, even if they were supposedly compatible). If (1) succeds, but (2) fails, that would indicate some general issue with libclang and the JVM. There is an issue that we currently have to workaround, where libclang tries to install its own signal handlers, which mess up the JVM's signal handler to deal with NPEs (and that causes a random JVM crash). This is documented here: https://reviews.llvm.org/D23662 We try to call "setenv" to disable that logic, but that might fail or not be supported on your platform, so worth checking that. Finally, if (1) and (2) both succeed, but you get spurious JVM crashes with jextract, then I'd start looking at jextract's bindings (the ones in the folder above), pick a struct (e.g. CXCursor, or CXString) and then inspect the layout and make sure that corresponds to what the layout should be in AIX - it is possible that the AIX compiler inserts some extra padding, and then passing structs with the wrong size in and out of libclang would explain the issues. Hope this helps! - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2016231211
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Mon, 18 Mar 2024 17:43:45 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > trraling spaces I'd like to uplevel the discussion a bit. This PR started off to tweak the way in which `System::load` worked in AIX. We then discussed a bunch of options, talked about `Symbol::libraryLookup` and verified that this lookup allows to load libraries as expected in AIX. There's some jextract issues which need to be worked out, but that's outside the scope of this PR. That said, is there anything that we feel could be improved in terms of library loading support with `System::load` ? My conclusion was that, given that in this case we needed a fully versioned archive member, it is hard to implement and/or to expose as a simple `mapLibraryName` add-on. Is that correct? If you feel that there's not much that `System::load` can do for these cases, then I'd suggest we close this PR, and perhaps move the discussion about jextract either on `jextract-dev` or on `panama-dev`. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2016237758
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v14]
> Re-write the IndexOf code without the use of the pcmpestri instruction, only > using AVX2 instructions. This change accelerates String.IndexOf on average > 1.3x for AVX2. The benchmark numbers: > > > BenchmarkScore > Latest > StringIndexOf.advancedWithMediumSub 343.573 317.934 > 0.925375393x > StringIndexOf.advancedWithShortSub1 1039.081 1053.96 > 1.014319384x > StringIndexOf.advancedWithShortSub2 55.828110.541 > 1.980027943x > StringIndexOf.constantPattern 9.361 11.906 > 1.271872663x > StringIndexOf.searchCharLongSuccess 4.216 4.218 > 1.000474383x > StringIndexOf.searchCharMediumSuccess 3.133 3.216 > 1.02649218x > StringIndexOf.searchCharShortSuccess 3.763.761 > 1.000265957x > StringIndexOf.success 9.186 9.713 > 1.057369911x > StringIndexOf.successBig14.34146.343 > 3.231504079x > StringIndexOfChar.latin1_AVX2_String6220.918 12154.52 > 1.953814533x > StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 > 1.006629895x > StringIndexOfChar.latin1_SSE4_String6978.854 6818.689 > 0.977049957x > StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 > 0.967675646x > StringIndexOfChar.latin1_Short_String 7132.541 6863.359 > 0.962260014x > StringIndexOfChar.latin1_Short_char 16013.389 16162.437 > 1.009307711x > StringIndexOfChar.latin1_mixed_String 7386.12314771.622 > 1.15517x > StringIndexOfChar.latin1_mixed_char 9901.671 9782.245 > 0.987938803 Scott Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 46 commits: - Merge branch 'openjdk:master' into indexof - Cleaned up, ready for review - Pre-cleanup code - Add JMH. Add 16-byte compares to arrays_equals - Better method for mask creation - Merge branch 'openjdk:master' into indexof - Most cleanup done. - Remove header dependency - Works - needs cleanup - Passes tests. - ... and 36 more: https://git.openjdk.org/jdk/compare/bc739639...e079fc12 - Changes: https://git.openjdk.org/jdk/pull/16753/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=13 Stats: 4905 lines in 19 files changed: 4551 ins; 241 del; 113 mod Patch: https://git.openjdk.org/jdk/pull/16753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16753/head:pull/16753 PR: https://git.openjdk.org/jdk/pull/16753
Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v13]
On Thu, 22 Feb 2024 03:15:10 GMT, Scott Gibbons wrote: >> Re-write the IndexOf code without the use of the pcmpestri instruction, only >> using AVX2 instructions. This change accelerates String.IndexOf on average >> 1.3x for AVX2. The benchmark numbers: >> >> >> Benchmark Score >> Latest >> StringIndexOf.advancedWithMediumSub 343.573317.934 >> 0.925375393x >> StringIndexOf.advancedWithShortSub11039.081 1053.96 >> 1.014319384x >> StringIndexOf.advancedWithShortSub255.828110.541 >> 1.980027943x >> StringIndexOf.constantPattern9.361 11.906 >> 1.271872663x >> StringIndexOf.searchCharLongSuccess 4.216 4.218 >> 1.000474383x >> StringIndexOf.searchCharMediumSuccess3.133 3.216 >> 1.02649218x >> StringIndexOf.searchCharShortSuccess 3.763.761 >> 1.000265957x >> StringIndexOf.success9.186 >> 9.713 1.057369911x >> StringIndexOf.successBig 14.34146.343 >> 3.231504079x >> StringIndexOfChar.latin1_AVX2_String 6220.918 12154.52 >> 1.953814533x >> StringIndexOfChar.latin1_AVX2_char 5503.556 5540.044 >> 1.006629895x >> StringIndexOfChar.latin1_SSE4_String 6978.854 6818.689 >> 0.977049957x >> StringIndexOfChar.latin1_SSE4_char 5657.499 5474.624 >> 0.967675646x >> StringIndexOfChar.latin1_Short_String 7132.541 >> 6863.3590.962260014x >> StringIndexOfChar.latin1_Short_char 16013.389 16162.437 >> 1.009307711x >> StringIndexOfChar.latin1_mixed_String 7386.12314771.622 >> 1.15517x >> StringIndexOfChar.latin1_mixed_char9901.671 9782.245 >> 0.987938803 > > Scott Gibbons has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed some review coments; replaced hard-coded registers with > descriptive names. Re-opening this PR after some insightful comments, which resulted in a re-design. Now ready for review. Now showing ~1.6x performance gain on original StringIndexOf benchmark. I added a benchmark (StringIndexOfHuge) that measures performance on large-ish strings (e.g., 34-byte substring within a 2052-byte string). This benchmark performed on average ~14x original. Sorry for the large change, but it couldn't be done piecemeal. - PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-2016308399
