Integrated: 8288068: Javadoc contains spurious reference to CLinker
On Wed, 8 Jun 2022 21:07:13 GMT, Maurizio Cimadamore wrote: > This simple patch fixes a bunch of references to "CLinker" and replaces them > with "Linker", following the API name changes. This pull request has now been integrated. Changeset: 65f0829d Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/65f0829d645fd988c6a208622b1f34bf9de08e60 Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod 8288068: Javadoc contains spurious reference to CLinker Reviewed-by: iris - PR: https://git.openjdk.java.net/jdk/pull/9094
RFR: 8288068: Javadoc contains spurious reference to CLinker
This simple patch fixes a bunch of references to "CLinker" and replaces them with "Linker", following the API name changes. - Commit messages: - Initial push Changes: https://git.openjdk.java.net/jdk/pull/9094/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9094=00 Issue: https://bugs.openjdk.org/browse/JDK-8288068 Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/9094.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9094/head:pull/9094 PR: https://git.openjdk.java.net/jdk/pull/9094
Re: RFR: 8287809: Revisit implementation of memory session [v2]
> This is a cleanup of the memory session implementation. The main new concept > is that `MemorySessionImpl` is split into two parts: there is an > implementation of memory session and then there is a state abstraction > (`MemorySessionImpl.State`). This allows to share the state across multiple > session views, in a more clean way. The big consequence of this change is > that the routines on `ScopedMemoryAccess` now have to be defined in terms of > the state abstraction (but the changes are mostly mechanical). > > I have consolidated the implementation quite a bit, by removing all the > duplicated logic for issuing similar-looking exceptions. I have also > addressed an issue with `checkValidState` throwing a _new_ > `WrongThreadException` instead of using a singleton (which is what the logic > for closing down shared session requires, to avoid stack walks that are too > deep). > > `MemorySession.State::checkValidState` is now fully monomorphic; when looking > at benchmarks, this seems to be the best solution in order to make things > fast. Specializing implmentations to remove few plain checks does not buy > enough, and always has the risk of adding profile pollution. Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits: - Merge branch 'master' into cleanup_memory_session_impl_state - Add null check on Buffer::checkState - Add docs Simplify liveness check in Buffer Drop redundant import in DirectBuffer - Simplify checkValidState - Add fastpath for implicit session state - Merge branch 'master' into cleanup_memory_session_impl_keep_list - Fix asNonCloseable to return self Improve direct buffer code with isImplicit predicate - Drop MemorySession interface type from AbstractMemorySessionImpl - Simplify code by removing intermediate getUnsafeBase/getUnsafeOffset methods - Simplify readOnly check - ... and 4 more: https://git.openjdk.java.net/jdk/compare/8d28734e...5b8f7246 - Changes: https://git.openjdk.java.net/jdk/pull/9017/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9017=01 Stats: 1752 lines in 39 files changed: 407 ins; 525 del; 820 mod Patch: https://git.openjdk.java.net/jdk/pull/9017.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9017/head:pull/9017 PR: https://git.openjdk.java.net/jdk/pull/9017
Re: RFR: 8287809: Revisit implementation of memory session
On Fri, 3 Jun 2022 15:47:21 GMT, Maurizio Cimadamore wrote: > This is a cleanup of the memory session implementation. The main new concept > is that `MemorySessionImpl` is split into two parts: there is an > implementation of memory session and then there is a state abstraction > (`MemorySessionImpl.State`). This allows to share the state across multiple > session views, in a more clean way. The big consequence of this change is > that the routines on `ScopedMemoryAccess` now have to be defined in terms of > the state abstraction (but the changes are mostly mechanical). > > I have consolidated the implementation quite a bit, by removing all the > duplicated logic for issuing similar-looking exceptions. I have also > addressed an issue with `checkValidState` throwing a _new_ > `WrongThreadException` instead of using a singleton (which is what the logic > for closing down shared session requires, to avoid stack walks that are too > deep). > > `MemorySession.State::checkValidState` is now fully monomorphic; when looking > at benchmarks, this seems to be the best solution in order to make things > fast. Specializing implmentations to remove few plain checks does not buy > enough, and always has the risk of adding profile pollution. Note: other cleanups are possible - such as flattening `ResourceList` into `MemorySession.State`. While that is possible, that's a tricky step, given that the session state can be registered against a cleaner. One option, which I explored, would be to use `MemorySessionImpl` for reachability - which then has an impact on `ScopedMemoryAccess`, as we would have to add reachability fences on the session, as well as on the session state. In any case, I'd like to proceed by steps, since this cleanup was already getting big enough. - PR: https://git.openjdk.java.net/jdk/pull/9017
RFR: 8287809: Revisit implementation of memory session
This is a cleanup of the memory session implementation. The main new concept is that `MemorySessionImpl` is split into two parts: there is an implementation of memory session and then there is a state abstraction (`MemorySessionImpl.State`). This allows to share the state across multiple session views, in a more clean way. The big consequence of this change is that the routines on `ScopedMemoryAccess` now have to be defined in terms of the state abstraction (but the changes are mostly mechanical). I have consolidated the implementation quite a bit, by removing all the duplicated logic for issuing similar-looking exceptions. I have also addressed an issue with `checkValidState` throwing a _new_ `WrongThreadException` instead of using a singleton (which is what the logic for closing down shared session requires, to avoid stack walks that are too deep). `MemorySession.State::checkValidState` is now fully monomorphic; when looking at benchmarks, this seems to be the best solution in order to make things fast. Specializing implmentations to remove few plain checks does not buy enough, and always has the risk of adding profile pollution. - Commit messages: - Add null check on Buffer::checkState - Add docs - Simplify checkValidState - Add fastpath for implicit session state - Merge branch 'master' into cleanup_memory_session_impl_keep_list - Fix asNonCloseable to return self - Drop MemorySession interface type from AbstractMemorySessionImpl - Simplify code by removing intermediate getUnsafeBase/getUnsafeOffset methods - Simplify readOnly check - Add @ForceInline in hot path from var handle access - ... and 3 more: https://git.openjdk.java.net/jdk/compare/d5b6c7bd...68028fa4 Changes: https://git.openjdk.java.net/jdk/pull/9017/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9017=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287809 Stats: 1752 lines in 39 files changed: 407 ins; 525 del; 820 mod Patch: https://git.openjdk.java.net/jdk/pull/9017.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9017/head:pull/9017 PR: https://git.openjdk.java.net/jdk/pull/9017
Integrated: 8287748: Fix issues in java.lang.foreign package javadoc
On Thu, 2 Jun 2022 20:41:39 GMT, Maurizio Cimadamore wrote: > This patch fixes a couple of issues in the package-level javadoc of the > foreign API. > I've also dropped uses of `var`, which I think are confusing so early in the > game. This pull request has now been integrated. Changeset: ba9ee8cb Author: Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/ba9ee8cb286268f1d6a2820508334aaaf3131e15 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod 8287748: Fix issues in java.lang.foreign package javadoc Reviewed-by: jvernee - PR: https://git.openjdk.java.net/jdk/pull/9005
RFR: 8287748: Fix issues in java.lang.foreign package javadoc
This patch fixes a couple of issues in the package-level javadoc of the foreign API. I've also dropped uses of `var`, which I think are confusing so early in the game. - Commit messages: - Initial push Changes: https://git.openjdk.java.net/jdk/pull/9005/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9005=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287748 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/9005.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9005/head:pull/9005 PR: https://git.openjdk.java.net/jdk/pull/9005
Integrated: 8287430 MemorySessionImpl::addOrCleanupIfFail does not rethrow exceptions
On Fri, 27 May 2022 10:29:14 GMT, Maurizio Cimadamore wrote: > This patch fix a missing rethrow in `MemorySessionImpl::addOrCleanupIfFail`. > As noted in the JBS issue, this bug does not affect correctness, but it > delays error reporting. > > Writing a test for this is nearly impossible, given that (a) a memory > resource created against a closed session would be inaccessible by clients > (because the session is closed!), and (b) because of the narrow window in > which the problem might manifest (for this problem to occur, a session state > change would have to occur between the first state check and when the cleanup > action list is updated). This pull request has now been integrated. Changeset: 0c420e03 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/0c420e03ae24144a8146edb39f546841da33e381 Stats: 9 lines in 1 file changed: 3 ins; 0 del; 6 mod 8287430: MemorySessionImpl::addOrCleanupIfFail does not rethrow exceptions Reviewed-by: jvernee - PR: https://git.openjdk.java.net/jdk/pull/8917
Re: RFR: 8287430 MemorySessionImpl::addOrCleanupIfFail does not rethrow exceptions
On Fri, 27 May 2022 10:29:14 GMT, Maurizio Cimadamore wrote: > This patch fix a missing rethrow in `MemorySessionImpl::addOrCleanupIfFail`. > As noted in the JBS issue, this bug does not affect correctness, but it > delays error reporting. > > Writing a test for this is nearly impossible, given that (a) a memory > resource created against a closed session would be inaccessible by clients > (because the session is closed!), and (b) because of the narrow window in > which the problem might manifest (for this problem to occur, a session state > change would have to occur between the first state check and when the cleanup > action list is updated). src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line 112: > 110: // to the list (and, in case of an add vs. close race, it might > happen that the cleanup action will be > 111: // called immediately after). > 112: resourceList.add(resource); Note that I've removed the try/catch, as resourceList::add cannot throw the ScopedAccessError singleton (that is only issued in the raw/internal `checkValidState`) - PR: https://git.openjdk.java.net/jdk/pull/8917
RFR: 8287430 MemorySessionImpl::addOrCleanupIfFail does not rethrow exceptions
This patch fix a missing rethrow in `MemorySessionImpl::addOrCleanupIfFail`. As noted in the JBS issue, this bug does not affect correctness, but it delays error reporting. Writing a test for this is nearly impossible, given that (a) a memory resource created against a closed session would be inaccessible by clients (because the session is closed!), and (b) because of the narrow window in which the problem might manifest (for this problem to occur, a session state change would have to occur between the first state check and when the cleanup action list is updated). - Commit messages: - Add more code comments in `MemorySession::addInternal` - Initial push Changes: https://git.openjdk.java.net/jdk/pull/8917/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8917=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287430 Stats: 9 lines in 1 file changed: 3 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8917.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8917/head:pull/8917 PR: https://git.openjdk.java.net/jdk/pull/8917
Integrated: 8287244: Add bound check in indexed memory access var handle
On Tue, 24 May 2022 14:40:56 GMT, Maurizio Cimadamore wrote: > Constructing indexed var handles using the `MemoryLayout` API produces > `VarHandle` which do not check the input indices for out-of-bounds conditions. > While this can never result in a VM crash (after all the memory segment will > protect against "true" OOB access), it is still possible for an access > expression to refer to parts of a segment that are logically unrelated. > > This patch adds a "logical" bound check to all indexed var handles generated > using the layout API. > Benchmarks are not affected by the check. Users are still able to create > custom "unchecked" var handles, using the combinator API in `MethodHandles`. This pull request has now been integrated. Changeset: f58c9a65 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/f58c9a659ba181407ecdb2aacb81e6a7f1cbd9ff Stats: 92 lines in 4 files changed: 70 ins; 5 del; 17 mod 8287244: Add bound check in indexed memory access var handle Reviewed-by: psandoz, jvernee - PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Wed, 25 May 2022 10:16:54 GMT, Maurizio Cimadamore wrote: > > Thinking more about it: `IllegalStateException` is fine for a closed > > `MemorySession`. If used by wrong thread the exception was indeed > > confusing. Now that the abiguity is gone, I can change our (Lucene) code > > and just transform every ISE to an already closed in our code. I just > > wanted to bring up that issue here. > > PR is here: [apache/lucene#912](https://github.com/apache/lucene/pull/912) > > I've been thinking something similar. I'd suggest to keep the API as is, and > maybe revise it at a later point if lack of a specific exception for the > "already closed" case proves to be too cumbersome to workaround. Basically, with this patch you only get ISE if you are accessing _in a moment in time_ when you are not supposed to. Similarly, when calling `close`, you get ISE if you are closing a segment that is in a bad state (e.g. already closed, or temporarily locked by some native call). This feels consistent. The confinement exception was the confounding factor, I think, and a lot of checks against the exception message came from there. - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. > Thinking more about it: `IllegalStateException` is fine for a closed > `MemorySession`. If used by wrong thread the exception was indeed confusing. > Now that the abiguity is gone, I can change our (Lucene) code and just > transform every ISE to an already closed in our code. I just wanted to bring > up that issue here. > > PR is here: [apache/lucene#912](https://github.com/apache/lucene/pull/912) I've been thinking something similar. I'd suggest to keep the API as is, and maybe revise it at a later point if lack of a specific exception for the "already closed" case proves to be too cumbersome to workaround. - PR: https://git.openjdk.java.net/jdk/pull/8865
Integrated: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. This pull request has now been integrated. Changeset: e1f140d2 Author: Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/e1f140d270cc666d26b888a0a25ca7b02e1239af Stats: 254 lines in 12 files changed: 150 ins; 1 del; 103 mod 8287206: Use WrongThreadException for confinement errors Reviewed-by: alanb, darcy, mchung - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v3]
On Tue, 24 May 2022 15:15:43 GMT, Alan Bateman wrote: >> This is the implementation of JEP 428: Structured Concurrency (Incubator). >> >> This is a non-final API that provides a gentle on-ramp to structure a task >> as a family of concurrent subtasks, and to coordinate the subtasks as a unit. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Add statement to close about thread termination Looks good! - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8787
Re: RFR: 8287244: Add bound check in indexed memory access var handle [v3]
> Constructing indexed var handles using the `MemoryLayout` API produces > `VarHandle` which do not check the input indices for out-of-bounds conditions. > While this can never result in a VM crash (after all the memory segment will > protect against "true" OOB access), it is still possible for an access > expression to refer to parts of a segment that are logically unrelated. > > This patch adds a "logical" bound check to all indexed var handles generated > using the layout API. > Benchmarks are not affected by the check. Users are still able to create > custom "unchecked" var handles, using the combinator API in `MethodHandles`. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8868/files - new: https://git.openjdk.java.net/jdk/pull/8868/files/a682cc03..66cf582a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8868=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8868=01-02 Stats: 7 lines in 2 files changed: 1 ins; 2 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8868.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8868/head:pull/8868 PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]
On Tue, 24 May 2022 18:02:50 GMT, Maurizio Cimadamore wrote: >> Sorry i misread the text, we are talking about the same thing. I think it >> would be clearer to refer `x_i` being in the range of `0` (inclusive) and >> `b_i` (exclusive), otherwise an is thrown. That way in subsequent doc >> on other methods in matches with `B`, which is exclusive. > > yes, but that seems to affect this statement: > > > `0 <= x_i <= b_i, where 0 <= i <= n`, or IndexOutOfBoundsException is thrown. > > So we can replace with > > > 0 <= x_i < b_i, where 1 <= i <= n`, or IndexOutOfBoundsException is thrown. > Sorry i misread the text, we are talking about the same thing. I think it > would be clearer to refer `x_i` being in the range of `0` (inclusive) and > `b_i` (exclusive), otherwise an is thrown. That way in subsequent doc on > other methods in matches with `B`, which is exclusive. I apologize too - I misread your original question and thought it was about the use of ceilDiv :-) - anyway, at least that prodded me to come up with an example that explains why the logic is the way it is :-) - PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]
On Tue, 24 May 2022 18:00:46 GMT, Paul Sandoz wrote: >> The terms `x_1, x_2, ... x__n` are defined, but `x_0` is not. >> >> I think you can refer to the first index out of bounds as the exclusive >> upper bound of the range? > > Sorry i misread the text, we are talking about the same thing. I think it > would be clearer to refer `x_i` being in the range of `0` (inclusive) and > `b_i` (exclusive), otherwise an is thrown. That way in subsequent doc on > other methods in matches with `B`, which is exclusive. yes, but that seems to affect this statement: `0 <= x_i <= b_i, where 0 <= i <= n`, or IndexOutOfBoundsException is thrown. So we can replace with 0 <= x_i < b_i, where 1 <= i <= n`, or IndexOutOfBoundsException is thrown. - PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]
On Tue, 24 May 2022 17:43:52 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 374:
>>
>>> 372: *
>>> 373: * Additionally, the provided dynamic values must conform to some
>>> bound which is derived from the layout path, that is,
>>> 374: * {@code 0 <= x_i <= b_i}, where {@code 0 <= i <= n}, or {@link
>>> IndexOutOfBoundsException} is thrown.
>>
>> Suggestion:
>>
>> * {@code 0 <= x_i < b_i}, where {@code 1 <= i <= n}, or {@link
>> IndexOutOfBoundsException} is thrown.
>>
>> We refer later to `B` being an exclusive upper bound (computed using
>> `ceilDiv`).
>
> Indices start at zero. The ceilDiv operation is needed so that the operation
> returns the first index outisde the range (it's a bit subtle, sorry, but I
> don't know how else to express).
Here's a concrete example:
Consider a sequence layout with 6 elements. Then:
element count = 6
valid indices 0, 1, 2, 3, 4, 5
Now consider a var handle that is obtained by calling the path element method,
passing the following parameters
start = 1
step = 2
This sets up the following mapping between logical an physical indices:
0 -> 1
1 -> 3
2 -> 5
Where on the LHS we have the logical index (the one passed to the VH) and on
the RHS we have the actual index it is translated to.
Note that the index map has three elements. So the upper bound (exclusive) of
the index map is 3 - that is, we can pass indices 0, 1, 2.
According to the formula shown in the javadoc:
B = ceilDiv((elementCount - start) / step);
so:
B = ceilDiv((6 - 1) / 2)
= ceilDiv(5 / 2)
Note how, w/o ceilDiv we'd get 2 (the last valid index), and not 3 (the first
invalid index).
-
PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]
On Tue, 24 May 2022 16:23:52 GMT, Paul Sandoz wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Tweak javadoc for ValueLayout::arrayElementVarHandle
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 374:
>
>> 372: *
>> 373: * Additionally, the provided dynamic values must conform to some
>> bound which is derived from the layout path, that is,
>> 374: * {@code 0 <= x_i <= b_i}, where {@code 0 <= i <= n}, or {@link
>> IndexOutOfBoundsException} is thrown.
>
> Suggestion:
>
> * {@code 0 <= x_i < b_i}, where {@code 1 <= i <= n}, or {@link
> IndexOutOfBoundsException} is thrown.
>
> We refer later to `B` being an exclusive upper bound (computed using
> `ceilDiv`).
Indices start at zero. The ceilDiv operation is needed so that the operation
returns the first index outisde the range (it's a bit subtle, sorry, but I
don't know how else to express).
-
PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]
> Constructing indexed var handles using the `MemoryLayout` API produces > `VarHandle` which do not check the input indices for out-of-bounds conditions. > While this can never result in a VM crash (after all the memory segment will > protect against "true" OOB access), it is still possible for an access > expression to refer to parts of a segment that are logically unrelated. > > This patch adds a "logical" bound check to all indexed var handles generated > using the layout API. > Benchmarks are not affected by the check. Users are still able to create > custom "unchecked" var handles, using the combinator API in `MethodHandles`. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak javadoc for ValueLayout::arrayElementVarHandle - Changes: - all: https://git.openjdk.java.net/jdk/pull/8868/files - new: https://git.openjdk.java.net/jdk/pull/8868/files/453392ae..a682cc03 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8868=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8868=00-01 Stats: 17 lines in 1 file changed: 16 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8868.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8868/head:pull/8868 PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle
On Tue, 24 May 2022 14:40:56 GMT, Maurizio Cimadamore
wrote:
> Constructing indexed var handles using the `MemoryLayout` API produces
> `VarHandle` which do not check the input indices for out-of-bounds conditions.
> While this can never result in a VM crash (after all the memory segment will
> protect against "true" OOB access), it is still possible for an access
> expression to refer to parts of a segment that are logically unrelated.
>
> This patch adds a "logical" bound check to all indexed var handles generated
> using the layout API.
> Benchmarks are not affected by the check. Users are still able to create
> custom "unchecked" var handles, using the combinator API in `MethodHandles`.
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 537:
> 535: *
> 536: *
> 537: *if {@code F > 0}, then {@code B = ceilDiv(C - S,
> F)}
These formulas come from the formula for computing the accessed index A:
`A = S + I * F`
And then deriving the value for I, by equating `A = C` (for F > 0) and `A = -1`
(for F < 0) - that is equating the accessed index to the "first" out of bound
index. `ceilDiv` ensures there is "some room" between the max/min index and the
selected one.
src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java line 109:
> 107: SequenceLayout seq = (SequenceLayout)layout;
> 108: checkSequenceBounds(seq, index);
> 109: long elemSize = seq.elementLayout().bitSize();
I've simplified the code here, as it still had traces of attempts to avoid the
call to `bitSize` (this method used to be partial).
-
PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle
On Tue, 24 May 2022 14:51:10 GMT, Maurizio Cimadamore
wrote:
>> Constructing indexed var handles using the `MemoryLayout` API produces
>> `VarHandle` which do not check the input indices for out-of-bounds
>> conditions.
>> While this can never result in a VM crash (after all the memory segment will
>> protect against "true" OOB access), it is still possible for an access
>> expression to refer to parts of a segment that are logically unrelated.
>>
>> This patch adds a "logical" bound check to all indexed var handles generated
>> using the layout API.
>> Benchmarks are not affected by the check. Users are still able to create
>> custom "unchecked" var handles, using the combinator API in `MethodHandles`.
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 537:
>
>> 535: *
>> 536: *
>> 537: *if {@code F > 0}, then {@code B = ceilDiv(C - S,
>> F)}
>
> These formulas come from the formula for computing the accessed index A:
>
> `A = S + I * F`
>
> And then deriving the value for I, by equating `A = C` (for F > 0) and `A =
> -1` (for F < 0) - that is equating the accessed index to the "first" out of
> bound index. `ceilDiv` ensures there is "some room" between the max/min index
> and the selected one.
Note also that these complex bound calculation are performed statically, when
we build the layout path. When we're done, we just have an upper bound, which
we can check against using `Objects.checkIndex`.
-
PR: https://git.openjdk.java.net/jdk/pull/8868
RFR: 8287244: Add bound check in indexed memory access var handle
Constructing indexed var handles using the `MemoryLayout` API produces `VarHandle` which do not check the input indices for out-of-bounds conditions. While this can never result in a VM crash (after all the memory segment will protect against "true" OOB access), it is still possible for an access expression to refer to parts of a segment that are logically unrelated. This patch adds a "logical" bound check to all indexed var handles generated using the layout API. Benchmarks are not affected by the check. Users are still able to create custom "unchecked" var handles, using the combinator API in `MethodHandles`. - Commit messages: - Tweak javadoc - Merge branch 'master' into vh_bound_checks - Initial push Changes: https://git.openjdk.java.net/jdk/pull/8868/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8868=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287244 Stats: 73 lines in 3 files changed: 54 ins; 4 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/8868.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8868/head:pull/8868 PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]
On Tue, 24 May 2022 10:47:15 GMT, Maurizio Cimadamore
wrote:
>> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>> line 1110:
>>
>>> 1108: * invoked {@link #join() join} (or {@link
>>> #joinUntil(Instant) joinUntil}).
>>> 1109: * The behavior of this method is unspecified when invoking
>>> this method before
>>> 1110: * the {@code join} is invoked.
>>
>> Suggestion:
>>
>> * {@link #join} is invoked.
>
> More generally, I see that you used `{@code ... }` in a lot of places where
> `{@link ... }` could also be used. In some of those places (like this one)
> where there is a clear cross-reference, I think `@link` could be preferrable.
> The only case where `@code` is fine is when referring to the name of the
> class itself (e.g. `{@code StructuredTaskScope}`). But of course this is
> subjective.
Also, note the typo `the join is invoked`. Either `the` is dropped, or `method`
is added. I've seen more than one occurrence of this.
-
PR: https://git.openjdk.java.net/jdk/pull/8787
Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]
On Tue, 24 May 2022 10:41:59 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>>
>> This is a non-final API that provides a gentle on-ramp to structure a task
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> Alan Bateman has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains six additional
> commits since the last revision:
>
> - Use {@code ...}, replace task->subtask, fix typos, add jls ref
> - Merge
> - @ignore StructuredThreadDumpTest until test infra in place
> - Refresh
> - Merge
> - Initial commit
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
line 214:
> 212: * Tree structure
> 213: *
> 214: * StructuredTaskScopes form a tree where parent-child relations are
> established
Should we mention what happens in the owner thread completes its execution and
the scope's `close` method has not been called? I think that, as discussed
offline, the fact that the thread will attempt to close any nested scopes when
terminating is an important aspect of this API.
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
line 1110:
> 1108: * invoked {@link #join() join} (or {@link #joinUntil(Instant)
> joinUntil}).
> 1109: * The behavior of this method is unspecified when invoking
> this method before
> 1110: * the {@code join} is invoked.
Suggestion:
* {@link #join} is invoked.
test/jdk/jdk/incubator/concurrent/StructuredTaskScope/StructuredThreadDumpTest.java
line 200:
> 198: }
> 199:
> 200: }
Watch out for the newline here
-
PR: https://git.openjdk.java.net/jdk/pull/8787
Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]
On Tue, 24 May 2022 10:44:39 GMT, Maurizio Cimadamore
wrote:
>> Alan Bateman has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains six additional
>> commits since the last revision:
>>
>> - Use {@code ...}, replace task->subtask, fix typos, add jls ref
>> - Merge
>> - @ignore StructuredThreadDumpTest until test infra in place
>> - Refresh
>> - Merge
>> - Initial commit
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
> line 1110:
>
>> 1108: * invoked {@link #join() join} (or {@link #joinUntil(Instant)
>> joinUntil}).
>> 1109: * The behavior of this method is unspecified when invoking
>> this method before
>> 1110: * the {@code join} is invoked.
>
> Suggestion:
>
> * {@link #join} is invoked.
More generally, I see that you used `{@code ... }` in a lot of places where
`{@link ... }` could also be used. In some of those places (like this one)
where there is a clear cross-reference, I think `@link` could be preferrable.
The only case where `@code` is fine is when referring to the name of the class
itself (e.g. `{@code StructuredTaskScope}`). But of course this is subjective.
-
PR: https://git.openjdk.java.net/jdk/pull/8787
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. javadoc: http://cr.openjdk.java.net/~mcimadamore/8287206/v1/javadoc/java.base/module-summary.html specdiff: http://cr.openjdk.java.net/~mcimadamore/8287206/v1/specdiff_out/overview-summary.html - PR: https://git.openjdk.java.net/jdk/pull/8865
RFR: 8287206: Use WrongThreadException for confinement errors
This patch tweaks the foreign API to use the newly added `WrongThreadException` instead of `IllegalStateException` to report confinement errors. - Commit messages: - Merge branch 'master' into wrong_thread_ex - Initial push Changes: https://git.openjdk.java.net/jdk/pull/8865/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8865=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287206 Stats: 254 lines in 12 files changed: 150 ins; 1 del; 103 mod Patch: https://git.openjdk.java.net/jdk/pull/8865.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8865/head:pull/8865 PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8287137: Problemlist failing x86_32 tests after Loom integration [v3]
On Mon, 23 May 2022 18:25:23 GMT, Aleksey Shipilev wrote: >> [JDK-8284161](https://bugs.openjdk.java.net/browse/JDK-8284161) broke a lot >> of x86_32 code. The x86_32 porting is done under >> [JDK-8286642](https://bugs.openjdk.java.net/browse/JDK-8286642). Meanwhile, >> we can problemlist the failing tests to get cleaner runs for other patches. >> This should also make GHA runs cleaner. >> >> Additional testing: >> - [x] Linux x86_32 fastdebug `tier1` (some unrelated failures in hotspot) >> - [x] Linux x86_32 fastdebug `tier2` (some unrelated failures in hotspot) >> - [x] Linux x86_32 fastdebug `tier3` (clean now) >> - [x] GHA run > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Another release test Marked as reviewed by mcimadamore (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8843
Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)
On Thu, 19 May 2022 13:05:54 GMT, Alan Bateman wrote:
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>
> This is a non-final API that provides a gentle on-ramp to structure a task as
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.
Marked as reviewed by mcimadamore (Reviewer).
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
line 88:
> 86: * {@code join} method after forking.
> 87: *
> 88: * StructuredTaskScope defines the {@link #shutdown() shutdown}
> method to shut down a
This sentence, because of the place where it appears, is a bit confusing. So
far we only know about the fact that a scope has an owner thread. So it seems
odd that shutdown could be called _while_ the owner thread is waiting on a
`join`. Of course, then you read what's next, and you discover that: (a)
shutdown might be called by a custom scope subclass and that (b) shutdown is
confined to the threads contained in this task scope - but this definition is
only given much later.
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
line 353:
> 351: *
> 352: * The {@code handleComplete} method should be thread safe. It
> may be
> 353: * invoked by several threads at around the same.
Something is missing? E.g. "at around the same TIME" ? (I'd suggest just using
"concurrently")
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
line 376:
> 374: *
> 375: * If this task scope is {@linkplain #shutdown() shutdown} (or
> in the process
> 376: * of shutting down) then {@code fork} returns a Future representing
> a {@link
Future in plaintext?
-
PR: https://git.openjdk.java.net/jdk/pull/8787
Re: RFR: 8287137: Problemlist failing x86_32 tests after Loom integration
On Mon, 23 May 2022 15:36:01 GMT, Aleksey Shipilev wrote: >> Okay, so the issue is SpliteratorTest uses preview APIs and having the >> configuration in TEST.properties means that all the streams tests are run >> with this option. > > Yes, `TEST.properties` is way too broad. I'd keep the current patch as is, > instead of trying to fix that one. I suspect there is some funky > TestNG/othervm interaction going... These specific stream tests are odd, in the sense that they live in a test root that is TestNG and they are only "imported" into jtreg. The toplevel TEST.properties file gets only once chance to set the correct parameters, for all tests. I've tried adding more local flags, nearer to the test, w/o success. I'm obviously happy to do it another way. I have tried adding a jtreg header in that test, but that doesn't fix the problem - because _all_ the tests are passed to testng at once, which compiles them and runs them. So either the flag is present when that happens, or it is ignored. - PR: https://git.openjdk.java.net/jdk/pull/8843
Re: RFR: 8287158: Explicitly reject unsupported call shapes on macos-aarch64 in mainline
On Mon, 23 May 2022 12:27:40 GMT, Jorn Vernee wrote: > Bring in changes from the panama-foreign repo > > These changes pertain to explicitly rejecting unsupported call shapes on > macos-aarch64. > > Original PRs: > 1. https://github.com/openjdk/panama-foreign/pull/676 > 2. https://github.com/openjdk/panama-foreign/pull/677 > > Testing: `jdk_foreign` on linux-aarch64-debug, macosx-aarch64-debug, > linux-x64-debug, macosx-x64-debug, and windows-x64-debug Looks good! - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8842
Integrated: 8286715: Generalize MemorySegment::ofBuffer
On Fri, 13 May 2022 12:33:10 GMT, Maurizio Cimadamore wrote: > This patch makes MemorySegment::ofBuffer more general, by allowing clients to > pass *any* `Buffer` instance, not just `ByteBuffer`. > This allows us to match expressiveness of JNI API, where JNI clients can > obtain the address of any direct buffer instance, using the > `GetDirectBufferAddress` function. > > We thought about also providing a more general way to view a segment as a > buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct > buffers can only created form `ByteBuffer`. > So, to create a direct `IntBuffer`, clients have to first create a direct > `ByteBuffer` then to view that buffer as an `IntBuffer`. > > In other words, `IntBuffer` and friends are not first-class citizens in the > `Buffer` API. As such it would not be possible to map many memory segments > into an `IntBuffer`; in fact, the only segment we could safely map into an > `IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it > doesn't seem worth adding a lot of API surface (in terms of additional > overloads) for such a corner case. This pull request has now been integrated. Changeset: 89a1d055 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/89a1d055d93ad57bcec7c1accb3f53b4c30f594d Stats: 95 lines in 8 files changed: 55 ins; 1 del; 39 mod 8286715: Generalize MemorySegment::ofBuffer Reviewed-by: jvernee - PR: https://git.openjdk.java.net/jdk/pull/8701
Re: RFR: 8286825: Linker naming cleanup [v2]
On Fri, 20 May 2022 10:57:44 GMT, Jorn Vernee wrote: >> This patch is a batch naming cleanup for the foreign linker implementation. >> >> The naming changes are as follows: >> >> - ProgrammableInvoker -> DowncallLinker >> - ProgrammableUpcallHandler -> UpcallLinker >> - 'native invoker' -> 'downcall stub' >> - 'optimzed entry blob' -> 'upcall stub' >> - OptimizedEntryBlob -> UpcallStub >> - optimized_entry_frame -> upcall_stub_frame >> >> Then renaming of some hotspot files: >> >> - universalNativeInvoker* -> downcallLinker* >> - universalUpcallHandler* -> upcallLinker* >> - foreign_globals* -> foreignGlobals* (to match existing convention) >> >> Method, field, and other variable names are also adjusted accordingly. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Comments + cleanup Marked as reviewed by mcimadamore (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8777
Re: RFR: 8286825: Linker naming cleanup
On Wed, 18 May 2022 16:54:06 GMT, Jorn Vernee wrote: > This patch is a batch naming cleanup for the foreign linker implementation. > > The naming changes are as follows: > > - ProgrammableInvoker -> DowncallLinker > - ProgrammableUpcallHandler -> UpcallLinker > - 'native invoker' -> 'downcall stub' > - 'optimzed entry blob' -> 'upcall stub' > - OptimizedEntryBlob -> UpcallStub > - optimized_entry_frame -> upcall_stub_frame > > Then renaming of some hotspot files: > > - universalNativeInvoker* -> downcallLinker* > - universalUpcallHandler* -> upcallLinker* > - foreign_globals* -> foreignGlobals* (to match existing convention) > > Method, field, and other variable names are also adjusted accordingly. Looks good - I spotted a potential issue in the risc-v port src/hotspot/cpu/riscv/upcallLinker_riscv.cpp line 30: > 28: #include "utilities/debug.hpp" > 29: > 30: address UpcallLinker::generate_optimized_upcall_stub(jobject receiver, > Method* entry, Hasn't "generated_optimized_upcall_stub" changed too? src/hotspot/cpu/riscv/upcallLinker_riscv.cpp line 38: > 36: ShouldNotCallThis(); > 37: return nullptr; > 38: } Missing newline (this was here before) - PR: https://git.openjdk.java.net/jdk/pull/8777
Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v8]
On Fri, 20 May 2022 09:51:24 GMT, Jatin Bhateja wrote: >> Hi All, >> >> Patch adds the planned support for new vector operations and APIs targeted >> for [JEP 426: Vector API (Fourth >> Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173) >> >> Following is the brief summary of changes:- >> >> 1) Extends the scope of existing lanewise API for following new vector >> operations. >>- VectorOperations.BIT_COUNT: counts the number of one-bits >>- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero >> bits >>- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing >> zero bits >>- VectorOperations.REVERSE: reversing the order of bits >>- VectorOperations.REVERSE_BYTES: reversing the order of bytes >>- compress and expand bits: Semantics are based on Hacker's Delight >> section 7-4 Compress, or Generalized Extract. >> >> 2) Adds following new APIs to perform cross lane vector compress and >> expansion operations under the influence of a mask. >>- Vector.compress >>- Vector.expand >>- VectorMask.compress >> >> 3) Adds predicated and non-predicated versions of following new APIs to load >> and store the contents of vector from foreign MemorySegments. >> - Vector.fromMemorySegment >> - Vector.intoMemorySegment >> >> 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support >> for each newly added operation. >> >> >> Patch has been regressed over AARCH64 and X86 targets different AVX levels. >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > 8284960: Integrating incremental patches. Javac changes look good - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8425
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 15:05:26 GMT, Jorn Vernee wrote:
>> It's not quite that simple since a binding recipe for a single parameter can
>> have multiple VMStores for instance if a struct is decomposed into multiple
>> values.
>>
>> It can be done by pulling the binding loops up to the `specialize` method,
>> and have if statements for VMStore and VMLoad, like this:
>>
>> for (Binding binding : callingSequence.argumentBindings(i)) {
>> if (binding instanceof Binding.VMStore vms &&
>> callingSequence.forDowncall()) {
>> emitSetOutput(vms.type());
>> } else if (binding instanceof Binding.VMLoad &&
>> callingSequence.forUpcall()) {
>> emitGetInput();
>> } else {
>> doBinding(binding);
>> }
>> }
>>
>> And for returns:
>>
>> for (Binding binding : callingSequence.returnBindings()) {
>> if (binding instanceof Binding.VMLoad vml &&
>> callingSequence.forDowncall()) {
>> if (!callingSequence.needsReturnBuffer()) {
>> emitRestoreReturnValue(vml.type());
>> } else {
>> emitReturnBufferLoad(vml);
>> }
>> } else if (binding instanceof Binding.VMStore vms &&
>> callingSequence.forUpcall()) {
>> if (!callingSequence.needsReturnBuffer()) {
>> emitSaveReturnValue(vms.type());
>> } else {
>> emitReturnBufferStore(vms);
>> }
>> } else {
>> doBinding(binding);
>> }
>> }
>>
>> But, maybe that's better?
>
> I think someone might look at this and think "why aren't these bindings
> handled by `doBinding`"?
Let's leave it as is for the time being. I guess what I'm trying to get at is
to try and make the code more explicit - but that is hard, given the
constraints.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v5]
On Wed, 18 May 2022 11:27:24 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR brings over commits from the panama-foreign repo. These commits >> mostly pertain to the switch to ASM and away from MethodHandle combinators >> for the binding recipe specialization. But, there is one more commit which >> adds freeing of downcall stubs, since those changes were mostly Java as well. >> >> Thanks, >> Jorn >> >> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on >> Windows and Linux. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 118 commits: > > - Merge branch 'JEP-19-ASM' of https://github.com/JornVernee/jdk into > JEP-19-ASM > - Merge branch 'pr/7959' into JEP-19-ASM > - Merge branch 'master' into JEP-19-VM-IMPL2 > - ifdef NOT_PRODUCT -> ifndef PRODUCT > - Missing ASSERT -> NOT_PRODUCT > - Cleanup UL usage > - Fix failure with SPEC disabled (accidentally dropped change) > - indentation > - fix space > - Merge branch 'master' into JEP-19-ASM > - ... and 108 more: > https://git.openjdk.java.net/jdk/compare/81e4bdbe...ce5c677a Looks good! - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 11:23:07 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
>> line 336:
>>
>>> 334:
>>> 335: if (callingSequence.forUpcall()) {
>>> 336: // for upcalls, recipes have a result, which we handle
>>> here
>>
>> I find this part a bit confusing. The comment speaks about recipes input and
>> outputs, hinting at the fact that downcall bindings have inputs, while
>> upcall bindings have outputs.
>> In reality, if we look at the bindings themselves, they look pretty
>> symmetric:
>>
>> * UNBOX_ADDRESS = pops an Addressable and push a long on the stack
>> * BOX_ADDRESS = pops a long and push a MemoryAddress on the stack
>>
>> That is, in both cases it appears we have an input and an output (and the
>> binding is just the function which maps one into another).
>>
>> I think the input/output asymmetry comes in when we consider the full
>> recipes. In downcalls, for a given argument we will have the following
>> sequence:
>>
>> Binding0, Binding1, ... BindingN-1, VMStore
>>
>> While for upcalls we will have:
>>
>> VMLoad, Binding1, Binding2, ... BindingN
>>
>> So (ignoring return buffer for simplicity), for downcalls, it is obvious
>> that before we can execute Binding0, we need some kind of "input value"
>> (e.g. the value of some local). For upcalls, this is not needed, as the
>> VMLoad binding will basically do that for free.
>> When we finished executing the bindings, again, for downcalls there's
>> nothing to do, as the chain always ends with a VMStore (which stores binding
>> result into some local), while for upcalls we do need to set the output
>> value explicitly.
>>
>> In other words, it seems to me that having VMLoad and VMStore in the chains
>> of bindings to be interpreted/specialized does more harm than good here.
>> These low level bindings make sense for the VM code, as the VM needs to know
>> how to load a value from a register, or how to store a value into a
>> register. But in terms of the specializing Java code, these bindings are
>> immaterial. At the same time, the fact that we have these bindings, and that
>> they are virtualized, makes the code hard to follow, as some preparation
>> happens in `BindingSpecializer::specialize`, while some other preparation
>> happens inside `BindingSpecializer::doBindings`, as part of the virtualized
>> impl of VMLoad/VMStore. And, this virtualized implementation ends up doing
>> similar steps as what the preparation code before/after the binding
>> execution does (e.g. for downcalls/VMStore we call setOutput, while for
>> upcalls/VMLoad we call getInput).
>>
>> All I'm saying here is that, for bindings to work, we _always_ need to call
>> both getInput and setOutput - but it is easy to overlook this when looking
>> at the code, because the getInput/setOutput calls are spread across two
>> different places in the specializing code, perhaps making things more
>> asymmetric than they need to be. (I realize I'm simplifying here, and that
>> some details of the return buffer are not 100% quite as symmetric).
>
> I'm not sure if there is anything actionable here?
>
> I've thought in the past that it might be nice to have
> `GetArgument`/`SetArgument` and `GetReturnValue`/`SetReturnValue` binding
> operators as well, to make the inputs/outputs more explicit in the recipe.
> But, it doesn't seem like that would make things _much_ better...
I wasn't suggesting to add more bindings. I was more suggesting to filter out
the load/store from the set of bindings (since these are virtualized anyways)
that are passed to `doBindings`. Then, before executing a set of bindings, (if
we are in downcall mode) we load the corresponding input local var. After
executing bindings (if we are in upcall mode) we store result in corresponding
var.
E.g. make the logic that load locals and store locals explicit in the
`specialize` method, rather than have parts of it execute "in disguise" as
"binding interpretation".
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v2]
On Fri, 13 May 2022 13:23:41 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This PR brings over commits from the panama-foreign repo. These commits
>> mostly pertain to the switch to ASM and away from MethodHandle combinators
>> for the binding recipe specialization. But, there is one more commit which
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>>
>> Thanks,
>> Jorn
>>
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use unaligned layout constants when filling in reconstituted structs (was
> accidentally dropped change)
src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequence.java line
109:
> 107: * @return the caller method type.
> 108: */
> 109: public MethodType callerMethodType() {
Would it make sense to either rename these, or to point to the fact that these
are equivalent to Linker::downcallType/upcallType ?
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 05:54:39 GMT, Rémi Forax wrote: >> Jorn Vernee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> BootstrapMethodError -> ExceptionInInitializerError > > src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java > line 98: > >> 96: private static final String CLASS_DATA_DESC = >> methodType(Object.class, MethodHandles.Lookup.class, String.class, >> Class.class).descriptorString(); >> 97: private static final String RELEASE0_DESC = >> methodType(void.class).descriptorString(); >> 98: private static final String ACQUIRE0_DESC = >> methodType(void.class).descriptorString(); > > calling methodType() is quite slow because of the cache of MethodType so > maybe those initialization should be done explicitly in a static block to > avoid to recompute methodType(long).descriptorString() several times What about using MethodTypeDesc/ClassDesc as building block? - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This PR brings over commits from the panama-foreign repo. These commits
>> mostly pertain to the switch to ASM and away from MethodHandle combinators
>> for the binding recipe specialization. But, there is one more commit which
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>>
>> Thanks,
>> Jorn
>>
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> BootstrapMethodError -> ExceptionInInitializerError
src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 257:
> 255: * the context's allocator is accessed.
> 256: */
> 257: public static Context ofSession() {
Thanks for fixing this
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 131:
> 129: private int[] scopeSlots;
> 130: private int curScopeLocalIdx = -1;
> 131: private int RETURN_ALLOCATOR_IDX = -1;
These are not constants, so it is odd to see the name capitalized
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 291:
> 289: emitGetStatic(Binding.Context.class, "DUMMY",
> BINDING_CONTEXT_DESC);
> 290: } else {
> 291: emitInvokeStatic(Binding.Context.class, "ofSession",
> OF_SESSION_DESC);
Maybe this is micro-optimization, but I realized that in reality we probably
don't need any scope/session if there are no "ToSegment" bindings.
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 336:
> 334:
> 335: if (callingSequence.forUpcall()) {
> 336: // for upcalls, recipes have a result, which we handle
> here
I find this part a bit confusing. The comment speaks about recipes input and
outputs, hinting at the fact that downcall bindings have inputs, while upcall
bindings have outputs.
In reality, if we look at the bindings themselves, they look pretty symmetric:
* UNBOX_ADDRESS = pops an Addressable and push a long on the stack
* BOX_ADDRESS = pops a long and push a MemoryAddress on the stack
That is, in both cases it appears we have an input and an output (and the
binding is just the function which maps one into another).
I think the input/output asymmetry comes in when we consider the full recipes.
In downcalls, for a given argument we will have the following sequence:
Binding0, Binding1, ... BindingN-1, VMStore
While for upcalls we will have:
VMLoad, Binding1, Binding2, ... BindingN
So (ignoring return buffer for simplicity), for downcalls, it is obvious that
before we can execute Binding0, we need some kind of "input value" (e.g. the
value of some local). For upcalls, this is not needed, as the VMLoad binding
will basically do that for free.
When we finished executing the bindings, again, for downcalls there's nothing
to do, as the chain always ends with a VMStore (which stores binding result
into some local), while for upcalls we do need to set the output value
explicitly.
In other words, it seems to me that having VMLoad and VMStore in the chains of
bindings to be interpreted/specialized does more harm than good here. These low
level bindings make sense for the VM code, as the VM needs to know how to load
a value from a register, or how to store a value into a register. But in terms
of the specializing Java code, these bindings are immaterial. At the same time,
the fact that we have these bindings, and that they are virtualized, makes the
code hard to follow, as some preparation happens in
`BindingSpecializer::specialize`, while some other preparation happens inside
`BindingSpecializer::doBindings`, as part of the virtualized impl of
VMLoad/VMStore. And, this virtualized implementation ends up doing similar
steps as what the preparation code before/after the binding execution does
(e.g. for downcalls/VMStore we call setOutput, while for upcalls/VMLoad we call
getInput).
All I'm saying here is that, for bindings to work, we _always_ need to call
both getInput and setOutput - but it is easy to overlook this when looking at
the code, because the getInput/setOutput calls are spread across two different
places in the specializing code, perhaps making things more asymmetric than
they need to be. (I realize I'm simplifying here, and that some details of the
return buffer are not 100% quite as symmetric).
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8283689: Update the foreign linker VM implementation [v21]
On Mon, 16 May 2022 16:15:49 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This PR updates the VM implementation of the foreign linker, by bringing
>> over commits from the panama-foreign repo.
>>
>> This is split off from the main JEP integration for 19, since we have
>> limited resources to handle this. As such, this PR might fall over to 20,
>> but it would be nice if we could get it into 19.
>>
>> I've written up an overview of the Linker architecture here:
>> http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful
>> to read that first.
>>
>> This patch moves from the "legacy" implementation, to what is currently
>> implemented in the panama-foreign repo, except for replacing the use of
>> method handle combinators with ASM. That will come in a later path. To
>> recap. This PR contains the following changes:
>>
>> 1. VM stubs for downcalls are now generated up front, instead of lazily by
>> C2 [1].
>> 2. the VM support for upcalls/downcalls now support all possible call
>> shapes. And VM stubs and Java code implementing the buffered invocation
>> strategy has been removed [2], [3], [4], [5].
>> 3. The existing C2 intrinsification support for the `linkToNative` method
>> handle linker was no longer needed and has been removed [6] (support might
>> be re-added in another form later).
>> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now
>> implements RuntimeBlob directly. Binding to java classes has been rewritten
>> to use javaClasses.h/cpp (this wasn't previously possible due to these java
>> classes being in an incubator module) [7], [8], [9].
>>
>> While the patch mostly consists of VM changes, there are also some Java
>> changes to support (2).
>>
>> The original commit structure has been mostly retained, so it might be
>> useful to look at a specific commit, or the corresponding patch in the
>> [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr)
>> repo as well. I've also left some inline comments to explain some of the
>> changes, which will hopefully make reviewing easier.
>>
>> Testing: Tier1-4
>>
>> Thanks,
>> Jorn
>>
>> [1]:
>> https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358
>> [2]:
>> https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49
>> [3]:
>> https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3
>> [4]:
>> https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3
>> [5]:
>> https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120
>> [6]:
>> https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a
>> [7]:
>> https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062
>> [8]:
>> https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f
>> [9]:
>> https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Missing ASSERT -> NOT_PRODUCT
src/java.base/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java
line 66:
> 64: private static final boolean USE_SPEC = Boolean.parseBoolean(
> 65:
> GetPropertyAction.privilegedGetProperty("jdk.internal.foreign.ProgrammableInvoker.USE_SPEC",
> "true"));
> 66: private static final boolean USE_INTRINSICS = Boolean.parseBoolean(
Do we need to update TestMatrix given that we're removing one dimension in the
invokers?
-
PR: https://git.openjdk.java.net/jdk/pull/7959
Integrated: 8286756: Cleanup foreign API benchmarks
On Fri, 13 May 2022 15:48:29 GMT, Maurizio Cimadamore wrote: > This simple patch regularizes some of the foreign API benchmarks. Some of the > changes are: > > * use of capital names for var handle and layout constants > * move shared layout and var handle constants in a new superclass, > `JavaLayouts` > * regularize aligned vs. unaligned benchmarks, especially in > `LoopOverNonConstantHeap`. Since the API now aligns layouts by default, where > the benchmark name doesn't say anything we assume aligned layout; for > benchmarks that test specifically the use of unaligned layouts, we use the > `_unaligned` prefix. This pull request has now been integrated. Changeset: 40f4dabc Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/40f4dabce8f6f13cf1c78354a2a1f3d8d7887e19 Stats: 204 lines in 13 files changed: 54 ins; 69 del; 81 mod 8286756: Cleanup foreign API benchmarks Reviewed-by: jvernee - PR: https://git.openjdk.java.net/jdk/pull/8705
Re: RFR: 8286715: Generalize MemorySegment::ofBuffer [v2]
> This patch makes MemorySegment::ofBuffer more general, by allowing clients to > pass *any* `Buffer` instance, not just `ByteBuffer`. > This allows us to match expressiveness of JNI API, where JNI clients can > obtain the address of any direct buffer instance, using the > `GetDirectBufferAddress` function. > > We thought about also providing a more general way to view a segment as a > buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct > buffers can only created form `ByteBuffer`. > So, to create a direct `IntBuffer`, clients have to first create a direct > `ByteBuffer` then to view that buffer as an `IntBuffer`. > > In other words, `IntBuffer` and friends are not first-class citizens in the > `Buffer` API. As such it would not be possible to map many memory segments > into an `IntBuffer`; in fact, the only segment we could safely map into an > `IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it > doesn't seem worth adding a lot of API surface (in terms of additional > overloads) for such a corner case. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java Co-authored-by: Jorn Vernee - Changes: - all: https://git.openjdk.java.net/jdk/pull/8701/files - new: https://git.openjdk.java.net/jdk/pull/8701/files/02494e2f..b6629787 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8701=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8701=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8701.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8701/head:pull/8701 PR: https://git.openjdk.java.net/jdk/pull/8701
RFR: 8286756: Cleanup foreign API benchmarks
This simple patch regularizes some of the foreign API benchmarks. Some of the changes are: * use of capital names for var handle and layout constants * move shared layout and var handle constants in a new superclass, `JavaLayouts` * regularize aligned vs. unaligned benchmarks, especially in `LoopOverNonConstantHeap`. Since the API now aligns layouts by default, where the benchmark name doesn't say anything we assume aligned layout; for benchmarks that test specifically the use of unaligned layouts, we use the `_unaligned` prefix. - Commit messages: - Fix whitespaces - Initial push Changes: https://git.openjdk.java.net/jdk/pull/8705/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8705=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286756 Stats: 204 lines in 13 files changed: 54 ins; 69 del; 81 mod Patch: https://git.openjdk.java.net/jdk/pull/8705.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8705/head:pull/8705 PR: https://git.openjdk.java.net/jdk/pull/8705
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v12]
On Fri, 13 May 2022 14:58:42 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup as suggested on the PR review. Marked as reviewed by mcimadamore (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v11]
On Fri, 13 May 2022 11:34:32 GMT, Jan Lahoda wrote:
>> src/java.base/share/classes/java/lang/MatchException.java line 58:
>>
>>> 56: * @param message the detail message (which is saved for later
>>> retrieval
>>> 57: * by the {@link #getMessage()} method).
>>> 58: * @param cause the cause (which is saved for later retrieval by
>>> the
>>
>> This looks odd - it seems like the sentence is like this:
>>
>> `the cause ( foo ). (bar)`. E.g. the test in parenthesis exceeds the test
>> outside parenthesis by a wide margin. I suggest both here and in the
>> "message" @param to avoid the parenthesis and split the sentence instead.
>> Examples:
>>
>>
>> * @param message the detail message. The message is saved for later
>> retrieval
>> * by the {@link #getMessage()} method).
>>
>>
>> and
>>
>>
>> * @param cause the cause. The cause is saved for later retrieval by the
>> * {@link #getCause()} method). A {@code null} value is
>> * permitted, and indicates that the cause is nonexistent or
>> * unknown.
>>
>>
>> Of course this is just an idea.
>
> I believe this text is taken form another exception in java.lang. If that
> would be OK, I'd look at this in a followup/separate issue.
ok!
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1802:
>>
>>> 1800: unguarded &&
>>> 1801: !patternType.isErroneous() &&
>>> 1802:
>>> types.isSubtype(types.boxedTypeOrType(types.erasure(seltype)),
>>
>> This seems to be a change compared to the previous code - e.g. handling of
>> boxing in the switch target type. Is this code even exercised? The test
>> "NotApplicableTypes" seems to rule the combination `switch (int) ... case
>> Integer` out.
>
> Yes, `switch ("int") { case Integer i -> }` is not allowed. The intent of
> `boxedTypeOrType` is to reduce follow-up errors, as `Integer i` will be
> considered to be unconditional over `int`.
Good - I suspected it had to do with error recovery - but wanted to make sure I
didn't miss anything.
-
PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 06:56:23 GMT, Alan Bateman wrote: >> Without `-XX:+UseShenandoahGC`, this bug wouldn't be exposed. >> >> What do you mean by `if you are testing with +ShenandoahGC then it will run >> already`? > > I assume you are running the tests with: >make run-tests TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" > in which case, all of the tests you select to run will be run with that GC. > > What you have is not wrong but wouldn't be a better to add a new test to > test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign? I think I agree with @AlanBateman - in the sense that this seems to go down a slippery slope where every test would need to be executed against all possible GCs. AFAIK, there are no other foreign tests doing this. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286715: Generalize MemorySegment::ofBuffer
On Fri, 13 May 2022 12:33:10 GMT, Maurizio Cimadamore
wrote:
> This patch makes MemorySegment::ofBuffer more general, by allowing clients to
> pass *any* `Buffer` instance, not just `ByteBuffer`.
> This allows us to match expressiveness of JNI API, where JNI clients can
> obtain the address of any direct buffer instance, using the
> `GetDirectBufferAddress` function.
>
> We thought about also providing a more general way to view a segment as a
> buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct
> buffers can only created form `ByteBuffer`.
> So, to create a direct `IntBuffer`, clients have to first create a direct
> `ByteBuffer` then to view that buffer as an `IntBuffer`.
>
> In other words, `IntBuffer` and friends are not first-class citizens in the
> `Buffer` API. As such it would not be possible to map many memory segments
> into an `IntBuffer`; in fact, the only segment we could safely map into an
> `IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it
> doesn't seem worth adding a lot of API surface (in terms of additional
> overloads) for such a corner case.
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 88:
> 86: * if it is associated with a confined session, it can only be accessed
> by the thread which owns the memory session.
> 87: *
> 88: * Heap segments are always associated with the {@linkplain
> MemorySession#global() global} memory session.
I've tweaked this text, as I realized the old version was erroneously
suggesting that all buffer segments were backed by a global session.
src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
line 577:
> 575: }
> 576:
> 577: private static int getScaleFactor(Buffer buffer) {
Note: for each buffer kind, there are three possible cases. Consider
`IntBuffer`: an `IntBuffer` can be backed by:
* `byte[]`, if it's a byte buffer view
* `int[]`, if it's allocated with `IntBuffer.allocate`, or `IntBuffer.wrap`
* null, if it comes from a direct byte buffer viewed as an `IntBuffer`
As such, the buffer kind (IntBuffer vs. LongBuffer) determines the buffer
element size, and therefore how much the buffer position and size should be
scaled.
But to check whether we need to create a heap vs. off-heap vs. mapped segment,
or to check _which_ kind of heap segment we need, we need to look into the
buffer base object, since there can be multiple options for any single buffer
type.
-
PR: https://git.openjdk.java.net/jdk/pull/8701
RFR: 8286715: Generalize MemorySegment::ofBuffer
This patch makes MemorySegment::ofBuffer more general, by allowing clients to pass *any* `Buffer` instance, not just `ByteBuffer`. This allows us to match expressiveness of JNI API, where JNI clients can obtain the address of any direct buffer instance, using the `GetDirectBufferAddress` function. We thought about also providing a more general way to view a segment as a buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct buffers can only created form `ByteBuffer`. So, to create a direct `IntBuffer`, clients have to first create a direct `ByteBuffer` then to view that buffer as an `IntBuffer`. In other words, `IntBuffer` and friends are not first-class citizens in the `Buffer` API. As such it would not be possible to map many memory segments into an `IntBuffer`; in fact, the only segment we could safely map into an `IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it doesn't seem worth adding a lot of API surface (in terms of additional overloads) for such a corner case. - Commit messages: - Tweak javadoc - Merge branch 'foreign-preview' into generalize_ofbuffer - Merge branch 'master' into foreign-preview - Merge branch 'master' into foreign-preview - Merge branch 'master' into foreign-preview - Fix crashes in heap segment benchmarks due to misaligned access - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java - Initial push - Add tests for loaderLookup/restricted method corner cases - ... and 59 more: https://git.openjdk.java.net/jdk/compare/11fa03f3...02494e2f Changes: https://git.openjdk.java.net/jdk/pull/8701/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8701=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286715 Stats: 95 lines in 8 files changed: 55 ins; 1 del; 39 mod Patch: https://git.openjdk.java.net/jdk/pull/8701.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8701/head:pull/8701 PR: https://git.openjdk.java.net/jdk/pull/8701
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 11:01:09 GMT, Uwe Schindler wrote: > RFE = issue? issue, with type RFE (request for enhancement) - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v11]
On Thu, 12 May 2022 10:54:16 GMT, Jan Lahoda wrote:
>> This is a (preliminary) patch for javac implementation for the third preview
>> of pattern matching for switch (type patterns in switches).
>>
>> Draft JLS:
>> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html
>>
>> The changes are:
>> -there are no guarded patterns anymore, guards are not bound to the
>> CaseElement (JLS 15.28)
>> -a new contextual keyword `when` is used to add a guard, instead of `&&`
>> -`null` selector value is handled on switch level (if a switch has `case
>> null`, it is used, otherwise a NPE is thrown), rather than on pattern
>> matching level.
>> -total patterns are allowed in `instanceof`
>> -`java.lang.MatchException` is added for the case where a switch is
>> exhaustive (due to sealed types) at compile-time, but not at runtime.
>>
>> Feedback is welcome!
>>
>> Thanks!
>
> Jan Lahoda has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Updating naming to more closely follow the spec: total patterns are
> renamed to unconditional patterns, unrefined is now unguarded.
> - Case label elements cannot be unguarded if they have a guard of a type
> different than boolean.
Looks good - left some minor comments and questions.
src/java.base/share/classes/java/lang/MatchException.java line 58:
> 56: * @param message the detail message (which is saved for later
> retrieval
> 57: * by the {@link #getMessage()} method).
> 58: * @param cause the cause (which is saved for later retrieval by the
This looks odd - it seems like the sentence is like this:
`the cause ( foo ). (bar)`. E.g. the test in parenthesis exceeds the test
outside parenthesis by a wide margin. I suggest both here and in the "message"
@param to avoid the parenthesis and split the sentence instead. Examples:
* @param message the detail message. The message is saved for later retrieval
* by the {@link #getMessage()} method).
and
* @param cause the cause. The cause is saved for later retrieval by the
* {@link #getCause()} method). A {@code null} value is
* permitted, and indicates that the cause is nonexistent or
* unknown.
Of course this is just an idea.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 177:
> 175: allowPatternSwitch = (preview.isEnabled() ||
> !preview.isPreview(Feature.PATTERN_SWITCH)) &&
> 176:
> Feature.PATTERN_SWITCH.allowedInSource(source);
> 177: allowUnconditionalPatternsInstance = (preview.isEnabled() ||
> !preview.isPreview(Feature.UNCONDITIONAL_PATTERN_IN_INSTANCEOF)) &&
Nit: perhaps adding "Of" to this already long variable name doesn't add much in
terms of chars, but makes the meaning of the variable name clearer?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1802:
> 1800: unguarded &&
> 1801: !patternType.isErroneous() &&
> 1802:
> types.isSubtype(types.boxedTypeOrType(types.erasure(seltype)),
This seems to be a change compared to the previous code - e.g. handling of
boxing in the switch target type. Is this code even exercised? The test
"NotApplicableTypes" seems to rule the combination `switch (int) ... case
Integer` out.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java line 1325:
> 1323: }
> 1324:
> 1325: public record PatternPrimaryType(Type type) {}
This is no longer needed - seems just a wrapper around a type.
-
PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 08:33:11 GMT, Uwe Schindler wrote: >> src/java.base/share/classes/java/nio/channels/FileChannel.java line 1045: >> >>> 1043: * >>> 1044: * @throws UnsupportedOperationException >>> 1045: * If an unsupported map mode is specified. >> >> I think this should mention that UOE is also throws if the filesystem >> implementation does not support memory mapping. This may happen for >> filesystems like jrtfs or custom wrapper filesystems that do not implement >> the method below. >> >> As this is already merged, should I open a PR fixing the docs or open an >> issue? > > This change here also closes > [JDK-8259034](https://bugs.openjdk.java.net/browse/JDK-8259034) @uschindler - the issue you mention with respect lack of UOE for wrong file system applies to BB as well. I suggest filing an issue. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 08:29:13 GMT, Uwe Schindler 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 65 commits:
>>
>> - Merge branch 'master' into foreign-preview
>> - Merge branch 'master' into foreign-preview
>> - Fix crashes in heap segment benchmarks due to misaligned access
>> - Merge branch 'master' into foreign-preview
>> - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> - Add tests for loaderLookup/restricted method corner cases
>> - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI
>>* Tweak restricted method check to work when there's no caller class
>> - Tweak examples in SymbolLookup javadoc
>> - Merge branch 'master' into foreign-preview
>> - Update
>> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> - ... and 55 more:
>> https://git.openjdk.java.net/jdk/compare/82aa0455...f170415b
>
> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 1164:
>
>> 1162: }
>> 1163: if (unmapper != null) {
>> 1164: AbstractMemorySegmentImpl segment = new
>> MappedMemorySegmentImpl(unmapper.address(), unmapper, size,
>
> When reviewing the method for MappedByteBuffer: I think to make this
> consistent the "old" method should also use `address()` which applies the
> pagePosition. Currently it is confusing:
> - New code returning `MemorySegment` uses `unmapper.address()`
> - Old code returning `MappedByteBuffer` uses `unmapper.address +
> unmapper.pagePosition` (fields)
>
> Should I open an issue or a PR to fix this (because this is already merged)?
>
> See the mailing list posts:
> - https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016981.html
> - https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016990.html
Please file an RFE. I suspect that there will be more little improvements and
consolidation like this we'll want to make to this code.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Integrated: 8282191: Implementation of Foreign Function & Memory API (Preview)
On Mon, 21 Mar 2022 10:45:27 GMT, Maurizio Cimadamore wrote: > This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 This pull request has now been integrated. Changeset: 2c5d1362 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/2c5d136260fa717afa374db8b923b7c886d069b7 Stats: 65711 lines in 373 files changed: 43720 ins; 19432 del; 2559 mod 8282191: Implementation of Foreign Function & Memory API (Preview) Reviewed-by: erikj, jvernee, psandoz, dholmes, mchung - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Fri, 29 Apr 2022 08:29:52 GMT, Guoxiong Li wrote: >>> Remind: please use the command `/jep JEP-424` [1] to mark this PR. >>> >>> [1] >>> https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep >> >> Question: I'm willing to try it out. If something goes wrong - would a `jep >> unneeded` be enough to "unstuck" this PR? > >> would a jep unneeded be enough to "unstuck" this PR? > > Yes if no bug. Conceptually, the `/jep unneeded` will behave as no jep > command. @lgxbslgx - JEP has been targeted, but the JEP action seems to be blocking this - what should we do? - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 65 commits: - Merge branch 'master' into foreign-preview - Merge branch 'master' into foreign-preview - Fix crashes in heap segment benchmarks due to misaligned access - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Add tests for loaderLookup/restricted method corner cases - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI * Tweak restricted method check to work when there's no caller class - Tweak examples in SymbolLookup javadoc - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - ... and 55 more: https://git.openjdk.java.net/jdk/compare/82aa0455...f170415b - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7888=44 Stats: 65711 lines in 373 files changed: 43720 ins; 19432 del; 2559 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Thu, 12 May 2022 09:24:23 GMT, Jorn Vernee wrote: > Do nothing special for async exceptions. i.e. if they happen anywhere between > 1. and 6. they will end up in one of the fallback handlers and the VM will be > terminated. My understanding is that if they materialize while we're executing the upcall Java code, if that code has a try/catch block, we will go there, rather than crash the VM. In other words, IMHO the only problem with async exception is if they occur _after_ the Java user code has completed, because that will crash the Java adapter, this preventing it from returning to native call cleanly. So, either we disable async exceptions during that phase (e.g. after user code has executed, but before we return back to native code), or we just punt and stop. Since this seems like a corner^3 case, and since there are also other issue with upcalls that can occur if other threads do not cooperate (e.g. an upcall can get stuck into an infinite safepoint if the VM exits while an async native thread runs the upcall), and given that obtaining a linker is a restricted operation anyway, I don't think we should bend over backwards to try to add 1% more safety to something that's unavoidably sharp anyways. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v44]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 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 'master' into foreign-preview - Fix crashes in heap segment benchmarks due to misaligned access - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Add tests for loaderLookup/restricted method corner cases - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI * Tweak restricted method check to work when there's no caller class - Tweak examples in SymbolLookup javadoc - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Tweak support for Linker lookup Improve javadoc of SymbolLookup - ... and 54 more: https://git.openjdk.java.net/jdk/compare/73c5e993...cdd006e7 - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7888=43 Stats: 65711 lines in 373 files changed: 43720 ins; 19432 del; 2559 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v43]
On Mon, 9 May 2022 18:09:51 GMT, ExE Boss wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix crashes in heap segment benchmarks due to misaligned access > > test/micro/org/openjdk/bench/java/lang/foreign/LoopOverPollutedSegments.java > line 69: > >> 67: static final ValueLayout.OfInt JAVA_INT_UNALIGNED = >> JAVA_INT.withBitAlignment(8); >> 68: static final ValueLayout.OfFloat JAVA_FLOAT_UNALIGNED = >> JAVA_FLOAT.withBitAlignment(8); >> 69: static final VarHandle intHandleUnaligned = >> JAVA_INT_UNALIGNED.arrayElementVarHandle(); > > To match `LoopOverNonConstantHeap`, this should probably be named > `VH_INT_UNALIGNED`. > > > > Maybe these could also be moved into > `org.openjdk.bench.java.lang.foreign.Utils`[^1] > > [^1]: > https://github.com/openjdk/jdk/blob/7b774297b1d04e104a988ea5bd2f2b04c8de3461/test/micro/org/openjdk/bench/java/lang/foreign/Utils.java#L29-L43 I noted other possible cleanups for benchmarks, I'll work on something after we integrate this PR as I'd like to minimize the churn. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v43]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix crashes in heap segment benchmarks due to misaligned access - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/3c88a2ef..7b774297 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=42 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=41-42 Stats: 41 lines in 2 files changed: 3 ins; 4 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v42]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 62 commits: - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Add tests for loaderLookup/restricted method corner cases - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI * Tweak restricted method check to work when there's no caller class - Tweak examples in SymbolLookup javadoc - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Tweak support for Linker lookup Improve javadoc of SymbolLookup - Tweak Addressable javadoc - Downcall and upcall tests use wrong layout which is missing some trailing padding - ... and 52 more: https://git.openjdk.java.net/jdk/compare/39f4434f...3c88a2ef - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7888=41 Stats: 65712 lines in 373 files changed: 43721 ins; 19432 del; 2559 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8262889: Compiler implementation for Record Patterns [v3]
On Sat, 7 May 2022 12:03:04 GMT, Jan Lahoda wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> Jan Lahoda has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Fixing guards after record patterns.
> - Raw types are not allowed in record patterns.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4208:
> 4206: if (site.tsym.kind == Kind.TYP && ((ClassSymbol)
> site.tsym).isRecord()) {
> 4207: ClassSymbol record = (ClassSymbol) site.tsym;
> 4208: if (record.type.getTypeArguments().nonEmpty()) {
There is a `Type::isRaw()` - I supposed you tried that one? Doesn't it do what
you want?
test/langtools/tools/javac/patterns/DeconstructionPatternErrors.out line 3:
> 1: DeconstructionPatternErrors.java:15:28:
> compiler.err.underscore.as.identifier
> 2: DeconstructionPatternErrors.java:15:29: compiler.err.expected:
> token.identifier
> 3: DeconstructionPatternErrors.java:43:37: compiler.err.illegal.start.of.type
should error be more specific here? E.g. diamond not supported with type test
pattern?
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v41]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/b71c4e93..f823bf84 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=40 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=39-40 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
On Fri, 6 May 2022 14:09:24 GMT, Jan Lahoda wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review feedback.
Looks good
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 752:
> 750:Iterable JCCaseLabel> labels) {
> 751: Set coveredSymbols = new HashSet<>();
> 752: Map>
> deconstructionPatternsBySymbol = new HashMap<>();
since you seem to have settled on "recordPattern" for implementation names -
you can probably revisit some of these names to say "record" instead of
"deconstruction".
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 801:
> 799: //i.e. represent all possible combinations.
> 800: //This is done by categorizing the patterns based on the
> type covered by the given
> 801: //starting component.
Example needed here. For instance (I discussed this with @biboudis):
record Outer(R r) { };
sealed interface I { };
class A implements I { };
class B implements I { };
sealed interface R { };
record Foo(I i) implements R { }
record Bar(I i) implements R { }
switch (o) {
case Outer(Foo(A), Foo(A)):
case Outer(Foo(B), Foo(B)):
case Outer(Foo(A), Foo(B)):
case Outer(Foo(B), Foo(A)):
case Outer(Bar(A), Bar(A)):
case Outer(Bar(B), Bar(B)):
case Outer(Bar(A), Bar(B)):
case Outer(Bar(B), Bar(A)):
}
Which generates two sets:
case Outer(Foo(A), Foo(A)):
case Outer(Foo(B), Foo(B)):
case Outer(Foo(A), Foo(B)):
case Outer(Foo(B), Foo(A)):
And
case Outer(Bar(A), Bar(A)):
case Outer(Bar(B), Bar(B)):
case Outer(Bar(A), Bar(B)):
case Outer(Bar(B), Bar(A)):
Sorry for being pedantic - this code is tricky and I'm worried we'll all forget
exactly how it works in 2 months :-)
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
1014:
> 1012: pattern = parsePattern(patternPos, mods, type,
> false);
> 1013: } else if (token.kind == LPAREN) {
> 1014: pattern = parsePattern(patternPos, mods, type,
> false);
Nice!
-
Marked as reviewed by mcimadamore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v39]
On Fri, 6 May 2022 08:42:12 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 120: >> >>> 118: // if there is no caller class, act as if the call came from >>> an unnamed module >>> 119: Module module = currentClass != null ? >>> 120: currentClass.getModule() : Holder.FALLBACK_MODULE; >> >> This can be simplified to: >> >> Module module = currentClass != null ? >>currentClass.getModule() : >> ClassLoader::getSystemClassLoader().getUnnamedModule(); >> >> >> No need to have the Holder class. > > gah! I missed that :-) I've addressed these comments (thanks!) and also added some tests for these corner cases. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v40]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add tests for loaderLookup/restricted method corner cases - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/4d24ffa9..b71c4e93 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=39 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=38-39 Stats: 248 lines in 10 files changed: 239 ins; 4 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Fri, 6 May 2022 10:51:33 GMT, Jan Lahoda wrote:
>> I now believe that the check is needed to properly classify patterns based
>> on the type of the i-th component. That said, not sure this should be a
>> subtyping check, or a type equality
>
> A good question. Consider code like:
>
> private void test(R r) {
> switch (r) {
> case R(A a, A v) -> {}
> case R(B b, A v) -> {}
> case R(I i, B v) -> {}
> }
> }
> final class A implements I {}
> sealed interface I permits A, B {}
> final class B implements I {}
> record R(I i1, I i2) {}
>
>
> The switch is exhaustive - all the possible combinations are covered. When
> checking the first component, the code will categorize the patterns like:
>
> A -> R(A a, A v), R(I i, B v)
> B -> R(B b, A v), R(I i, B v)
> I -> R(I i, B v)
>
> this categorization is done using the subtype check, so that `R(I i, B v)`
> will appear in the list for `A`. When checking the second component, the
> possibility for `I` is not exhaustive (does not cover `A` in the second
> component), but the possibilities for `A` and `B` are exhaustive, and they
> together cover `I`.
Ah - makes sense of course - I "just" needed a more convoluted example :-)
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v39]
On Thu, 5 May 2022 21:28:32 GMT, Mandy Chung wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI >> * Tweak restricted method check to work when there's no caller class > > src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 161: > >> 159: ClassLoader.getSystemClassLoader(); >> 160: MemorySession loaderSession = (loader == null) ? >> 161: MemorySession.global() : // boot loader never goes away > > The other built-in class loaders (`ClassLoaders::appClassLoader` and > `ClassLoaders::platformClassLoader` are both instance of > `BuiltinClassLoader`) will never be unloaded. Should they use the globel > memory session? good idea > src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 120: > >> 118: // if there is no caller class, act as if the call came from an >> unnamed module >> 119: Module module = currentClass != null ? >> 120: currentClass.getModule() : Holder.FALLBACK_MODULE; > > This can be simplified to: > > Module module = currentClass != null ? >currentClass.getModule() : > ClassLoader::getSystemClassLoader().getUnnamedModule(); > > > No need to have the Holder class. gah! I missed that :-) - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v39]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI * Tweak restricted method check to work when there's no caller class - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/853f06b8..4d24ffa9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=38 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=37-38 Stats: 22 lines in 2 files changed: 16 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Thu, 5 May 2022 16:39:08 GMT, Mandy Chung wrote: >> `BootLoader` is what you want in this case - it defines the static methods >> to access resources, packages etc defined to the boot loader (aka null >> `ClassLoader`). >> >> To find a symbol defined in the native libraries loaded by the boot loader, >> you can call `BootLoader.getNativeLibraries().find(name)`. > > When a caller-sensitive method is invoked from a thread attached via JNI, the > caller class returned from `Reflection::getCallerClass` is `null`.I would > recommend the default to be a class in the unnamed module defined to the > system class loader; hence it defaults to the system class loader. This is > consistent with JNI `FindClass` when invoked with no caller frame. > > It's an open issue [1] to revisit the default for `System::load` and > `System::loadLibrary` when invoked with null caller class. However, the > existing behavior may likely be unchanged because of the compatibility risk. > > [1] https://bugs.openjdk.java.net/browse/JDK-8281001 Thanks for the comments. I've pushed a new change which fixes a couple of thing: * first, for `SymbolLookup.loaderLookup`, the system class loader is used when no caller class exists (e.g. when method is called from JNI). If caller class exist but its loader is null (boot loader), we just call ClassLoader::findNative with a `null` loader which will do the right thing (thanks @mlchung for the tips!) * second, the restricted method check in `Reflection::ensureNativeAccess` has been improved to also work in case where caller class is `null`. In such cases, a dummy unnamed module module is used for the check. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Tue, 3 May 2022 12:07:50 GMT, Jan Lahoda wrote:
> 8262889: Compiler implementation for Record Patterns
>
> A first version of a patch that introduces record patterns into javac as a
> preview feature. For the specification, please see:
> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>
> There are two notable tricky parts:
> -in the parser, it was necessary to improve the `analyzePattern` method to
> handle nested/record patterns, while still keeping error recovery reasonable
> -in the `TransPatterns`, the desugaring of the record patterns is very
> straightforward - effectivelly the record patterns are desugared into
> guards/conditions. This will likely be improved in some future version/preview
>
> `MatchException` has been extended to cover additional cases related to
> record patterns.
I've added some renaming suggestions for the code in Flow, after some
discussions with @biboudis.
More generally, that code should contain comments, with example of what it's
trying to do - e.g. how it's partitioning the set of patterns, etc.
src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 64:
> 62: public enum Feature {
> 63: SWITCH_PATTERN_MATCHING,
> 64: DECONSTRUCTION_PATTERNS,
The spec and JEP talks about record patterns - I think we should follow the
correct name here
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 751:
> 749:Iterable JCCaseLabel> labels) {
> 750: Set constants = new HashSet<>();
> 751: Map>
> categorizedDeconstructionPatterns = new HashMap<>();
maybe rename to `deconstructionPatternsBySymbol` ?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 790:
> 788: }
> 789:
> 790: private boolean
> coversDeconstructionStartingFromComponent(DiagnosticPosition pos,
maybe rename as `coverDeconstructionFromComponent`/`coverDeconstructionAt`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 792:
> 790: private boolean
> coversDeconstructionStartingFromComponent(DiagnosticPosition pos,
> 791: Type
> targetType,
> 792:
> List patterns,
patterns -> "deconstructionPatterns"
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 800:
> 798: }
> 799:
> 800: Type parameterizedComponentType =
> types.memberType(targetType, components.get(component));
maybe `instantiatedComponentType`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 802:
> 800: Type parameterizedComponentType =
> types.memberType(targetType, components.get(component));
> 801: List nestedComponentPatterns = patterns.map(d ->
> d.nested.get(component));
> 802: Set nestedCovered = coveredSymbols(pos,
> parameterizedComponentType,
maybe `coveredComponents`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 807:
> 805: Set covered = new HashSet<>();
> 806:
> 807: for (JCDeconstructionPattern subTypeCandidate : patterns) {
`subTypeCandidate` -> `deconstructionPattern`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 809:
> 807: for (JCDeconstructionPattern subTypeCandidate : patterns) {
> 808: JCPattern nestedPattern =
> subTypeCandidate.nested.get(component);
> 809: Symbol currentPatternType;
`currentPatternType` -> `componentPatternType`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 823:
> 821: }
> 822: }
> 823: for (Symbol currentType : nestedCovered) {
`currentType` -> `coveredComponent`
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 246:
> 244: PARENTHESIZEDPATTERN,
> 245:
> 246: DECONSTRUCTIONPATTERN,
might want to rename to RECORDPATTERN (but this is impl dependent, so less
important to fix)
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 2373:
> 2371: }
> 2372:
> 2373: public static class JCDeconstructionPattern extends JCPattern
JCRecordPattern (although this is impl-only, so less important to fix)
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Thu, 5 May 2022 12:28:42 GMT, Aggelos Biboudis wrote: >>> I think this is i) from the domination relation: >>> >>> > A record pattern with type R and record component pattern list L >>> > dominates another record pattern with type S and record component pattern >>> > list M if (i) the erasure of S is a subtype of the erasure of R, and (ii) >>> > every pattern, if any, in L dominates the corresponding pattern in M. >> >> But this subtyping test seems to happen at the level of the component >> pattern list, not at the R/S level, right? > > You are right. It is the ii) which iteratively checks the component pattern > list L. I now believe that the check is needed to properly classify patterns based on the type of the i-th component. That said, not sure this should be a subtyping check, or a type equality - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Thu, 5 May 2022 12:06:38 GMT, Aggelos Biboudis wrote: > I think this is i) from the domination relation: > > > A record pattern with type R and record component pattern list L dominates > > another record pattern with type S and record component pattern list M if > > (i) the erasure of S is a subtype of the erasure of R, and (ii) every > > pattern, if any, in L dominates the corresponding pattern in M. But this subtyping test seems to happen at the level of the component pattern list, not at the R/S level, right? - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:29:53 GMT, Maurizio Cimadamore wrote: >> Good points. Regarding `ClassLoader` being null, I think we can still return >> something using the `BootLoader`'s `NativeLibraries` object - that would >> allow this method to be called internally. @mlchung Can you please confirm? >> >> As for the caller sensitive having no real caller (e.g. because method is >> called directly from native code), I think we should add a check in >> `Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such >> a check does not compromise upcall stub created via the Linker interface >> (e.g. a Java upcall might require to perform restricted operations - but >> those operation always happen inside the upcall MH, which is always >> associated with some class). > > Another option would be to treat calls to `ensureNativeAccess` with `null` > caller class as coming from unnamed module. Looking deeper, `System::loadLibrary` seems to search the boot loader libraries when invoked with a `null` caller class, so replicating that behavior should be safe. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:20:21 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
>>
>>> 151: static SymbolLookup loaderLookup() {
>>> 152: Class caller = Reflection.getCallerClass();
>>> 153: ClassLoader loader =
>>> Objects.requireNonNull(caller.getClassLoader());
>>
>> Shouldn’t this be changed to throw `IllegalCallerException` instead of
>> throwing `NullPointerException` when the `caller`’s `ClassLoader` is
>> `null`[^1] or when `caller` itself is `null`[^2]?
>>
>> [^1]: This happens when `caller` is on the bootstrap classpath.
>> [^2]: This happens when `SymbolLookup.loaderLookup()` is called by native
>> code and no **Java** code is on the call stack.
>
> Good points. Regarding `ClassLoader` being null, I think we can still return
> something using the `BootLoader`'s `NativeLibraries` object - that would
> allow this method to be called internally. @mlchung Can you please confirm?
>
> As for the caller sensitive having no real caller (e.g. because method is
> called directly from native code), I think we should add a check in
> `Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such a
> check does not compromise upcall stub created via the Linker interface (e.g.
> a Java upcall might require to perform restricted operations - but those
> operation always happen inside the upcall MH, which is always associated with
> some class).
Another option would be to treat calls to `ensureNativeAccess` with `null`
caller class as coming from unnamed module.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v38]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak examples in SymbolLookup javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/41d055ab..853f06b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=37 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=36-37 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 16:47:28 GMT, ExE Boss 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 57 commits:
>>
>> - Merge branch 'master' into foreign-preview
>> - Update
>> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> - Tweak support for Linker lookup
>>Improve javadoc of SymbolLookup
>> - Tweak Addressable javadoc
>> - Downcall and upcall tests use wrong layout which is missing some trailing
>> padding
>> - Simplify libraryLookup impl
>> - Address CSR comments
>> - Merge branch 'master' into foreign-preview
>> - Add ValueLayout changes
>> - Tweak support for array element var handle
>> - ... and 47 more:
>> https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab
>
> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
>
>> 151: static SymbolLookup loaderLookup() {
>> 152: Class caller = Reflection.getCallerClass();
>> 153: ClassLoader loader =
>> Objects.requireNonNull(caller.getClassLoader());
>
> Shouldn’t this be changed to throw `IllegalCallerException` instead of
> throwing `NullPointerException` when the `caller`’s `ClassLoader` is
> `null`[^1] or when `caller` itself is `null`[^2]?
>
> [^1]: This happens when `caller` is on the bootstrap classpath.
> [^2]: This happens when `SymbolLookup.loaderLookup()` is called by native
> code and no **Java** code is on the call stack.
Good points. Regarding `ClassLoader` being null, I think we can still return
something using the `BootLoader`'s `NativeLibraries` object - that would allow
this method to be called internally. @mlchung Can you please confirm?
As for the caller sensitive having no real caller (e.g. because method is
called directly from native code), I think we should add a check in
`Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such a
check does not compromise upcall stub created via the Linker interface (e.g. a
Java upcall might require to perform restricted operations - but those
operation always happen inside the upcall MH, which is always associated with
some class).
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Tue, 3 May 2022 12:07:50 GMT, Jan Lahoda wrote:
> 8262889: Compiler implementation for Record Patterns
>
> A first version of a patch that introduces record patterns into javac as a
> preview feature. For the specification, please see:
> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>
> There are two notable tricky parts:
> -in the parser, it was necessary to improve the `analyzePattern` method to
> handle nested/record patterns, while still keeping error recovery reasonable
> -in the `TransPatterns`, the desugaring of the record patterns is very
> straightforward - effectivelly the record patterns are desugared into
> guards/conditions. This will likely be improved in some future version/preview
>
> `MatchException` has been extended to cover additional cases related to
> record patterns.
Changes look generally good. Personally, I found the changes in Flow the harder
to follow - but I that part is just hard (even in the spec), as it has to
perform a search in both breadth (as records have more than one component) and
in depth (as patterns can be nested). I've added some comments to check my
understanding.
src/java.base/share/classes/java/lang/MatchException.java line 41:
> 39: *constants at runtime than it had at compilation time, or the
> type hierarchy has changed
> 40: *in incompatible ways between compile time and run time.
> 41: *Null targets and sealed types. If an interface or abstract
> class {@code C} is sealed
Should this be `null targets and nested sealed type test patterns` ?
Also, not sure `null` target is the right expression - maybe `null and nested
xyz` would work better?
src/jdk.compiler/share/classes/com/sun/source/tree/DeconstructionPatternTree.java
line 43:
> 41: * @return the deconstructed type
> 42: */
> 43: Tree getDeconstructor();
Shouldn't this return an ExpressionTree?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4180:
> 4178: type = attribTree(tree.var.vartype, env, varInfo);
> 4179: } else {
> 4180: type = resultInfo.pt;
Looks good - infers the binging var type from the record component being
processed. If not in a record, then I suspect resultInfo.pt is just the target
expression type (e.g. var in toplevel environment).
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 750:
> 748: private Set coveredSymbols(DiagnosticPosition pos, Type
> targetType,
> 749:Iterable JCCaseLabel> labels) {
> 750: Set constants = new HashSet<>();
Should this be called `constants` ?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 824:
> 822: }
> 823: for (Symbol currentType : nestedCovered) {
> 824: if (types.isSubtype(types.erasure(currentType.type),
Not 100% what this test does
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 836:
> 834: for (Entry> e :
> componentType2Patterns.entrySet()) {
> 835: if (coversDeconstructionStartingFromComponent(pos,
> targetType, e.getValue(), component + 1)) {
> 836: covered.add(e.getKey());
So... it seems to me that what this code is doing is that, for a component
index _i_, we get all its recursively covered symbols - then we add them to the
set of covered symbols of component _i_, but only if components _w_, where _w_
> _i_ are also covered? That is, if in `R(P1, P2, ... Pn)`, we see that `P1` is
covered, but `P2` is not, we also consider `P1` not covered (which in turn
makes `R` not covered).
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
788:
> 786: }
> 787: if (token.kind == LPAREN) {
> 788: ListBuffer nested = new ListBuffer<>();
should we add comment saying "deconstruction pattern"
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
808:
> 806: pattern = toP(F.at(pos).DeconstructionPattern(e,
> nested.toList(), var));
> 807: } else {
> 808: JCVariableDecl var = toP(F.at(token.pos).VarDef(mods,
> ident(), e, null));
and, here, a comment saying "type test pattern" ?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
1004:
> 1002: JCExpression type = unannotatedType(false);
> 1003: if (token.kind == IDENTIFIER) {
> 1004: checkSourceLevel(token.pos,
> Feature.PATTERN_MATCHING_IN_INSTANCEOF);
Two question/comments:
* I believe that `checkSourceLevel` is called here, but not in `analyzePattern`
because `analyzePattern` is also called in the `switch` code, in which case we
already check for source level which supports pattern in switch, correct?
* For type test pattern, this
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Wed, 4 May 2022 09:59:33 GMT, Maurizio Cimadamore
wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4180:
>
>> 4178: type = attribTree(tree.var.vartype, env, varInfo);
>> 4179: } else {
>> 4180: type = resultInfo.pt;
>
> Looks good - infers the binging var type from the record component being
> processed. If not in a record, then I suspect resultInfo.pt is just the
> target expression type (e.g. var in toplevel environment).
That said, I'm not sure how this connects with `instanceof`. This patch doesn't
seem to alter `visitTypeTest`. In the current code I can see this:
if (tree.pattern.getTag() == BINDINGPATTERN ||
tree.pattern.getTag() == PARENTHESIZEDPATTERN) {
attribTree(tree.pattern, env, unknownExprInfo);
...
This seems wrong for two reasons:
* it doesn't take into account the new pattern tag
* it uses an "unknown" result info when attributing, meaning that any toplevel
`var` pattern will not be attributed correctly
But we seem to have tests covering record patterns and instanceof. so I'm
wondering if I'm missing some code update?
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 57 commits: - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Tweak support for Linker lookup Improve javadoc of SymbolLookup - Tweak Addressable javadoc - Downcall and upcall tests use wrong layout which is missing some trailing padding - Simplify libraryLookup impl - Address CSR comments - Merge branch 'master' into foreign-preview - Add ValueLayout changes - Tweak support for array element var handle - ... and 47 more: https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7888=36 Stats: 65464 lines in 367 files changed: 43470 ins; 19432 del; 2562 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v35]
On Fri, 29 Apr 2022 08:15:32 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Tweak Addressable javadoc We've tweaked the support for looking up symbols in the standard libraries - `CLinker` used to implement `SymbolLookup`, now `CLinker` returns a "default lookup" instead. We've also greatly improved the javadoc of `SymbolLookup` - many thanks to Alex for the help! New javadoc here: http://cr.openjdk.java.net/~mcimadamore/8282191/v3/javadoc/java.base/module-summary.html - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v36]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Tweak support for Linker lookup Improve javadoc of SymbolLookup - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/d1fcf012..dc309e8b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=35 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=34-35 Stats: 159 lines in 14 files changed: 96 ins; 8 del; 55 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v35]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak Addressable javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/945317ec..d1fcf012 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=34 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=33-34 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Fri, 29 Apr 2022 00:44:01 GMT, Guoxiong Li wrote: > Remind: please use the command `/jep JEP-424` [1] to mark this PR. > > [1] > https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep Question: I'm willing to try it out. If something goes wrong - would a `jep unneeded` be enough to "unstuck" this PR? - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Downcall and upcall tests use wrong layout which is missing some trailing padding - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/5866bbb5..945317ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=33 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=32-33 Stats: 9 lines in 2 files changed: 7 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v33]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Simplify libraryLookup impl - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/6990183f..5866bbb5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=32 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=31-32 Stats: 21 lines in 1 file changed: 6 ins; 11 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v32]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address CSR comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/746de5ce..6990183f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=31 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=30-31 Stats: 114 lines in 9 files changed: 36 ins; 0 del; 78 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v31]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 50 commits: - Merge branch 'master' into foreign-preview - Add ValueLayout changes - Tweak support for array element var handle - Add @see - Initial push - Remove whitespaces - Drop `UnsupportedOperationException` from throws clause in FileChannel/FileChannelImpl - Add @ForceInline annotation on session accessor Beef up UnrolledAccess benchmark - Use a less expensive array element alignment check The bit to byte conversion shows up in hot paths - Fix UnrolledAccess benchmark - ... and 40 more: https://git.openjdk.java.net/jdk/compare/72f82dd7...746de5ce - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7888=30 Stats: 65314 lines in 367 files changed: 43344 ins; 19432 del; 2538 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v3]
On Tue, 12 Apr 2022 13:18:14 GMT, Jan Lahoda wrote:
>> This is a (preliminary) patch for javac implementation for the third preview
>> of pattern matching for switch (type patterns in switches).
>>
>> Draft JLS:
>> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html
>>
>> The changes are:
>> -there are no guarded patterns anymore, guards are not bound to the
>> CaseElement (JLS 15.28)
>> -a new contextual keyword `when` is used to add a guard, instead of `&&`
>> -`null` selector value is handled on switch level (if a switch has `case
>> null`, it is used, otherwise a NPE is thrown), rather than on pattern
>> matching level.
>> -total patterns are allowed in `instanceof`
>> -`java.lang.MatchException` is added for the case where a switch is
>> exhaustive (due to sealed types) at compile-time, but not at runtime.
>>
>> Feedback is welcome!
>>
>> Thanks!
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Cleanup.
Looks good - I like how moving the guard from pattern to an expression inside a
switch made the code simpler to reason about, and also simplified the compiler
API. I left some comments in the javadoc of MatchException.
src/java.base/share/classes/java/lang/MatchException.java line 33:
> 31: * Thrown to indicate an unexpected failure in pattern matching.
> 32: *
> 33: * MatchException may be thrown when an exhaustive pattern matching
> language construct
Maybe wrap with `{@code}`
src/java.base/share/classes/java/lang/MatchException.java line 34:
> 32: *
> 33: * MatchException may be thrown when an exhaustive pattern matching
> language construct
> 34: * (such as a switch expression) encounters a match target that does not
> match any of the provided
Is the term "match target" defined somewhere? In general, it would be good
quoting relevant sections of the JLS, and using consistent terminology
src/java.base/share/classes/java/lang/MatchException.java line 37:
> 35: * patterns at runtime. This can currently arise from the following case:
> 36: *
> 37: * Separate compilation anomalies, where a sealed interface has a
> different
Why a list if there's only one case? I know we have plans to expand this to
cover nulls in records patterns, but right now the javadoc should stand on its
own
src/java.base/share/classes/java/lang/MatchException.java line 52:
> 50:
> 51: /**
> 52: * Constructs an {@code MatchException} with no detail message.
double space before "message" ?
src/java.base/share/classes/java/lang/MatchException.java line 73:
> 71: *
> 72: * @param cause the cause (which is saved for later retrieval by the
> 73: * {@link #getCause()} method). (A {@code null} value is
double space before "(A" ?
src/java.base/share/classes/java/lang/MatchException.java line 88:
> 86: * by the {@link #getMessage()} method).
> 87: * @param cause the cause (which is saved for later retrieval by the
> 88: * {@link #getCause()} method). (A {@code null} value is
Same here, double space before "(A"
-
PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v30]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with four additional commits since the last revision: - Add ValueLayout changes - Tweak support for array element var handle - Add @see - Initial push - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/8637379e..2e3d57a8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=29 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=28-29 Stats: 129 lines in 4 files changed: 97 ins; 25 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v29]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Remove whitespaces - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/3a87df5e..8637379e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=28 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=27-28 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v28]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Drop `UnsupportedOperationException` from throws clause in FileChannel/FileChannelImpl - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/66cebe77..3a87df5e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=27 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=26-27 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v27]
On Wed, 13 Apr 2022 16:12:31 GMT, Alan Bateman wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add @ForceInline annotation on session accessor >> Beef up UnrolledAccess benchmark > > src/java.base/share/classes/java/nio/channels/FileChannel.java line 1052: > >> 1050: public MemorySegment map(MapMode mode, long offset, long size, >> 1051: MemorySession session) >> 1052: throws IOException, UnsupportedOperationException > > Just a minor here is that UOE is a runtime exception so probably shouldn't be > in the throws. whoops - good catch - will fix! - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v27]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add @ForceInline annotation on session accessor Beef up UnrolledAccess benchmark - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/a68195ae..66cebe77 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=26 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=25-26 Stats: 32 lines in 2 files changed: 32 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v26]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Use a less expensive array element alignment check The bit to byte conversion shows up in hot paths - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/c9afcd17..a68195ae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=25 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=24-25 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v25]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix UnrolledAccess benchmark - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/d84de510..c9afcd17 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=24 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=23-24 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v24]
On Mon, 11 Apr 2022 10:33:54 GMT, David Holmes wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix TestLinkToNativeRBP > > src/hotspot/share/prims/scopedMemoryAccess.cpp line 141: > >> 139: >> 140: /* >> 141: * This function performs a thread-local handshake against all threads >> running at the time > > Nit: thread-local?? I was assuming the term "thread-local handshake" was a thing in the VM, as per: https://openjdk.java.net/jeps/312 - PR: https://git.openjdk.java.net/jdk/pull/7888
