Re: RFR: 8256018: Adler32/CRC32/CRC32C missing reachabilityFence
On Tue, 10 Nov 2020 21:26:59 GMT, Lance Andersen wrote:
> Hi,
>
> Please review the fix for JDK-8256018 which addresses the issue that the
> update(ByteBuffer) methods of Adler32, CRC32, and CRC32C should use
> Reference.reachabilityFence to ensure that direct byte buffer are kept kept
> alive when they are accessed directly.
>
> The Mach5 jdk-tier1, jdk-tier2, jdk-tier3 as well as the
> jck:api/java_util/zip,jck:api/java_util/jar continue to run clean.
>
> Best
> Lance
Marked as reviewed by alanb (Reviewer).
src/java.base/share/classes/java/util/zip/Adler32.java line 103:
> 101: try {
> 102: adler = updateByteBuffer(adler,
> ((DirectBuffer)buffer).address(), pos, rem);
> 103:
spurious blank line here but otherwise looks good.
-
PR: https://git.openjdk.java.net/jdk/pull/1149
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]
On Tue, 10 Nov 2020 14:16:22 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the first incubation round >> of the foreign linker access API incubation >> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory >> access support (see JEP 393 [2] and associated pull request [3]). >> >> The main goal of this API is to provide a way to call native functions from >> Java code without the need of intermediate JNI glue code. In order to do >> this, native calls are modeled through the MethodHandle API. I suggest >> reading the writeup [4] I put together few weeks ago, which illustrates what >> the foreign linker support is, and how it should be used by clients. >> >> Disclaimer: the pull request mechanism isn't great at managing *dependent* >> reviews. For this reasons, I'm attaching a webrev which contains only the >> differences between this PR and the memory access PR. I will be periodically >> uploading new webrevs, as new iterations come out, to try and make the life >> of reviewers as simple as possible. >> >> A big thank to Jorn Vernee and Vladimir Ivanov - they are the main >> architects of all the hotspot changes you see here, and without their help, >> the foreign linker support wouldn't be what it is today. As usual, a big >> thank to Paul Sandoz, who provided many insights (often by trying the bits >> first hand). >> >> Thanks >> Maurizio >> >> Webrev: >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev >> >> Javadoc: >> >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html >> >> Specdiff (relative to [3]): >> >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254232 >> >> >> >> ### API Changes >> >> The API changes are actually rather slim: >> >> * `LibraryLookup` >> * This class allows clients to lookup symbols in native libraries; the >> interface is fairly simple; you can load a library by name, or absolute >> path, and then lookup symbols on that library. >> * `FunctionDescriptor` >> * This is an abstraction that is very similar, in spirit, to `MethodType`; >> it is, at its core, an aggregate of memory layouts for the function >> arguments/return type. A function descriptor is used to describe the >> signature of a native function. >> * `CLinker` >> * This is the real star of the show. A `CLinker` has two main methods: >> `downcallHandle` and `upcallStub`; the first takes a native symbol (as >> obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` >> and returns a `MethodHandle` instance which can be used to call the target >> native symbol. The second takes an existing method handle, and a >> `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a >> code stub allocated by the VM which acts as a trampoline from native code to >> the user-provided method handle. This is very useful for implementing >> upcalls. >>* This class also contains the various layout constants that should be >> used by clients when describing native signatures (e.g. `C_LONG` and >> friends); these layouts contain additional ABI classfication information (in >> the form of layout attributes) which is used by the runtime to *infer* how >> Java arguments should be shuffled for the native call to take place. >> * Finally, this class provides some helper functions e.g. so that clients >> can convert Java strings into C strings and back. >> * `NativeScope` >> * This is an helper class which allows clients to group together logically >> related allocations; that is, rather than allocating separate memory >> segments using separate *try-with-resource* constructs, a `NativeScope` >> allows clients to use a _single_ block, and allocate all the required >> segments there. This is not only an usability boost, but also a performance >> boost, since not all allocation requests will be turned into `malloc` calls. >> * `MemorySegment` >> * Only one method added here - namely `handoff(NativeScope)` which allows >> a segment to be transferred onto an existing native scope. >> >> ### Safety >> >> The foreign linker API is intrinsically unsafe; many things can go wrong >> when requesting a native method handle. For instance, the description of the >> native signature might be wrong (e.g. have too many arguments) - and the >> runtime has, in the general case, no way to detect such mismatches. For >> these reasons, obtaining a `CLinker` instance is a *restricted* operation, >> which can be enabled by specifying the usual JDK property >> `-Dforeign.restricted=permit` (as it's the case for other restricted method >> in the foreign memory API). >> >> ### Implementation changes >> >> The Java changes associated with `LibraryLookup` are relative >> straightforward; the only interesting thing to note here is that library
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v2]
On Tue, 10 Nov 2020 18:36:22 GMT, Aleksey Shipilev wrote: >> Hui Shi has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. The pull request contains one >> new commit since the last revision: >> >> 8255883: Avoid multiple GeneratedAccessor for same >> NativeMethod/ConstructorAccessorImpl object > > Minor nits here. @shipilev all comments are updated, please help review again! - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v2]
On Tue, 10 Nov 2020 18:28:11 GMT, Aleksey Shipilev wrote:
>> Hui Shi has refreshed the contents of this pull request, and previous
>> commits have been removed. The incremental views will show differences
>> compared to the previous content of the PR. The pull request contains one
>> new commit since the last revision:
>>
>> 8255883: Avoid multiple GeneratedAccessor for same
>> NativeMethod/ConstructorAccessorImpl object
>
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 37:
>
>> 35:
>> 36: class NativeConstructorAccessorImpl extends ConstructorAccessorImpl {
>> 37: private static final Unsafe unsafe = Unsafe.getUnsafe();
>
> `static final` field identifiers should be capitalized. Suggestion: `private
> static final Unsafe U = Unsafe.getUnsafe()`.
fixed
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 38:
>
>> 36: class NativeConstructorAccessorImpl extends ConstructorAccessorImpl {
>> 37: private static final Unsafe unsafe = Unsafe.getUnsafe();
>> 38: private static final long accessorGeneratedaOffset
>
> Typo "Generated*a*Offset". But also this is `static final`, so it should be
> e.g. `GENERATED_OFFSET`. The field can be just `generated` to match the name
> of this offset.
field name udpated
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 61:
>
>> 59: && !c.getDeclaringClass().isHidden()
>> 60: && !ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())
>> 61: && accessorGenerated == false
>
> Should be `!accessorGenerated`.
change to "generated == 0", as generated is int type now
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 62:
>
>> 60: && !ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())
>> 61: && accessorGenerated == false
>> 62: && unsafe.compareAndSetBoolean(this,
>> accessorGeneratedaOffset, false, true)) {
>
> `compareAndSetBoolean` gets us into the awkward territory of sub-word CASes.
> (Click through to `Unsafe.compareAndExchangeByte` to see what I am talking
> about). It is probably not relevant here, though. Still, might be as good to
> use `int` field for generated flag.
use int type now
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 72:
>
>> 70: parent.setDelegate(acc);
>> 71: } catch (Throwable t) {
>> 72: // in case Thowable happens in generateMethod
>
> Typo: `Thowable`.
updated
-
PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v5]
> …AccessorImpl object > > We met real problem when using protobuf with option optimized for code size, > detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 > > Optimize solution is adding a new boolean field to detect concurrent method > accessor generation in same NativeMethodAccessorImpl object, only one thread > is allowed to generate accessor, other threads still invoke in jni way until > parent's delegator is updated from NativeMethodAccessorImpl to generated > accessor. > > In common case, extra overhead is an atomic operation, compared with method > accessor generate, this cost is trivial. Hui Shi has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8255883: Avoid multiple GeneratedAccessor for same NativeMethod/ConstructorAccessorImpl object - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/abf34872..ec1531b2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1070=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1070=03-04 Stats: 16 lines in 2 files changed: 0 ins; 2 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/1070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1070/head:pull/1070 PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v4]
> …AccessorImpl object > > We met real problem when using protobuf with option optimized for code size, > detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 > > Optimize solution is adding a new boolean field to detect concurrent method > accessor generation in same NativeMethodAccessorImpl object, only one thread > is allowed to generate accessor, other threads still invoke in jni way until > parent's delegator is updated from NativeMethodAccessorImpl to generated > accessor. > > In common case, extra overhead is an atomic operation, compared with method > accessor generate, this cost is trivial. Hui Shi has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8255883: Avoid multiple GeneratedAccessor for same NativeMethod/ConstructorAccessorImpl object - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/7a1b274a..abf34872 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1070=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1070=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1070/head:pull/1070 PR: https://git.openjdk.java.net/jdk/pull/1070
RFR: 8255947: [macos] Signed macOS jpackage app doesn't filter spurious '-psn' argument
This is regression from JDK-8242302 and for some reason removing -psn argument code was removed during refactoring. Fixed be adding removing -psn argument back. Also, test was added to test this functionality. - Commit messages: - 8255947: [macos] Signed macOS jpackage app doesn't filter spurious '-psn' argument Changes: https://git.openjdk.java.net/jdk/pull/1154/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1154=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255947 Stats: 113 lines in 5 files changed: 100 ins; 10 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1154.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1154/head:pull/1154 PR: https://git.openjdk.java.net/jdk/pull/1154
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v3]
> …AccessorImpl object > > We met real problem when using protobuf with option optimized for code size, > detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 > > Optimize solution is adding a new boolean field to detect concurrent method > accessor generation in same NativeMethodAccessorImpl object, only one thread > is allowed to generate accessor, other threads still invoke in jni way until > parent's delegator is updated from NativeMethodAccessorImpl to generated > accessor. > > In common case, extra overhead is an atomic operation, compared with method > accessor generate, this cost is trivial. Hui Shi has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8255883: Avoid multiple GeneratedAccessor for same NativeMethod/ConstructorAccessorImpl object - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/8f65047e..7a1b274a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1070=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1070=01-02 Stats: 18 lines in 2 files changed: 0 ins; 0 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/1070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1070/head:pull/1070 PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v11]
On Fri, 6 Nov 2020 18:41:15 GMT, Jan Lahoda wrote: >> This is an update to javac and javadoc, to introduce support for Preview >> APIs, and generally improve javac and javadoc behavior to more closely >> adhere to JEP 12. >> >> The notable changes are: >> >> * adding support for Preview APIs (javac until now supported primarily only >> preview language features, and APIs associated with preview language >> features). This includes: >> * the @PreviewFeature annotation has boolean attribute "reflective", >> which should be true for reflective Preview APIs, false otherwise. This >> replaces the existing "essentialAPI" attribute with roughly inverted meaning. >> * the preview warnings for preview APIs are auto-suppressed as >> described in the JEP 12. E.g. the module that declares the preview API is >> free to use it without warnings >> * improving error/warning messages. Please see [1] for a list of >> cases/examples. >> * class files are only marked as preview if they are using a preview >> feature. [1] also shows if a class file is marked as preview or not. >> * the PreviewFeature annotation has been moved to jdk.internal.javac >> package (originally was in the jdk.internal package). >> * Preview API in JDK's javadoc no longer needs to specify @preview tag in >> the source files. javadoc will auto-generate a note for @PreviewFeature >> elements, see e.g. [2] and [3] (non-reflective and reflective API, >> respectively). A summary of preview elements is also provided [4]. Existing >> uses of @preview have been updated/removed. >> * non-JDK javadoc is also enhanced to auto-generate preview notes for uses >> of Preview elements, and for declaring elements using preview language >> features [5]. >> >> Please also see the CSR [6] for more information. >> >> [1] >> http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html >> [2] >> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html >> [3] >> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html >> [4] >> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html >> [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/ >> [6] https://bugs.openjdk.java.net/browse/JDK-8250769 > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing navigator for the PREVIEW page. I have a mild queasiness about this new overloaded use of the word "Summary", since "summary tables" are normally the summary of the contents of a declaration, like fields and methods of a class. That being said, the usage is primarily internal, and I have no overwhelmingly wonderful alternative, and (overloading aside) the term is accurate. So, OK for now. We can change it later if we want to. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 2949: > 2947: > 2948: /** > 2949: * Return the set of preview language features used to declare the > given element. "Returns" - Marked as reviewed by jjg (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: 8255787: Tag container tests that use cGroups with cgroups keyword
On Tue, 10 Nov 2020 21:24:25 GMT, Harold Seigel wrote: > Please review this small change to add a cgroups keyword to tests that use > cgroups. The fix was tested by running Mach5 container tests. Hi Harold, The fix looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1148
Integrated: 8254354: Add a withInvokeExactBehavior() VarHandle combinator
On Fri, 23 Oct 2020 17:47:36 GMT, Jorn Vernee wrote: > Hi, > > This patch adds an asExact() combinator to VarHandle, that will return a new > VarHandle that performs exact type checks, similar to > MethodHandle::invokeExact, to help developers catch inexact VarHandle usage, > which can lead to performance degradation. > > This is implemented using a boolean flag in VarForm. If the flag is set, the > exact type of the invocation is checked against the exact type in the > VarForm. If there is a mismatch, a WrongMethodTypeException is thrown. > > Other than that, there is also an asGeneric() combinator added that does the > inverse operation (thanks to Rémi for the suggestion). I've also added The > `@Hidden` annotation to the VarHandleGuards methods, as well as a > type-checking helper method called from the generic invocation lambda form, > so that the stack trace we get points at the location where the VarHandle is > being used. > > Thanks, > Jorn > > CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375 This pull request has now been integrated. Changeset: 0a41ca6b Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/0a41ca6b Stats: 1668 lines in 12 files changed: 1263 ins; 62 del; 343 mod 8254354: Add a withInvokeExactBehavior() VarHandle combinator Reviewed-by: psandoz, chegar - PR: https://git.openjdk.java.net/jdk/pull/843
Integrated: 8233332: Need to create exploded tests covering all forms of modules
On Fri, 6 Nov 2020 21:59:51 GMT, Alexey Semenyuk wrote: > 822: Need to create exploded tests covering all forms of modules This pull request has now been integrated. Changeset: d6f1463c Author:Alexey Semenyuk URL: https://git.openjdk.java.net/jdk/commit/d6f1463c Stats: 65 lines in 3 files changed: 59 ins; 1 del; 5 mod 822: Need to create exploded tests covering all forms of modules Reviewed-by: herrick, almatvee - PR: https://git.openjdk.java.net/jdk/pull/1103
Re: RFR: 8256018: Adler32/CRC32/CRC32C missing reachabilityFence
On Tue, 10 Nov 2020 21:26:59 GMT, Lance Andersen wrote: > Hi, > > Please review the fix for JDK-8256018 which addresses the issue that the > update(ByteBuffer) methods of Adler32, CRC32, and CRC32C should use > Reference.reachabilityFence to ensure that direct byte buffer are kept kept > alive when they are accessed directly. > > The Mach5 jdk-tier1, jdk-tier2, jdk-tier3 as well as the > jck:api/java_util/zip,jck:api/java_util/jar continue to run clean. > > Best > Lance Looks good, Lance. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1149
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]
On Tue, 10 Nov 2020 14:16:22 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the first incubation round >> of the foreign linker access API incubation >> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory >> access support (see JEP 393 [2] and associated pull request [3]). >> >> The main goal of this API is to provide a way to call native functions from >> Java code without the need of intermediate JNI glue code. In order to do >> this, native calls are modeled through the MethodHandle API. I suggest >> reading the writeup [4] I put together few weeks ago, which illustrates what >> the foreign linker support is, and how it should be used by clients. >> >> Disclaimer: the pull request mechanism isn't great at managing *dependent* >> reviews. For this reasons, I'm attaching a webrev which contains only the >> differences between this PR and the memory access PR. I will be periodically >> uploading new webrevs, as new iterations come out, to try and make the life >> of reviewers as simple as possible. >> >> A big thank to Jorn Vernee and Vladimir Ivanov - they are the main >> architects of all the hotspot changes you see here, and without their help, >> the foreign linker support wouldn't be what it is today. As usual, a big >> thank to Paul Sandoz, who provided many insights (often by trying the bits >> first hand). >> >> Thanks >> Maurizio >> >> Webrev: >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev >> >> Javadoc: >> >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html >> >> Specdiff (relative to [3]): >> >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254232 >> >> >> >> ### API Changes >> >> The API changes are actually rather slim: >> >> * `LibraryLookup` >> * This class allows clients to lookup symbols in native libraries; the >> interface is fairly simple; you can load a library by name, or absolute >> path, and then lookup symbols on that library. >> * `FunctionDescriptor` >> * This is an abstraction that is very similar, in spirit, to `MethodType`; >> it is, at its core, an aggregate of memory layouts for the function >> arguments/return type. A function descriptor is used to describe the >> signature of a native function. >> * `CLinker` >> * This is the real star of the show. A `CLinker` has two main methods: >> `downcallHandle` and `upcallStub`; the first takes a native symbol (as >> obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` >> and returns a `MethodHandle` instance which can be used to call the target >> native symbol. The second takes an existing method handle, and a >> `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a >> code stub allocated by the VM which acts as a trampoline from native code to >> the user-provided method handle. This is very useful for implementing >> upcalls. >>* This class also contains the various layout constants that should be >> used by clients when describing native signatures (e.g. `C_LONG` and >> friends); these layouts contain additional ABI classfication information (in >> the form of layout attributes) which is used by the runtime to *infer* how >> Java arguments should be shuffled for the native call to take place. >> * Finally, this class provides some helper functions e.g. so that clients >> can convert Java strings into C strings and back. >> * `NativeScope` >> * This is an helper class which allows clients to group together logically >> related allocations; that is, rather than allocating separate memory >> segments using separate *try-with-resource* constructs, a `NativeScope` >> allows clients to use a _single_ block, and allocate all the required >> segments there. This is not only an usability boost, but also a performance >> boost, since not all allocation requests will be turned into `malloc` calls. >> * `MemorySegment` >> * Only one method added here - namely `handoff(NativeScope)` which allows >> a segment to be transferred onto an existing native scope. >> >> ### Safety >> >> The foreign linker API is intrinsically unsafe; many things can go wrong >> when requesting a native method handle. For instance, the description of the >> native signature might be wrong (e.g. have too many arguments) - and the >> runtime has, in the general case, no way to detect such mismatches. For >> these reasons, obtaining a `CLinker` instance is a *restricted* operation, >> which can be enabled by specifying the usual JDK property >> `-Dforeign.restricted=permit` (as it's the case for other restricted method >> in the foreign memory API). >> >> ### Implementation changes >> >> The Java changes associated with `LibraryLookup` are relative >> straightforward; the only interesting thing to note here is that library
RFR: 8256018: Adler32/CRC32/CRC32C missing reachabilityFence
Hi, Please review the fix for JDK-8256018 which addresses the issue that the update(ByteBuffer) methods of Adler32, CRC32, and CRC32C should use Reference.reachabilityFence to ensure that direct byte buffer are kept kept alive when they are accessed directly. The Mach5 jdk-tier1, jdk-tier2, jdk-tier3 as well as the jck:api/java_util/zip,jck:api/java_util/jar continue to run clean. Best Lance - Commit messages: - Adler32/CRC32/CRC32C missing reachabilityFence Changes: https://git.openjdk.java.net/jdk/pull/1149/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1149=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256018 Stats: 23 lines in 3 files changed: 16 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/1149.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1149/head:pull/1149 PR: https://git.openjdk.java.net/jdk/pull/1149
RFR: 8255787: Tag container tests that use cGroups with cgroups keyword
Please review this small change to add a cgroups keyword to tests that use cgroups. The fix was tested by running Mach5 container tests. - Commit messages: - 8255787: Tag container tests that use cGroups with cgroups keyword Changes: https://git.openjdk.java.net/jdk/pull/1148/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1148=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255787 Stats: 20 lines in 15 files changed: 15 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/1148.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1148/head:pull/1148 PR: https://git.openjdk.java.net/jdk/pull/1148
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v15]
On Mon, 9 Nov 2020 16:31:23 GMT, Jorn Vernee wrote: >> src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 121: >> >>> 119: upcall_info.upcall_method.name, >>> upcall_info.upcall_method.sig, >>> 120: , thread); >>> 121: } >> >> This code shouldn't be in the cpu directory. This should be in >> SharedRuntime or in jni.cpp. It should have a JNI_ENTRY and not transition >> directly. I don't know what AttachCurrentThreadAsDaemon does. > > Roger that. > > We need the thread state transition though in case we get a random native > thread calling us. yikes. Does that work? - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8233332: Need to create exploded tests covering all forms of modules
On Fri, 6 Nov 2020 21:59:51 GMT, Alexey Semenyuk wrote: > 822: Need to create exploded tests covering all forms of modules Marked as reviewed by almatvee (Committer). - PR: https://git.openjdk.java.net/jdk/pull/1103
Re: RFR: 8247781: Day periods support [v12]
On Mon, 9 Nov 2020 01:37:39 GMT, Naoto Sato wrote: >> I've had a look tonight, but found two more problems! >> >> The comments at the start of `resolveTimeLenient()` indicate that setting >> the midpoint in `resolveTimeFields()` is a bad idea - it needs to be done >> inside `resolveTimeLenient()`. (This ensures user-defined fields can resolve >> to ChronoFields IIRC). Thus, the day period resolving would have to be split >> between the two methods. How important is the midpoint logic? Could it just >> be dropped? >> >> Secondly, we need to ensure that "24:00 midnight" (using HOUR_OF_DAY only) >> correctly parses to the end of day midnight, not the start of day or an >> exception. This is related to the problem above. >> >> I _think_ (but haven't confirmed everything yet) that the only thing that >> should be in `resolveTimeFields()` is the resolution of HOUR_OF_AMPM + >> dayPeriod to HOUR_OF_DAY (with `dayPeriod` then being set to `null`). >> Everything else should go in `resolveTimeLenient()` - the midpoint logic in >> the first if block (where time fields are defaulted), and the validation >> logic at the end of the method. > > Thanks for the insights. I believe I fixed both of the issues with the new > commit. Added a test case with the latest commit: https://github.com/openjdk/jdk/commit/a960804ff4d3f7df18e51fe59dcdcaf04542e10a - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v12]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added a test case for user defined temporal field resolution with day period. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/ccefe70c..a960804f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=10-11 Stats: 76 lines in 1 file changed: 76 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v2]
On Sun, 8 Nov 2020 05:07:07 GMT, Hui Shi wrote:
>> …AccessorImpl object
>>
>> We met real problem when using protobuf with option optimized for code size,
>> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
>>
>> Optimize solution is adding a new boolean field to detect concurrent method
>> accessor generation in same NativeMethodAccessorImpl object, only one thread
>> is allowed to generate accessor, other threads still invoke in jni way until
>> parent's delegator is updated from NativeMethodAccessorImpl to generated
>> accessor.
>>
>> In common case, extra overhead is an atomic operation, compared with method
>> accessor generate, this cost is trivial.
>
> Hui Shi has refreshed the contents of this pull request, and previous commits
> have been removed. The incremental views will show differences compared to
> the previous content of the PR. The pull request contains one new commit
> since the last revision:
>
> 8255883: Avoid multiple GeneratedAccessor for same
> NativeMethod/ConstructorAccessorImpl object
Minor nits here.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 37:
> 35:
> 36: class NativeConstructorAccessorImpl extends ConstructorAccessorImpl {
> 37: private static final Unsafe unsafe = Unsafe.getUnsafe();
`static final` field identifiers should be capitalized. Suggestion: `private
static final Unsafe U = Unsafe.getUnsafe()`.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 38:
> 36: class NativeConstructorAccessorImpl extends ConstructorAccessorImpl {
> 37: private static final Unsafe unsafe = Unsafe.getUnsafe();
> 38: private static final long accessorGeneratedaOffset
Typo "Generated*a*Offset". But also this is `static final`, so it should be
e.g. `GENERATED_OFFSET`. The field can be just `generated` to match the name of
this offset.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 61:
> 59: && !c.getDeclaringClass().isHidden()
> 60: && !ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())
> 61: && accessorGenerated == false
Should be `!accessorGenerated`.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 62:
> 60: && !ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())
> 61: && accessorGenerated == false
> 62: && unsafe.compareAndSetBoolean(this,
> accessorGeneratedaOffset, false, true)) {
`compareAndSetBoolean` gets us into the awkward territory of sub-word CASes.
(Click through to `Unsafe.compareAndExchangeByte` to see what I am talking
about). It is probably not relevant here, though. Still, might be as good to
use `int` field for generated flag.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 72:
> 70: parent.setDelegate(acc);
> 71: } catch (Throwable t) {
> 72: // in case Thowable happens in generateMethod
Typo: `Thowable`.
-
Changes requested by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8233332: Need to create exploded tests covering all forms of modules
On Fri, 6 Nov 2020 21:59:51 GMT, Alexey Semenyuk wrote: > 822: Need to create exploded tests covering all forms of modules looks good. - Marked as reviewed by herrick (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1103
Re: RFR: 8254354: Add a withInvokeExactBehavior() VarHandle combinator [v15]
> Hi, > > This patch adds an asExact() combinator to VarHandle, that will return a new > VarHandle that performs exact type checks, similar to > MethodHandle::invokeExact, to help developers catch inexact VarHandle usage, > which can lead to performance degradation. > > This is implemented using a boolean flag in VarForm. If the flag is set, the > exact type of the invocation is checked against the exact type in the > VarForm. If there is a mismatch, a WrongMethodTypeException is thrown. > > Other than that, there is also an asGeneric() combinator added that does the > inverse operation (thanks to Rémi for the suggestion). I've also added The > `@Hidden` annotation to the VarHandleGuards methods, as well as a > type-checking helper method called from the generic invocation lambda form, > so that the stack trace we get points at the location where the VarHandle is > being used. > > Thanks, > Jorn > > CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Add additional asserts to tests checking result of hasInvokeExactBehavior - Changes: - all: https://git.openjdk.java.net/jdk/pull/843/files - new: https://git.openjdk.java.net/jdk/pull/843/files/3097f080..4248f7ee Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=843=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=843=13-14 Stats: 15 lines in 1 file changed: 14 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/843.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/843/head:pull/843 PR: https://git.openjdk.java.net/jdk/pull/843
Re: RFR: 8254354: Add a withInvokeExactBehavior() VarHandle combinator [v14]
On Tue, 10 Nov 2020 16:27:48 GMT, Chris Hegarty wrote: >> Jorn Vernee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Re-order javadoc > > LGTM. > > The test could be sprinkled with a number of assertions related to the > invocation behaviour, a.k.a `hasInvokeExactBehavior`; either the newly > specified default behaviour assertFalse(vh. hasInvokeExactBehavior()), or > post switching to exact: assertTrue(vh. hasInvokeExactBehavior()) @ChrisHegarty Good suggestion! Added. - PR: https://git.openjdk.java.net/jdk/pull/843
Re: 'Find' method for Iterable
Did this discussion get lost?
On Sun, Sep 20, 2020 at 1:27 AM Nir Lisker wrote:
> While it might not be difficult to add a find() method to Iterable, why
>> limit it to
>> the find operation, and what about all the other operations available on
>> Stream?
>
>
> Good question. I would say it's a matter of how much it is used and what
> it takes to implement it. The find operation is a lot more common than
> reduce from what I observe, for example, so I wouldn't suggest reduce to be
> added..A map(Function) operation would require creating a new
> collection/iterable internally, and that can be messy (you could
> preemptively create and pass your own, but then I doubt the worthiness of
> it). forEach already exists. I just don't see anything enticing.
>
> Maybe what's necessary is a way to convert an Iterable to a Stream.
>
>
> Most Iterables are Collections and arrays, and these are easy to convert,
> so I'm not sure if that really helps. Besides,the idea is to avoid Stream,
> as I've mentioned, due to the cumbersomeness and the overhead of creating a
> stream. If I need to do
>
> iterable.stream().filter(person -> person.id == 123456).findAny/First()
>
> then I didn't really solve my problem.
>
> On the other hand, your examples use a list. The List interface already
>> has methods
>> indexOf/lastIndexOf which search the list for a particular object that's
>> compared
>> using equals(). It seems reasonable to consider similar methods that take
>> a
>> predicate instead of an object.
>
>
> I could have used a Set just as well. As for indexOf(Predicate), I
> would say that it is useful (but personally, I hardly ever need the index
> of an object, I need the object itself). Interestingly,
> removeIf(Predicate) exists, but remove(Predicate) doesn't. I would
> think twice before suggesting to add it though.
>
> Ultimately, you have access to a lot of analytics and codebase scans. If
> you know which patterns are used a lot more than others it would be a good
> guide. If there are a lot of iterations in order to find an object, its
> index, or to remove it (or something else), perhaps it's worth supplying
> these methods. After all, forEach(Consumer) exists and it iterates while
> calling accept(t) - not that different from iterating with test(t).
>
> P.S. lastIndexOf I find odd in the sense that it's the only method I found
> that iterates backwards, We don't have, removeLast, removeIfBackwards,
> forEachBackwards, a backwards for-each loop, or addLast (the latter is
> add(list.size()-1, e); ).
>
> - Nir
>
> On Thu, Sep 17, 2020 at 1:32 AM Stuart Marks
> wrote:
>
>>
>>
>> On 9/16/20 1:59 PM, Remi Forax wrote:
>> > - Mail original -
>> >> De: "Nir Lisker"
>> >> À: "core-libs-dev"
>> >> Envoyé: Lundi 14 Septembre 2020 20:56:27
>> >> Objet: 'Find' method for Iterable
>> >
>> >> Hi,
>> >>
>> >> This has probably been brought up at some point. When we need to find
>> an
>> >> item in a collection based on its properties, we can either do it in a
>> >> loop, testing each item, or in a stream with filter and findFirst/Any.
>> >>
>> >> I would think that a method in Iterable be useful, along the lines
>> of:
>> >>
>> >> public Optional find(Predicate condition) {
>> >> Objects.requireNonNull(condition);
>> >> for (T t : this) {
>> >> if (condition.test(t)) {
>> >> return Optional.of(t);
>> >> }
>> >> }
>> >> return Optional.empty();
>> >> }
>> >>
>> >> With usage:
>> >>
>> >> list.find(person -> person.id == 123456);
>> >>
>> >> There are a few issues with the method here such as t being null in
>> >> null-friendly collections and the lack of bound generic types, but this
>> >> example is just used to explain the intention.
>> >>
>> >> It will be an alternative to
>> >>
>> >> list.stream().filter(person -> person.id == 123456).findAny/First()
>> >> (depending on if the collection is ordered or not)
>> >>
>> >> which doesn't create a stream, similar to Iterable#forEach vs
>> >> Stream#forEach.
>> >>
>> >> Maybe with pattern matching this would become more appetizing.
>> >
>> > During the development of Java 8, we first tried to use
>> Iterator/Iterable instead of using a novel interface Stream.
>> > But a Stream cleanly separate the lazy side effect free API from the
>> mutable one (Collection) and can be optimized better by the VM (it's a push
>> API instead of being a pull API).
>> >
>> > The other question is why there is no method find() on Collection, i
>> believe it's because while find() is ok for any DB API, find() is dangerous
>> on a Collection because the execution time is linear, so people may use it
>> instead of using a Map.
>>
>>
>> Hi Nir,
>>
>> Rémi is correct to point out this distinction between the lazy operations
>> (which
>> appear on Stream) and the eager (and possibly mutating) operations on
>> Collections. I
>> think we want to preserve this distinction.
>>
>> While it might not be difficult to add a find() method to Iterable, why
>>
Integrated: 8256066: Tests use deprecated TestNG API that is no longer available in new versions
On Tue, 10 Nov 2020 17:41:28 GMT, Mandy Chung wrote: > @ExpectedExceptions is no longer available in TestNG 7.*. Update the tests > to use @Test(expectedExceptions = "...") instead. This pull request has now been integrated. Changeset: 6d8acd26 Author:Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/6d8acd26 Stats: 12 lines in 3 files changed: 0 ins; 6 del; 6 mod 8256066: Tests use deprecated TestNG API that is no longer available in new versions Reviewed-by: jjg - PR: https://git.openjdk.java.net/jdk/pull/1145
Re: Integrated: 8256066: Tests use deprecated TestNG API that is no longer available in new versions
On Tue, 10 Nov 2020 17:41:28 GMT, Mandy Chung wrote: > @ExpectedExceptions is no longer available in TestNG 7.*. Update the tests > to use @Test(expectedExceptions = "...") instead. Marked as reviewed by jjg (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1145
Integrated: 8256066: Tests use deprecated TestNG API that is no longer available in new versions
@ExpectedExceptions is no longer available in TestNG 7.*. Update the tests to use @Test(expectedExceptions = "...") instead. - Commit messages: - 8256066: Tests use deprecated TestNG API that is no longer available in new versions Changes: https://git.openjdk.java.net/jdk/pull/1145/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1145=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256066 Stats: 12 lines in 3 files changed: 0 ins; 6 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/1145.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1145/head:pull/1145 PR: https://git.openjdk.java.net/jdk/pull/1145
Re: RFR: 8254354: Add a withInvokeExactBehavior() VarHandle combinator [v14]
On Tue, 10 Nov 2020 15:02:08 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This patch adds an asExact() combinator to VarHandle, that will return a new
>> VarHandle that performs exact type checks, similar to
>> MethodHandle::invokeExact, to help developers catch inexact VarHandle usage,
>> which can lead to performance degradation.
>>
>> This is implemented using a boolean flag in VarForm. If the flag is set, the
>> exact type of the invocation is checked against the exact type in the
>> VarForm. If there is a mismatch, a WrongMethodTypeException is thrown.
>>
>> Other than that, there is also an asGeneric() combinator added that does the
>> inverse operation (thanks to Rémi for the suggestion). I've also added The
>> `@Hidden` annotation to the VarHandleGuards methods, as well as a
>> type-checking helper method called from the generic invocation lambda form,
>> so that the stack trace we get points at the location where the VarHandle is
>> being used.
>>
>> Thanks,
>> Jorn
>>
>> CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Re-order javadoc
LGTM.
The test could be sprinkled with a number of assertions related to the
invocation behaviour, a.k.a `hasInvokeExactBehavior`; either the newly
specified default behaviour assertFalse(vh. hasInvokeExactBehavior()), or post
switching to exact: assertTrue(vh. hasInvokeExactBehavior())
test/jdk/java/lang/invoke/VarHandles/VarHandleTestExact.java line 82:
> 80: throws NoSuchFieldException, IllegalAccessException {
> 81: if (ro) throw new SkipException("Can not test setter with read
> only field");
> 82: VarHandle vh = MethodHandles.lookup().findVarHandle(Widget.class,
> fieldBaseName + "_RW", fieldType);
Quite trivially we can assertFalse(vh. hasInvokeExactBehavior())
test/jdk/java/lang/invoke/VarHandles/VarHandleTestExact.java line 92:
> 90: }
> 91:
> 92: vh = vh.withInvokeExactBehavior();
Quite trivially we can assertTrue(vh. hasInvokeExactBehavior())
-
Marked as reviewed by chegar (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/843
Re: RFR: 8254354: Add a withInvokeExactBehavior() VarHandle combinator [v14]
> Hi, > > This patch adds an asExact() combinator to VarHandle, that will return a new > VarHandle that performs exact type checks, similar to > MethodHandle::invokeExact, to help developers catch inexact VarHandle usage, > which can lead to performance degradation. > > This is implemented using a boolean flag in VarForm. If the flag is set, the > exact type of the invocation is checked against the exact type in the > VarForm. If there is a mismatch, a WrongMethodTypeException is thrown. > > Other than that, there is also an asGeneric() combinator added that does the > inverse operation (thanks to Rémi for the suggestion). I've also added The > `@Hidden` annotation to the VarHandleGuards methods, as well as a > type-checking helper method called from the generic invocation lambda form, > so that the stack trace we get points at the location where the VarHandle is > being used. > > Thanks, > Jorn > > CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Re-order javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/843/files - new: https://git.openjdk.java.net/jdk/pull/843/files/ea7c920c..3097f080 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=843=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=843=12-13 Stats: 7 lines in 1 file changed: 4 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/843.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/843/head:pull/843 PR: https://git.openjdk.java.net/jdk/pull/843
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v15]
On Mon, 9 Nov 2020 04:10:30 GMT, David Holmes wrote: >> Maurizio Cimadamore has updated the pull request with a new target base due >> to a merge or a rebase. The pull request now contains 64 commits: >> >> - Merge branch '8254162' into 8254231_linker >> - Fix post-merge issues caused by 8219014 >> - Merge branch 'master' into 8254162 >> - Addess remaining feedback from @AlanBateman and @mrserb >> - Address comments from @AlanBateman >> - Fix typo in upcall helper for aarch64 >> - Merge branch '8254162' into 8254231_linker >> - Merge branch 'master' into 8254162 >> - Fix issues with derived buffers and IO operations >> - More 32-bit fixes for TestLayouts >> - ... and 54 more: >> https://git.openjdk.java.net/jdk/compare/a50fdd54...b38afb3f > > A high-level scan through - mostly VM files. I've addressed the open review comments. One of the commits is a bigger change that removes the code duplication in the upcall handler code. The initialization code is moved to the ProgrammableUpcallHandler class' constructor instead. That class is then lazily instantiated using a local `static` variable (see ProgrammableUpcallHandler::instance), which since C++11 guarantees thread-safe initialize-once behaviour. Along with those changes I've also removed some other duplicated code in the native invoker code (ProgrammableInvokerGenerator), cleaned up the includes most files, as well as using a JNI_ENTRY function when doing upcalls (as requested), by splitting the current functionality of the upcall_helper function in 2; one function that does the thread attach, and then other that does the actual upcall (which is the one using JNI_ENTRY). (see: https://github.com/openjdk/jdk/pull/634/commits/719224ca9dc70fce6d28885acfb362fee715ebbd). As discussed, changing ProgrammableUpcallHandler to be a well-known class didn't work, since it is not in java.base. Changes: - Merge both versions of upcall_init and move the code to (the constructor of) ProgrammableUpcallHandler. Using the same lazy singleton pattern as for ForeignGlobals to make initialization thread-safe. - Merge both PorgrammableInvokeGenerator classes into a shared ProgrammableInvoke::Generator class. - Also move ProgrammableStub to ProgrammableInvoke::Stub for better name-spacing - Also move native_invoker_size constant to ProgrammableInvoker (we now have 1 instead of 2) - Merge ProgrammableInvoker::Generator::generate and top-level generate_invoke_native functions (avoiding the need to forward fields) - Split upcall_helper method into ProgrammableUpcallHandler::attach_thread_and_do_upcall and upcall_helper. The former does the thread attach/detach, the latter does the actual upcall. - Add a few comments to ProgrammableUpcallHandler::generate_upcall_stub - Remove unused imports The rest of the review comments were addressed in a set of smaller commits (see timeline on GitHub). The changes therein are: - remove blank line in thread.hpp - Remove os::is_MP() check - remove excessive asserts in ProgrammableInvoker::invoke_native - Extra space after if in jni_util_md (Windows) - Relax ret_addr_offset() assert - Sort includes alphabetically in upcallHandler CPU files - Check result of AttachCurrentThread call - Set copyright year for added files to 2020 (I didn't touch the ARM copyright headers) That should address all open review comments (but please let me know if I've missed something). Thanks for the reviews so far. - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]
> This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and associated pull request [3]). > > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate JNI glue code. In order to do > this, native calls are modeled through the MethodHandle API. I suggest > reading the writeup [4] I put together few weeks ago, which illustrates what > the foreign linker support is, and how it should be used by clients. > > Disclaimer: the pull request mechanism isn't great at managing *dependent* > reviews. For this reasons, I'm attaching a webrev which contains only the > differences between this PR and the memory access PR. I will be periodically > uploading new webrevs, as new iterations come out, to try and make the life > of reviewers as simple as possible. > > A big thank to Jorn Vernee and Vladimir Ivanov - they are the main architects > of all the hotspot changes you see here, and without their help, the foreign > linker support wouldn't be what it is today. As usual, a big thank to Paul > Sandoz, who provided many insights (often by trying the bits first hand). > > Thanks > Maurizio > > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library by name, or absolute path, > and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory layouts for the function > arguments/return type. A function descriptor is used to describe the > signature of a native function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes a native symbol (as > obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` and > returns a `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, and a > `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a > code stub allocated by the VM which acts as a trampoline from native code to > the user-provided method handle. This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures (e.g. `C_LONG` and > friends); these layouts contain additional ABI classfication information (in > the form of layout attributes) which is used by the runtime to *infer* how > Java arguments should be shuffled for the native call to take place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than allocating separate memory segments > using separate *try-with-resource* constructs, a `NativeScope` allows clients > to use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a performance boost, since not all > allocation requests will be turned into `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For instance, the description of the > native signature might be wrong (e.g. have too many arguments) - and the > runtime has, in the general case, no way to detect such mismatches. For these > reasons, obtaining a `CLinker` instance is a *restricted* operation, which > can be enabled by specifying the usual JDK property > `-Dforeign.restricted=permit` (as it's the case for other restricted method > in the foreign memory API). > > ### Implementation changes > > The Java changes associated with `LibraryLookup` are relative > straightforward; the only interesting thing to note here is that library > loading does _not_ depend on class loaders, so `LibraryLookup` is not subject > to the same restrictions which apply to JNI library loading (e.g. same > library
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v17]
> This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and associated pull request [3]). > > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate JNI glue code. In order to do > this, native calls are modeled through the MethodHandle API. I suggest > reading the writeup [4] I put together few weeks ago, which illustrates what > the foreign linker support is, and how it should be used by clients. > > Disclaimer: the pull request mechanism isn't great at managing *dependent* > reviews. For this reasons, I'm attaching a webrev which contains only the > differences between this PR and the memory access PR. I will be periodically > uploading new webrevs, as new iterations come out, to try and make the life > of reviewers as simple as possible. > > A big thank to Jorn Vernee and Vladimir Ivanov - they are the main architects > of all the hotspot changes you see here, and without their help, the foreign > linker support wouldn't be what it is today. As usual, a big thank to Paul > Sandoz, who provided many insights (often by trying the bits first hand). > > Thanks > Maurizio > > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library by name, or absolute path, > and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory layouts for the function > arguments/return type. A function descriptor is used to describe the > signature of a native function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes a native symbol (as > obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` and > returns a `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, and a > `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a > code stub allocated by the VM which acts as a trampoline from native code to > the user-provided method handle. This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures (e.g. `C_LONG` and > friends); these layouts contain additional ABI classfication information (in > the form of layout attributes) which is used by the runtime to *infer* how > Java arguments should be shuffled for the native call to take place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than allocating separate memory segments > using separate *try-with-resource* constructs, a `NativeScope` allows clients > to use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a performance boost, since not all > allocation requests will be turned into `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For instance, the description of the > native signature might be wrong (e.g. have too many arguments) - and the > runtime has, in the general case, no way to detect such mismatches. For these > reasons, obtaining a `CLinker` instance is a *restricted* operation, which > can be enabled by specifying the usual JDK property > `-Dforeign.restricted=permit` (as it's the case for other restricted method > in the foreign memory API). > > ### Implementation changes > > The Java changes associated with `LibraryLookup` are relative > straightforward; the only interesting thing to note here is that library > loading does _not_ depend on class loaders, so `LibraryLookup` is not subject > to the same restrictions which apply to JNI library loading (e.g. same > library
Re: RFR: 8066622 8066637: remove deprecated using java.io.File.toURL() in internal classes
On Sun, 8 Nov 2020 16:58:24 GMT, Phil Race wrote: > You reference a desktop bug that discusses many, many deprecations ... Yep. In my opinion this is a bot problem here and need other place to discuss. > Yet you propose to fix precisely one of these. @prrace: Not really. The way to work with problems differ. Both bugs - maybe these are more quality change requests than bugs - are bullet lists and are created on build process. The different is, I work on single quality tests and remove deprecated code from source base. So in my clean Java build the method java.io.File.toUrl() does not(!) exist. Because java.io.File.toURL() doesn't exist, also two patches are needed for JDK build process (internal I use diff-patch). > And I'd like to hear whether you actually _tested_ splashscreen with your > change ? I see no sign that you did. I'm not sure, what you want to listen. I work with a clean Java build with patch to remove java.io.File.toURL() and change the collateral damages. - PR: https://git.openjdk.java.net/jdk/pull/1108
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v26]
On Mon, 9 Nov 2020 16:07:13 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation (see JEP 393 [1]). This >> iteration focus on improving the usability of the API in 3 main ways: >> >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee that the memory will be >> deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class has been added, which >> defines several useful dereference routines; these are really just thin >> wrappers around memory access var handles, but they make the barrier of >> entry for using this API somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not the same as it used to be; it >> used to be the case that a memory address could (sometimes, not always) have >> a back link to the memory segment which originated it; additionally, memory >> access var handles used `MemoryAddress` as a basic unit of dereference. >> >> This has all changed as per this API refresh; now a `MemoryAddress` is just >> a dumb carrier which wraps a pair of object/long addressing coordinates; >> `MemorySegment` has become the star of the show, as far as dereferencing >> memory is concerned. You cannot dereference memory if you don't have a >> segment. This improves usability in a number of ways - first, it is a lot >> easier to wrap native addresses (`long`, essentially) into a >> `MemoryAddress`; secondly, it is crystal clear what a client has to do in >> order to dereference memory: if a client has a segment, it can use that; >> otherwise, if the client only has an address, it will have to create a >> segment *unsafely* (this can be done by calling >> `MemoryAddress::asSegmentRestricted`). >> >> A list of the API, implementation and test changes is provided below. If >> you have any questions, or need more detailed explanations, I (and the rest >> of the Panama team) will be happy to point at existing discussions, and/or >> to provide the feedback required. >> >> A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without >> whom the work on shared memory segment would not have been possible; also >> I'd like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. >> >> Thanks >> Maurizio >> >> Javadoc: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a carrier is one of the usual >> suspect (a Java primitive, minus `boolean`); the access mode can be simple >> (e.g. access base address of given segment), or indexed, in which case the >> accessor takes a segment and either a low-level byte offset,or a high level >> logical index. The classification is reflected in the naming scheme (e.g. >> `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which it is easy to derive all >> the other handles using plain var handle combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both `MemoryAddress` and >>
RFR: 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer
Finalizer calls Reference.get() from the Finalizer to acquire the finalizee. Concurrent reference processing GCs like Shenandoah and ZGC would return NULL for unreachable referents, and thus would not call finalize() on them. ZGC works around this by fixing the referent before enqueuing, so that the barrier would take the fast-path, but Shenandoah cannot do this. It is ok to bypass the barrier altogether in this place, because the FinalReference is inactive and marking and reference-discovery treat inactive FinalReferences like strong references. Testing: - [x] hotspot_gc_shenandoah - [x] tier1 +UseShenandoahGC +ShenandoahVerify - [x] tier2 +UseShenandoahGC +ShenandoahVerify - [ ] tier1 - [ ] tier2 - Commit messages: - 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer Changes: https://git.openjdk.java.net/jdk/pull/1140/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1140=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256106 Stats: 8 lines in 2 files changed: 7 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1140.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1140/head:pull/1140 PR: https://git.openjdk.java.net/jdk/pull/1140
Re: RFR: 8180352: Add Stream.toList() method [v2]
On Sun, 8 Nov 2020 15:55:58 GMT, Peter Levart wrote:
>> Hi Stuart,
>>
>> I would like to discuss the serialization. You introduce new
>> CollSer.IMM_LIST_NULLS type of immutable collection. This means that if this
>> change goes into JDK16 for example, JDK15 and before will not be able to
>> deserialize such list as they know nothing about IMM_LIST_NULLS even if such
>> lists don't contain nulls. The reason you say to chose new type of
>> serialization format is the following:
>>
>>> "Suppose I had an application that created a data structure that used lists
>>> from List.of(), and I had a global assertion over that structure that it
>>> contained no nulls. Further suppose that I serialized and deserizalized
>>> this structure. I'd want that assertion to be preserved after
>>> deserialization. If another app (or a future version of this app) created
>>> the structure using Stream.to
>>> List(), this would allow nulls to leak into that structure and violate
>>> that assertion. Therefore, the result of Stream.toList() should not be
>>> serialization-compatible with List.of() et. al. That's why there's the new
>>> IMM_LIST_NULLS tag in the serial format"
>>
>> I don't quite get this reasoning. Let's try to decompose the reasoning
>> giving an example. Suppose we had the following data structure:
>>
>> public class Names implements Serializable {
>> private final List names;
>> Names(List names) {
>> this.names = names;
>> }
>> public List names() { return names; }
>> }
>>
>> App v1 creates such structures using new Names(List.of(...)) and
>> serializes/deserializes them. They keep the invariant that no nulls are
>> present. Now comes App v2 that starts using new Names(stream.toList()) which
>> allows nulls to be present. When such Names instance from app v2 is
>> serialized and then deserialized in app v1, nulls "leak" into data structure
>> of app v1 that does not expect them.
>>
>> But the question is how does having a separate CollSer.IMM_LIST_NULLS type
>> prevent that from happening?
>
> I can see that having a separate IMM_LIST_NULLS type might be necessary to
> preserve the allows-null/disallows-null behaviour of indexOf and lastIndexOf
> methods...
>
> NOTE ALSO that ListN.equals(o) and ListN.hashCode() are inherited from
> AbstractImmutableList:
>
> @Override
> public boolean equals(Object o) {
> if (o == this) {
> return true;
> }
>
> if (!(o instanceof List)) {
> return false;
> }
>
> Iterator oit = ((List) o).iterator();
> for (int i = 0, s = size(); i < s; i++) {
> if (!oit.hasNext() || !get(i).equals(oit.next())) {
> return false;
> }
> }
> return !oit.hasNext();
> }
> and
> public int hashCode() {
> int hash = 1;
> for (int i = 0, s = size(); i < s; i++) {
> hash = 31 * hash + get(i).hashCode();
> }
> return hash;
> }
>
> ...which means they will throw NPE when the list contains null. The same goes
> for SubList.
> @plevart wrote:
>
> > But the question is how does having a separate CollSer.IMM_LIST_NULLS type
> > prevent that from happening?
>
> When a serialized list with IMM_LIST_NULLS is deserialized on an older JDK,
> it'll throw InvalidObjectException since that tag isn't valid on older JDKs.
> Obviously this is still an error, but it's a fail-fast approach that avoids
> letting nulls leak into a data structure where they might cause a problem
> some arbitrary time later.
>
Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both
apps run on JDK16+, there will be no exception.
What I'm trying to say is that the same problem of injecting unexpected nulls
via serialization/deserialization can happen also if App V2 starts using
ArrayList to construct the data structure and serialize it while App V1
deserializes it and expects non-null values only. App V1 would already have to
guard against null values during deserialization in that case, because
possibility of null values in deserialized data structure is nothing new for
App V1.
-
PR: https://git.openjdk.java.net/jdk/pull/1026
