RFR: 8332749: Broken link in MemorySegment.Scope.html
This PR proposes to fix a broken link in the `MemorySegment.Scope` class. - Commit messages: - Fix link to section Changes: https://git.openjdk.org/jdk/pull/19366/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19366&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8332749 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19366.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19366/head:pull/19366 PR: https://git.openjdk.org/jdk/pull/19366
Re: RFR: 8332749: Broken link in MemorySegment.Scope.html
On Thu, 23 May 2024 11:39:11 GMT, Per Minborg wrote: > This PR proposes to fix a broken link in the `MemorySegment.Scope` class. Seams to work after the patch:  .../api/java.base/java/lang/foreign/MemorySegment.html#wrapping-addresses - PR Comment: https://git.openjdk.org/jdk/pull/19366#issuecomment-2126976574
Integrated: 8332749: Broken link in MemorySegment.Scope.html
On Thu, 23 May 2024 11:39:11 GMT, Per Minborg wrote: > This PR proposes to fix a broken link in the `MemorySegment.Scope` class. This pull request has now been integrated. Changeset: 0a9d1f8c Author: Per Minborg URL: https://git.openjdk.org/jdk/commit/0a9d1f8c89e946d99f01549515f6044e53992168 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8332749: Broken link in MemorySegment.Scope.html Reviewed-by: iris - PR: https://git.openjdk.org/jdk/pull/19366
Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException
On Wed, 27 Mar 2024 17:36:28 GMT, Liam Miller-Cushon wrote:
> This change overrides mutator methods in the implementation returned by
> `Map.of().entrySet()` to throw `UnsupportedOperationException`.
src/java.base/share/classes/java/util/ImmutableCollections.java line 1323:
> 1321: @Override
> 1322: public int hashCode() {
> 1323: return MapN.this.hashCode();
The hash code for a `Set` is defined as the sum of the elements in the `Set`
(hash(`null`) == 0). The `Map. Entry` hash code is defined the same way
`MapN.this.hashCode` operates so this seems right. It would be nice with a
couple of tests that assert this invariant.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18522#discussion_r1615591889
Re: RFR: 8292955: Collections.checkedMap Map.merge does not properly check key and value [v3]
On Mon, 27 May 2024 12:40:07 GMT, Chen Liang wrote: > @minborg Is it possible for you to review this collection bugfix? Will do! Thanks for the heads up. - PR Comment: https://git.openjdk.org/jdk/pull/18141#issuecomment-2134446236
Re: RFR: 8292955: Collections.checkedMap Map.merge does not properly check key and value [v3]
On Thu, 7 Mar 2024 05:33:16 GMT, Lei Zhu wrote: >> When the specified key did not associated with a value, should check the >> `key` and `value` type. > > Lei Zhu has updated the pull request incrementally with one additional commit > since the last revision: > > Use testNG builtin functionalities and modify the test function name Adding the checks before remapping seems the right thing to do in order to uphold the checked maps guarantees on types and values. - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18141#pullrequestreview-2081881249
Re: RFR: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing [v2]
On Fri, 31 May 2024 16:18:33 GMT, Maurizio Cimadamore wrote: >> This PR restores a var handle cache in `Utils::makeSegmentViewVarHandle`. >> The cache was moved to `ValueLayouts::varHandle` as part of >> [pull/19251](https://git.openjdk.org/jdk/pull/19251), on the basis that we >> want to optimize the common case like: >> >> >> ValueLayout layout = ... >> layout.varHandle().get(...) >> >> >> And that caching more complex var handles didn't seem to add value, given >> that, for these var handles, the logic in `LayoutPath` needs to adapt the >> returned var handle anyways. >> >> But, `TestAccessModes` revealed a different picture - w/o any cache in >> `Utils` the test end up allocating 8963 var handle instances (instead of >> just 4), in each of the 4 runs the test includes. While this is admittedly a >> stress test, it seems nice to restore the level of sharing we had before >> [pull/19251](https://git.openjdk.org/jdk/pull/19251). > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Would it make sense to add some tests that assert, `VarHandle` objects get cached? - PR Comment: https://git.openjdk.org/jdk/pull/19485#issuecomment-2142604322
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Wed, 29 May 2024 15:20:15 GMT, Chen Liang wrote: >> This was something that I tried and I observed a performance regression on >> the ARM platform that I was testing. From my understanding, `@Stable` tells >> the compiler to trust the contents of this field which is only given when >> the field holder is a known constant. This is generally true for cases like >> `Enums` but not usually for `Method`. >> >> >> Benchmark Mode Cnt Score Error Units >> # Intel Skylake >> MethodHashCode.benchmarkHashCode avgt5 1.113 ± 1.146 ns/op >> # Arm Neoverse N1 >> MethodHashCode.benchmarkHashCode avgt5 3.204 ± 0.001 ns/op > > Interesting, don't know about hotspot internals so I can't diagnose the exact > cause of this regression. In order to be eligible for constant folding, the benchmark must declare the `Method hashCodeMethod;` as `static final`. It is hard for me to understand why a `@Stable` annotation should have a detrimental performance impact on an instance field. Can we see a benchmark on Arm Neoverse N1 with the field declared `@Stable` compared to not declared `@Stable`? Also, if the field is `static final`, how would it look like? - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1627803876
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Wed, 5 Jun 2024 13:37:15 GMT, Per Minborg wrote: >> Interesting, don't know about hotspot internals so I can't diagnose the >> exact cause of this regression. > > In order to be eligible for constant folding, the benchmark must declare the > `Method hashCodeMethod;` as `static final`. > > It is hard for me to understand why a `@Stable` annotation should have a > detrimental performance impact on an instance field. > > Can we see a benchmark on Arm Neoverse N1 with the field declared `@Stable` > compared to not declared `@Stable`? Also, if the field is `static final`, how > would it look like? As a note, If we would have access to the contemplated `StableValue` and `hash` was declared even as an _instance variable_ `StableValue` in a regular class, then it would be trusted and would be eligible for constant folding due to the VM will have special rules for fields of StableValue type. - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1627810415
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Wed, 5 Jun 2024 13:41:10 GMT, Per Minborg wrote: >> In order to be eligible for constant folding, the benchmark must declare the >> `Method hashCodeMethod;` as `static final`. >> >> It is hard for me to understand why a `@Stable` annotation should have a >> detrimental performance impact on an instance field. >> >> ~~Can we see a benchmark on Arm Neoverse N1 with the field declared >> `@Stable` compared to not declared `@Stable`? ~~ Also, if the field is >> `static final`, how would it look like? > > As a note, If we would have access to the contemplated `StableValue` and > `hash` was declared even as an _instance variable_ `StableValue` in > a regular class, then it would be trusted and would be eligible for constant > folding due to the VM will have special rules for fields of StableValue type. Ahh. There was a benchmark in the initial message. Sorry. - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1627818122
Re: RFR: 8333774: Avoid eagerly loading various EmptySpliterator classes [v3]
On Fri, 7 Jun 2024 13:08:24 GMT, Claes Redestad wrote: >> Trivially move a few private static finals to their respective source type >> to avoid eagerly loading a few small classes. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Update src/java.base/share/classes/java/util/Spliterators.java > > Co-authored-by: Chen Liang Looks good to me. Nice observation, the laziness was for free as existing classes could be reused. - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19591#pullrequestreview-2104630412
Re: RFR: 8310843: Reimplement ByteArray and ByteArrayLittleEndian with Unsafe [v2]
On Mon, 10 Jun 2024 03:51:40 GMT, Glavo wrote: >> Things have changed since https://github.com/openjdk/jdk/pull/14636 was >> closed, so let me reopen it. >> >> https://github.com/openjdk/jdk/pull/15386 confirmed that `VarHandle` in BALE >> caused a startup regression. In order to not have any more revert patches, >> it makes sense to optimize BALE. >> >> The optimization of https://github.com/openjdk/jdk/pull/16245 allows the >> traditional expression to have good performance, but BA and BALE save us >> from having to copy these lengthy expressions everywhere. So it makes sense >> to keep them. >> >> Now here's the question, should I rewrite this PR without `Unsafe`? I'll do >> some research (e.g. does `Unsafe` have better performance during warmup?), >> but I'd also like to take some advice. > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > update copyright Would it be possible to add a benchmark for some of the methods here so we can see if there is a difference? Also, it would be interesting to see if startup performance is impacted. - PR Comment: https://git.openjdk.org/jdk/pull/19616#issuecomment-2157489839
Re: RFR: 8330465: Stable Values and Collections (Internal) [v20]
On Fri, 17 May 2024 09:31:33 GMT, Per Minborg wrote: >> # Stable Values & Collections (Internal) >> >> ## Summary >> This PR proposes to introduce an internal _Stable Values & Collections_ API, >> which provides immutable value holders where elements are initialized _at >> most once_. Stable Values & Collections offer the performance and safety >> benefits of final fields while offering greater flexibility as to the timing >> of initialization. >> >> ## Goals >> * Provide an easy and intuitive API to describe value holders that can >> change at most once. >> * Decouple declaration from initialization without significant footprint or >> performance penalties. >> * Reduce the amount of static initializer and/or field initialization code. >> * Uphold integrity and consistency, even in a multi-threaded environment. >> >> For more details, see the draft JEP: https://openjdk.org/jeps/8312611 >> >> ## Performance >> Performance compared to instance variables using two `AtomicReference` and >> two protected by double-checked locking under concurrent access by all >> threads: >> >> >> Benchmark Mode Cnt Score >> Error Units >> StableBenchmark.atomicthrpt 10259.478 ? >> 36.809 ops/us >> StableBenchmark.dcl thrpt 10225.710 ? >> 26.638 ops/us >> StableBenchmark.stablethrpt 10 4382.478 ? >> 1151.472 ops/us <- StableValue significantly faster >> >> >> Performance compared to static variables protected by `AtomicReference`, >> class-holder idiom holder, and double-checked locking (all threads): >> >> >> Benchmark Mode Cnt Score >> Error Units >> StableStaticBenchmark.atomic thrpt 10 6487.835 ? >> 385.639 ops/us >> StableStaticBenchmark.dcl thrpt 10 6605.239 ? >> 210.610 ops/us >> StableStaticBenchmark.stable thrpt 10 14338.239 ? >> 1426.874 ops/us >> StableStaticBenchmark.staticCHI thrpt 10 13780.341 ? >> 1839.651 ops/us >> >> >> Performance for stable lists (thread safe) in both instance and static >> contexts whereby we access a single value compared to `ArrayList` instances >> (which are not thread-safe) (all threads): >> >> >> Benchmark Mode Cnt Score >> Error Units >> StableListElementBenchmark.instanceArrayList thrpt 10 5812.992 ? >> 1169.730 ops/us >> StableListElementBenchmark.instanceList thrpt 10 4818.643 ? >> 704.893 ops/us >> StableListElementBenchmark... > > Per Minborg has updated the pull request incrementally with two additional > commits since the last revision: > > - Add benchmarks for memoized IntFunction and Function > - Add benchmark for memoized supplier A new PR will be made available shortly. - PR Comment: https://git.openjdk.org/jdk/pull/18794#issuecomment-2158140833
Withdrawn: 8330465: Stable Values and Collections (Internal)
On Tue, 16 Apr 2024 11:47:23 GMT, Per Minborg wrote: > # Stable Values & Collections (Internal) > > ## Summary > This PR proposes to introduce an internal _Stable Values & Collections_ API, > which provides immutable value holders where elements are initialized _at > most once_. Stable Values & Collections offer the performance and safety > benefits of final fields while offering greater flexibility as to the timing > of initialization. > > ## Goals > * Provide an easy and intuitive API to describe value holders that can > change at most once. > * Decouple declaration from initialization without significant footprint or > performance penalties. > * Reduce the amount of static initializer and/or field initialization code. > * Uphold integrity and consistency, even in a multi-threaded environment. > > For more details, see the draft JEP: https://openjdk.org/jeps/8312611 > > ## Performance > Performance compared to instance variables using two `AtomicReference` and > two protected by double-checked locking under concurrent access by all > threads: > > > Benchmark Mode Cnt Score > Error Units > StableBenchmark.atomicthrpt 10259.478 ? > 36.809 ops/us > StableBenchmark.dcl thrpt 10225.710 ? > 26.638 ops/us > StableBenchmark.stablethrpt 10 4382.478 ? > 1151.472 ops/us <- StableValue significantly faster > > > Performance compared to static variables protected by `AtomicReference`, > class-holder idiom holder, and double-checked locking (all threads): > > > Benchmark Mode Cnt Score > Error Units > StableStaticBenchmark.atomic thrpt 10 6487.835 ? > 385.639 ops/us > StableStaticBenchmark.dcl thrpt 10 6605.239 ? > 210.610 ops/us > StableStaticBenchmark.stable thrpt 10 14338.239 ? > 1426.874 ops/us > StableStaticBenchmark.staticCHI thrpt 10 13780.341 ? > 1839.651 ops/us > > > Performance for stable lists (thread safe) in both instance and static > contexts whereby we access a single value compared to `ArrayList` instances > (which are not thread-safe) (all threads): > > > Benchmark Mode Cnt Score > Error Units > StableListElementBenchmark.instanceArrayList thrpt 10 5812.992 ? > 1169.730 ops/us > StableListElementBenchmark.instanceList thrpt 10 4818.643 ? > 704.893 ops/us > StableListElementBenchmark.staticArrayListthrpt 10 7614.741 ? > 564.777 ops/us > StableListElementBe... This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/18794
RFR: 8333884: MemorySegment::reinterpret removes read-only property
This PR proposes to retain the read-only state when any of the `MemorySegment::reinterpret` methods is called. Previously, the read-only state was lost and the returned `MemorySegment` was always writable regardless of the original segment's read-only state. - Commit messages: - Make reinterpret retain read-only state Changes: https://git.openjdk.org/jdk/pull/19629/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19629&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333884 Stats: 37 lines in 4 files changed: 9 ins; 1 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/19629.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19629/head:pull/19629 PR: https://git.openjdk.org/jdk/pull/19629
Re: RFR: 8333884: MemorySegment::reinterpret removes read-only property [v2]
> This PR proposes to retain the read-only state when any of the > `MemorySegment::reinterpret` methods is called. > > Previously, the read-only state was lost and the returned `MemorySegment` was > always writable regardless of the original segment's read-only state. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Rework docs - Changes: - all: https://git.openjdk.org/jdk/pull/19629/files - new: https://git.openjdk.org/jdk/pull/19629/files/acebad9b..538eb2ed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19629&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19629&range=00-01 Stats: 29 lines in 1 file changed: 9 ins; 0 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/19629.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19629/head:pull/19629 PR: https://git.openjdk.org/jdk/pull/19629
Re: RFR: 8333884: MemorySegment::reinterpret removes read-only property [v2]
On Mon, 10 Jun 2024 13:45:16 GMT, Maurizio Cimadamore wrote: > Note that `asSlice` methods also lacks a similar guarantee on read-only (but > the impl works fine there) Even though not mentioned in the original issue, we could add documentation for this here as well. Or maybe via https://bugs.openjdk.org/browse/JDK-8333886 ? - PR Comment: https://git.openjdk.org/jdk/pull/19629#issuecomment-2158628430
Re: RFR: 8333884: MemorySegment::reinterpret removes read-only property [v3]
> This PR proposes to retain the read-only state when any of the > `MemorySegment::reinterpret` methods is called. > > Previously, the read-only state was lost and the returned `MemorySegment` was > always writable regardless of the original segment's read-only state. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Add read-only comment on asSegment too - Changes: - all: https://git.openjdk.org/jdk/pull/19629/files - new: https://git.openjdk.org/jdk/pull/19629/files/538eb2ed..bc0ade8a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19629&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19629&range=01-02 Stats: 19 lines in 1 file changed: 13 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19629.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19629/head:pull/19629 PR: https://git.openjdk.org/jdk/pull/19629
RFR: 8333886: Explicitly specify that asSlice and reinterpret return a memory segment backed by the same region of memory.
This PR proposes to explicitly state that returned segments form the `asSlice` and `reinterpret` method share memory regions with `this` memory segment. Note: The term "subset" means a true subset or the same set. - Commit messages: - Add segments share the same backing store in docs Changes: https://git.openjdk.org/jdk/pull/19633/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19633&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333886 Stats: 21 lines in 1 file changed: 21 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19633.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19633/head:pull/19633 PR: https://git.openjdk.org/jdk/pull/19633
Re: RFR: 8316493: Remove the caching fields in AbstractMap [v11]
On Fri, 10 Nov 2023 08:17:22 GMT, Per Minborg wrote: >> This PR outlines a solution for making immutable maps `@ValueBased` by >> removing cacheing of certain values in `AbstractMap`. >> >> By removing these caching fields in `AbstractMap`, we can make the immutable >> maps `@ValueBased` and at the same time, performance is likely improved >> because the JVM is probably able to optimize away object creation anyway via >> escape analysis. Also, all maps will occupy less space as we get rid of a >> number of objects and references stored for each map. >> >> We need to benchmark this solution to better understand its implications. > > Per Minborg 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 vb-map2 > - Fix formatting > - Remove caching in TreeMap > - Remove caching from CHM and CSLM > - Move back clone to original position > - Reintroduce AbstractMap::clone > - Add 'fresh' to implSpec > - Remove AbstractMap::clone > - Merge master > - Merge branch 'master' into vb-map2 > - ... and 4 more: https://git.openjdk.org/jdk/compare/9cce9fe0...b1bfcd17 Keep alive - PR Comment: https://git.openjdk.org/jdk/pull/15614#issuecomment-2158838349
Re: RFR: 8333886: Explicitly specify that asSlice and reinterpret return a memory segment backed by the same region of memory.
On Mon, 10 Jun 2024 15:45:07 GMT, Per Minborg wrote: > This PR proposes to explicitly state that returned segments form the > `asSlice` and `reinterpret` method share memory regions with `this` memory > segment. > > Note: The term "subset" means a true subset or the same set. > In my opinion, any 'sub' terminology is misleading since, as you pointed out, > one can (and often will) `reinterpret` to a _larger_ size. Wouldn't something > like, "The returned memory segment is backed by the same region of memory > that backs this memory segment", be sufficiently accurate? Could even append, > "No memory will be allocated or deallocated", to really hammer the point home. > > Using 'subset' for `asSlice` makes sense. Though possibly unnecessary. If a segment is reinterpreted to be *larger* than `this` segment, then the extra memory is not a part of `this` segment's backing part. So, in this case the total memory is a subset (but not a _proper subset_ but rather the same) of `this` backing memory _plus_ some additional memory. If a segment is reinterpreted to be *smaller* than `this` segment, then the reduced memory is a proper subset. So, strictly the proposed text is correct. The proposition "The returned memory segment is backed by the same region of memory that backs this memory segment" appears to be incorrect if a _smaller_ chunk is carved out? Maybe something more neutral like "The returned memory segment shares a region of backing memory with this segment"? Adding text similar to "No memory will be allocated or deallocated" sounds like a good idea. - PR Comment: https://git.openjdk.org/jdk/pull/19633#issuecomment-2160213978
Re: RFR: 8333886: Explicitly specify that asSlice and reinterpret return a memory segment backed by the same region of memory. [v2]
> This PR proposes to explicitly state that returned segments form the > `asSlice` and `reinterpret` method share memory regions with `this` memory > segment. > > Note: The term "subset" means a true subset or the same set. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Change wording in doc updates - Changes: - all: https://git.openjdk.org/jdk/pull/19633/files - new: https://git.openjdk.org/jdk/pull/19633/files/6cde00c8..13f33e65 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19633&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19633&range=00-01 Stats: 14 lines in 1 file changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/19633.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19633/head:pull/19633 PR: https://git.openjdk.org/jdk/pull/19633
Integrated: 8333886: Explicitly specify that asSlice and reinterpret return a memory segment backed by the same region of memory.
On Mon, 10 Jun 2024 15:45:07 GMT, Per Minborg wrote: > This PR proposes to explicitly state that returned segments form the > `asSlice` and `reinterpret` method share memory regions with `this` memory > segment. > > Note: The term "subset" means a true subset or the same set. This pull request has now been integrated. Changeset: c80e2eb3 Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/c80e2eb35c4eb03f17a2a31e979e5c369453e203 Stats: 21 lines in 1 file changed: 21 ins; 0 del; 0 mod 8333886: Explicitly specify that asSlice and reinterpret return a memory segment backed by the same region of memory. Reviewed-by: jvernee, mcimadamore - PR: https://git.openjdk.org/jdk/pull/19633
Re: RFR: 8333884: MemorySegment::reinterpret removes read-only property [v4]
> This PR proposes to retain the read-only state when any of the > `MemorySegment::reinterpret` methods is called. > > Previously, the read-only state was lost and the returned `MemorySegment` was > always writable regardless of the original segment's read-only state. Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Merge master - Add read-only comment on asSegment too - Rework docs - Make reinterpret retain read-only state - Changes: https://git.openjdk.org/jdk/pull/19629/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19629&range=03 Stats: 38 lines in 4 files changed: 30 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19629.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19629/head:pull/19629 PR: https://git.openjdk.org/jdk/pull/19629
Integrated: 8333884: MemorySegment::reinterpret removes read-only property
On Mon, 10 Jun 2024 13:18:43 GMT, Per Minborg wrote: > This PR proposes to retain the read-only state when any of the > `MemorySegment::reinterpret` methods is called. > > Previously, the read-only state was lost and the returned `MemorySegment` was > always writable regardless of the original segment's read-only state. This pull request has now been integrated. Changeset: 6f7f0f1d Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/6f7f0f1de05fdc0f6a88ccd90b806e8a5c5074ef Stats: 38 lines in 4 files changed: 30 ins; 1 del; 7 mod 8333884: MemorySegment::reinterpret removes read-only property Reviewed-by: jvernee, mcimadamore - PR: https://git.openjdk.org/jdk/pull/19629
Re: RFR: 8337712: Wrong javadoc in java.util.Date#toString(): "61" and right parenthesis
On Fri, 2 Aug 2024 07:20:39 GMT, Per Minborg wrote:
> This trivial PR proposes to add a missing parenthesis in
> `java.util.Date::toString`.
src/java.base/share/classes/java/util/Date.java line 1015:
> 1013: * {@code 59}), as two decimal digits.
> 1014: * {@code ss} is the second within the minute ({@code 00}
> through
> 1015: * {@code 61}), as two decimal digits.
The doc mentions seconds could be in the range [0, 61]. The normal interval
should be [0, 59] but maybe there are situations where leap seconds are
inserted (see https://en.wikipedia.org/wiki/Leap_second). Historically, only
one second has been used so I wonder why it says "61"?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20439#discussion_r1701412451
RFR: 8337712: Wrong javadoc in java.util.Date#toString(): "61" and right parenthesis
This trivial PR proposes to add a missing parenthesis in `java.util.Date::toString`. - Commit messages: - Update copyright year - Add missing parenthesis in Date::toString Changes: https://git.openjdk.org/jdk/pull/20439/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20439&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337712 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20439.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20439/head:pull/20439 PR: https://git.openjdk.org/jdk/pull/20439
Re: RFR: 8337712: Wrong javadoc in java.util.Date#toString(): "61" and right parenthesis
On Fri, 2 Aug 2024 07:24:20 GMT, Per Minborg wrote:
>> This trivial PR proposes to add a missing parenthesis in
>> `java.util.Date::toString`.
>
> src/java.base/share/classes/java/util/Date.java line 1015:
>
>> 1013: * {@code 59}), as two decimal digits.
>> 1014: * {@code ss} is the second within the minute ({@code 00}
>> through
>> 1015: * {@code 61}), as two decimal digits.
>
> The doc mentions seconds could be in the range [0, 61]. The normal interval
> should be [0, 59] but maybe there are situations where leap seconds are
> inserted (see https://en.wikipedia.org/wiki/Leap_second). Historically, only
> one second has been used so I wonder why it says "61"?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20439#discussion_r1701429707
Integrated: 8337712: Wrong javadoc in java.util.Date#toString(): "61" and right parenthesis
On Fri, 2 Aug 2024 07:20:39 GMT, Per Minborg wrote: > This trivial PR proposes to add a missing parenthesis in > `java.util.Date::toString`. This pull request has now been integrated. Changeset: 219e1eb1 Author: Per Minborg URL: https://git.openjdk.org/jdk/commit/219e1eb13a688532705e603e276799c0157f5f28 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8337712: Wrong javadoc in java.util.Date#toString(): "61" and right parenthesis Reviewed-by: rgiulietti - PR: https://git.openjdk.org/jdk/pull/20439
Re: RFR: 8337205: Typo in Stack vs Deque Method table in Deque specification
On Fri, 26 Jul 2024 21:06:31 GMT, Turkhan wrote: > This PR fixes `java.util.Deque`'s specification to name `peekFirst()` as an > equivalent to Stack's `peek()` method since both of them don't throw when a > collection is empty. This is not the case with the current `getFirst()` > method. > > Please let me know if I have to fix something in the PR. Will appreciate your > guidance since this is my first PR to JDK. Thanks for taking a look at this @tbadalov and sorry for the slow progress of OCA. - PR Comment: https://git.openjdk.org/jdk/pull/20363#issuecomment-2270511250
Re: RFR: 8338728: Misc issues in memory layout javadoc [v2]
On Wed, 21 Aug 2024 13:24:15 GMT, Maurizio Cimadamore wrote: >> This PR fixes two minor issues in the `MemoryLayout` javadoc: >> * the section describing dereference path talks about `P` and `P'` but then >> only uses `P` in the code; >> * the `ceilDiv` math on the `PathElement::sequenceElement(long, long)` is >> wrong, as the division returns a negative number for F < 0, which is >> incorrect > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > * Clarify javadoc for var handles with dereference path elements > * Add test where dereference path element is last element in path LGTM. Nice with a test even though just for documentation validation. - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20659#pullrequestreview-2257299547
Re: RFR: 8338731: MemoryLayout::offsetHandle can return a negative offset
On Wed, 21 Aug 2024 13:26:58 GMT, Maurizio Cimadamore wrote: > When working on startup improvements, I noticed that the method handle > returned by `MemoryLayout::offsetHandle` can overflow if the client calls the > handle with a base offset that is too big. > > In other similar situations, the layout API always fails with > `ArithmeticException` (see `MemoryLayout::scale`), so we should do the same > here. > > The fix is to use a `Math::addExact(long, long)` for the outermost add > operation in the computation of the offset method handle. That outermost > computation in fact is the only one that can overflow: it is an addition > between a user-provided base offset `B` and a layout offset `L`. `L` is > guaranteed not to overflow, by construction (as `L` is derived from a layout > path). But `B` + `L` might overflow, so the new logic checks for that. Nice catch. Unrelated: I wonder if the performance of: MH_ADD = lookup.findStatic(Long.class, "sum", MethodType.methodType(long.class, long.class, long.class)); and MH_ADD = MethodHandles.publicLookup().findStatic(Long.class, "sum", MethodType.methodType(long.class, long.class, long.class)); Differ? - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20662#pullrequestreview-2257329578 PR Comment: https://git.openjdk.org/jdk/pull/20662#issuecomment-2307163178
RFR: 8338967: Improve performance for MemorySegment::fill
The performance of the `MemorySegment::fil` can be improved by replacing the `checkAccess()` method call with calling `checkReadOnly()` instead (as the bounds of the segment itself do not need to be checked). Also, smaller segments can be handled directly by Java code rather than transitioning to native code. Here is how the `MemorySegment::fill` performance is improved by this PR:  Operations involving 8 or more bytes are delegated to native code whereas smaller segments are handled via a switch rake. It should be noted that `Arena::allocate` is using `MemorySegment::fil`. Hence, this PR will also have a positive effect on memory allocation performance. - Commit messages: - Reduce kick-in size and add test - Initial implementation Changes: https://git.openjdk.org/jdk/pull/20712/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8338967 Stats: 235 lines in 3 files changed: 231 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v2]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Remove unused import - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/4d8268d1..da88ef77 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v3]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Add a comment about the old switch type - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/da88ef77..81ed8cea Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=01-02 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v4]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Fix typo - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/81ed8cea..b28f97fa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v3]
On Mon, 26 Aug 2024 11:59:44 GMT, Per Minborg wrote: >> The performance of the `MemorySegment::fil` can be improved by replacing the >> `checkAccess()` method call with calling `checkReadOnly()` instead (as the >> bounds of the segment itself do not need to be checked). >> >> Also, smaller segments can be handled directly by Java code rather than >> transitioning to native code. >> >> Here is how the `MemorySegment::fill` performance is improved by this PR: >> >>  >> >> Operations involving 8 or more bytes are delegated to native code whereas >> smaller segments are handled via a switch rake. >> >> It should be noted that `Arena::allocate` is using `MemorySegment::fil`. >> Hence, this PR will also have a positive effect on memory allocation >> performance. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Add a comment about the old switch type Here is what it looks like for Windows x64:  - PR Comment: https://git.openjdk.org/jdk/pull/20712#issuecomment-2310048420
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v4]
On Mon, 26 Aug 2024 12:50:53 GMT, Francesco Nigro wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typo
>
> src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
> line 200:
>
>> 198: switch ((int) length) {
>> 199: case 0 : checkReadOnly(false); checkValidState();
>> break; // Explicit tests
>> 200: case 1 : set(JAVA_BYTE, 0, value); break;
>
> beware using a switch, because if this code if is too big to be inlined (or
> we're unlucky) will die due to branch-mispredict in case the different "small
> fills" are unstable/unpredictable.
> Having a test which feed different fill sizes per each iteration + counting
> branch misses, will reveal if the improvement is worthy even with such cases
It is true, that this is a compromise where we give up inline space, code-cache
space, and added complexity against the prospect of better small-size
performance. Depending on the workload, this may or may not pay off. In the
(presumably common) case where we allocate/fill small segments of constant
sizes, this is likely a win. Writing a dynamic performance test sounds like a
good idea.
> test/micro/org/openjdk/bench/java/lang/foreign/TestFill.java line 86:
>
>> 84: @Benchmark
>> 85: public void buffer_fill() {
>> 86: // Hopefully, the creation of the intermediate array will be
>> optimized away.
>
> This maybe won'twhy not making the byte array a static final?
The size of the array varies with the length of the array. It seams that escape
analysis works here though.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20712#discussion_r1731249923
PR Review Comment: https://git.openjdk.org/jdk/pull/20712#discussion_r1731252199
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v3]
On Mon, 26 Aug 2024 12:07:02 GMT, Per Minborg wrote: >> Per Minborg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add a comment about the old switch type > > Here is what it looks like for Windows x64: > >  > Hi @minborg as #16760 has shown, handling small chunks in Java can be > beneficial, but AFAIK Unsafe::setMemory has been intrinsified already at > #18555, so I'm surprised that should provide some benefit now. I think the cost of transitioning to native code is what gives us this opportunity. So, there is always a fairly constant performance hit to transition to native code. Once native, the actual filling performance is way better in native code. The cut-off size lies appears to be close to 8 bytes and hence, this PR handles 0 <= length < 7 specifically. - PR Comment: https://git.openjdk.org/jdk/pull/20712#issuecomment-2310232722
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v4]
On Mon, 26 Aug 2024 13:29:40 GMT, Per Minborg wrote:
>> src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
>> line 200:
>>
>>> 198: switch ((int) length) {
>>> 199: case 0 : checkReadOnly(false); checkValidState();
>>> break; // Explicit tests
>>> 200: case 1 : set(JAVA_BYTE, 0, value); break;
>>
>> beware using a switch, because if this code if is too big to be inlined (or
>> we're unlucky) will die due to branch-mispredict in case the different
>> "small fills" are unstable/unpredictable.
>> Having a test which feed different fill sizes per each iteration + counting
>> branch misses, will reveal if the improvement is worthy even with such cases
>
> It is true, that this is a compromise where we give up inline space,
> code-cache space, and introduce added complexity against the prospect of
> better small-size performance. Depending on the workload, this may or may not
> pay off. In the (presumably common) case where we allocate/fill small
> segments of constant sizes, this is likely a win. Writing a dynamic
> performance test sounds like a good idea.
Here is a benchmark that fills segments of various random sizes:
@BenchmarkMode(Mode.AverageTime)
@Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
@Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
@State(Scope.Thread)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Fork(value = 3)
public class TestFill {
private static final int SIZE = 16;
private static final int[] INDICES = new Random(42).ints(0, 8)
.limit(SIZE)
.toArray();
private MemorySegment[] segments;
@Setup
public void setup() {
segments = IntStream.of(INDICES)
.mapToObj(i -> MemorySegment.ofArray(new byte[i]))
.toArray(MemorySegment[]::new);
}
@Benchmark
public void heap_segment_fill() {
for (int i = 0; i < SIZE; i++) {
segments[i].fill((byte) 0);
}
}
}
This produces the following on my Mac M1:
Benchmark Mode Cnt Score Error Units
TestFill.heap_segment_fill avgt 30 59.054 ? 3.723 ns/op
On average, an operation will take 59/16 = ~3 ns per operation.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20712#discussion_r1731331461
Re: RFR: 8338979: Avoid bootstrapped switches in the classfile API [v2]
On Mon, 26 Aug 2024 13:52:35 GMT, Claes Redestad wrote:
>> Noticed these on a few startup tests in code generating proxies. Desugaring
>> switch expressions reduce risk of bootstrap circularity from any future
>> changes to switch bootstrapping (which itself depends on the classfile API).
>> It's also a tiny improvement to startup performance.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Copyright
src/java.base/share/classes/jdk/internal/classfile/impl/ClassFileImpl.java line
86:
> 84: var amo = attributeMapperOption;
> 85: for (var o : options) {
> 86: if (o instanceof StackMapsOption oo) {
Just out of curiosity, will this be slower than the switch statement?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20714#discussion_r1731374834
RFR: 8333843: Provide methods on MemorySegment to read strings with known lengths
This PR proposes to add a new overload to `MemorySegment::getString` whereby it is possible to pass in a known byte length of the content in a segment that should be converted to a String. This is useful in case one already knows the byte length and thereby does not need to scan for a null terminator. - Commit messages: - Add copyright year - Add MemorySegment::getString overload Changes: https://git.openjdk.org/jdk/pull/20725/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20725&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333843 Stats: 132 lines in 4 files changed: 114 ins; 9 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/20725.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20725/head:pull/20725 PR: https://git.openjdk.org/jdk/pull/20725
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v4]
On Mon, 26 Aug 2024 21:38:37 GMT, Maurizio Cimadamore
wrote:
>> Here is a benchmark that fills segments of various random sizes:
>>
>>
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(value = 3)
>> public class TestFill {
>>
>> private static final int SIZE = 16;
>> private static final int[] INDICES = new Random(42).ints(0, 8)
>> .limit(SIZE)
>> .toArray();
>>
>>
>> private MemorySegment[] segments;
>>
>> @Setup
>> public void setup() {
>> segments = IntStream.of(INDICES)
>> .mapToObj(i -> MemorySegment.ofArray(new byte[i]))
>> .toArray(MemorySegment[]::new);
>> }
>>
>> @Benchmark
>> public void heap_segment_fill() {
>> for (int i = 0; i < SIZE; i++) {
>> segments[i].fill((byte) 0);
>> }
>> }
>>
>> }
>>
>>
>> This produces the following on my Mac M1:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>> TestFill.heap_segment_fill avgt 30 59.054 ? 3.723 ns/op
>>
>>
>> On average, an operation will take 59/16 = ~3 ns per operation (including
>> looping).
>>
>> A test with the same size for every benchmark looks like this on my machine:
>>
>>
>> Benchmark (ELEM_SIZE) Mode Cnt Score Error Units
>> TestFill.heap_segment_fill0 avgt 30 1.112 ? 0.027 ns/op
>> TestFill.heap_segment_fill1 avgt 30 1.602 ? 0.060 ns/op
>> TestFill.heap_segment_fill2 avgt 30 1.583 ? 0.004 ns/op
>> TestFill.heap_segment_fill3 avgt 30 1.909 ? 0.055 ns/op
>> TestFill.heap_segment_fill4 avgt 30 1.605 ? 0.059 ns/op
>> TestFill.heap_segment_fill5 avgt 30 1.900 ? 0.064 ns/op
>> TestFill.heap_segment_fill6 avgt 30 1.891 ? 0.038 ns/op
>> TestFill.heap_segment_fill7 avgt 30 2.237 ? 0.091 ns/op
>
> As discussed offline, can't we use a stable array of functions or something
> like that which can be populated lazily? That way you can access the function
> you want in a single array access, and we could put all these helper methods
> somewhere else.
Unfortunately, a stable array of functions/MethodHandles didn't work from a
performance perspective.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20712#discussion_r1732503991
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v5]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'master' into fill-performance - Fix typo - Add a comment about the old switch type - Remove unused import - Reduce kick-in size and add test - Initial implementation - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/b28f97fa..6fb6eefa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=03-04 Stats: 1722 lines in 197 files changed: 1151 ins; 181 del; 390 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
RFR: 8338489: Typo in MemorySegment doc
This trivial PR proposes to fix a typo in the `MemorySegment` docs. - Commit messages: - Fix typo in MemorySegment docs Changes: https://git.openjdk.org/jdk/pull/20727/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20727&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8338489 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20727.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20727/head:pull/20727 PR: https://git.openjdk.org/jdk/pull/20727
Integrated: 8338489: Typo in MemorySegment doc
On Tue, 27 Aug 2024 10:42:08 GMT, Per Minborg wrote: > This trivial PR proposes to fix a typo in the `MemorySegment` docs. This pull request has now been integrated. Changeset: 2e96f159 Author: Per Minborg URL: https://git.openjdk.org/jdk/commit/2e96f159aaee782a627902c04dbd51daa3406ab5 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8338489: Typo in MemorySegment doc Reviewed-by: rriggs, mcimadamore, iris - PR: https://git.openjdk.org/jdk/pull/20727
Re: RFR: 8338731: MemoryLayout::offsetHandle can return a negative offset [v2]
On Tue, 27 Aug 2024 14:45:37 GMT, Maurizio Cimadamore wrote: >> When working on startup improvements, I noticed that the method handle >> returned by `MemoryLayout::offsetHandle` can overflow if the client calls >> the handle with a base offset that is too big. >> >> In other similar situations, the layout API always fails with >> `ArithmeticException` (see `MemoryLayout::scale`), so we should do the same >> here. >> >> The fix is to use a `Math::addExact(long, long)` for the outermost add >> operation in the computation of the offset method handle. That outermost >> computation in fact is the only one that can overflow: it is an addition >> between a user-provided base offset `B` and a layout offset `L`. `L` is >> guaranteed not to overflow, by construction (as `L` is derived from a layout >> path). But `B` + `L` might overflow, so the new logic checks for that. > > Maurizio Cimadamore has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains three commits: > > - Merge branch 'master' into offset_overflow > - Merge branch 'master' into offset_overflow > - Initial push Merge ok ;-) - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20662#pullrequestreview-2265544939
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v5]
On Tue, 27 Aug 2024 20:25:46 GMT, Paul Sandoz wrote: > How fast do we need to be here given we are measuring in a few nanoseconds > per operation? > > What if the goal is not to regress from say explicitly filling in a small > sized segment or a comparable array (e.g., < 8 bytes) then maybe a loop > suffices and the code is simple? Fair question. I have another version (called "patch bits" below) that is based on bit logic (first doing int ops, then short and lastly byte, similar to `ArraySupport::vectorizedMismatch`). This has slightly worse performance but is more scalable and perhaps simpler.  - PR Comment: https://git.openjdk.org/jdk/pull/20712#issuecomment-2314760835
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v6]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Switch to bit checking instead of switch statement - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/6fb6eefa..e2a52912 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=04-05 Stats: 33 lines in 1 file changed: 6 ins; 15 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v7]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Make minor updates after comments - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/e2a52912..78eb92a6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=05-06 Stats: 9 lines in 2 files changed: 2 ins; 6 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v9]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Remove unused imports - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/fc0e1aee..e3c0417b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=07-08 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v8]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Move logic to ScopedMemoryAccess for improved performance - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/78eb92a6..fc0e1aee Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=06-07 Stats: 102 lines in 4 files changed: 58 ins; 25 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v10]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request incrementally with two additional commits since the last revision: - Revert copyright year - Move logic back to AMSI - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/e3c0417b..5c2ce6ad Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=08-09 Stats: 82 lines in 2 files changed: 40 ins; 36 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v5]
On Wed, 28 Aug 2024 15:32:40 GMT, Francesco Nigro wrote: >>> How fast do we need to be here given we are measuring in a few nanoseconds >>> per operation? >>> >>> What if the goal is not to regress from say explicitly filling in a small >>> sized segment or a comparable array (e.g., < 8 bytes) then maybe a loop >>> suffices and the code is simple? >> >> Fair question. I have another version (called "patch bits" below) that is >> based on bit logic (first doing int ops, then short and lastly byte, similar >> to `ArraySupport::vectorizedMismatch`). This has slightly worse performance >> but is more scalable and perhaps simpler. >> >>  > > @minborg Hi! I didn't checked the numbers with the benchmark I've written at > https://github.com/openjdk/jdk/pull/20712#discussion_r1732802685 which is > meant to stress the branch predictor (without enough `samples` i.e. past 128K > on my machine) - can you give it a shot with M1 🙏 ? @franz1981 Here is what I get if I run your performance test on my M1 Mac (unfortunately no -perf data): Benchmark (samples) (shuffle) Mode CntScore Error Units TestBranchFill.heap_segment_fill 1024 false avgt 30 3695.815 ?24.615 ns/op TestBranchFill.heap_segment_fill 1024 true avgt 30 3938.582 ? 124.510 ns/op TestBranchFill.heap_segment_fill 128000 false avgt 30 420845.301 ? 1605.080 ns/op TestBranchFill.heap_segment_fill 128000 true avgt 30 1778362.506 ? 39250.756 ns/op - PR Comment: https://git.openjdk.org/jdk/pull/20712#issuecomment-2321048180
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v10]
On Fri, 30 Aug 2024 10:51:59 GMT, Per Minborg wrote: >> The performance of the `MemorySegment::fil` can be improved by replacing the >> `checkAccess()` method call with calling `checkReadOnly()` instead (as the >> bounds of the segment itself do not need to be checked). >> >> Also, smaller segments can be handled directly by Java code rather than >> transitioning to native code. >> >> Here is how the `MemorySegment::fill` performance is improved by this PR: >> >>  >> >> >> Operations involving 8 or more bytes are delegated to native code whereas >> smaller segments are handled via a switch rake. >> >> It should be noted that `Arena::allocate` is using `MemorySegment::fil`. >> Hence, this PR will also have a positive effect on memory allocation >> performance. > > Per Minborg has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert copyright year > - Move logic back to AMSI Here is the performance improvement for Linux a64. Looks like a significant improvement.  - PR Comment: https://git.openjdk.org/jdk/pull/20712#issuecomment-2324186401
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v10]
On Fri, 30 Aug 2024 14:15:24 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
>> line 208:
>>
>>> 206: }
>>> 207: final long u = Byte.toUnsignedLong(value);
>>> 208: final long longValue = u << 56 | u << 48 | u << 40 | u <<
>>> 32 | u << 24 | u << 16 | u << 8 | u;
>>
>> this can be u * 0xL if value != 0 and just 0L if not: not sure
>> if fast(er), need to measure.
>>
>> Most of the time filling is happy with 0 since zeroing is the most common
>> case
>
>> this can be u * 0xL if value != 0 and just 0L if not: not sure
>> if fast(er), need to measure.
>>
>> Most of the time filling is happy with 0 since zeroing is the most common
>> case
>
> It's a clever trick. However, I was looking at similar tricks and found that
> the time spent here is irrelevant (e.g. I tried to always force `0` as the
> value, and couldn't see any difference).
If I run:
@Benchmark
public long shift() {
return ELEM_SIZE << 56 | ELEM_SIZE << 48 | ELEM_SIZE << 40 | ELEM_SIZE
<< 32 | ELEM_SIZE << 24 | ELEM_SIZE << 16 | ELEM_SIZE << 8 | ELEM_SIZE;
}
@Benchmark
public long mul() {
return ELEM_SIZE * 0x__L;
}
Then I get:
Benchmark (ELEM_SIZE) Mode Cnt Score Error Units
TestFill.mul 31 avgt 30 0.586 ? 0.045 ns/op
TestFill.shift 31 avgt 30 0.938 ? 0.017 ns/op
On my M1 machine.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20712#discussion_r1740564110
RFR: 8338591: Improve performance of MemorySegment::copy
This PR proposes to handle smaller FFM copy operations with Java code rather than transitioning to native code. This will improve performance. In this PR, copy operations involving zero to 63 bytes will be handled by Java code. Here is what it looks like for Windows x64:  Here is another chart for Linux a64:  Other platforms exhibit similar behavior. It should be noted that the gain with this PR is pronounced for certain common sizes that are more likely to appear in code (e.g. 8, 16, 24, and 32) It would be possible to use the same code path for the 7arg `MemorySegment::copy` method if it is similar to: MemorySegment.copy(heapSrcSegment, JAVA_BYTE, 0, heapDstSegment, JAVA_BYTE, 0, ELEM_SIZE); This could be added in a separate PR. - Commit messages: - Fix copyright years - Clean up - Increase threshhold to 64 bytes - Merge branch 'master' into copy-performance - Merge branch 'master' into mismatch-performance - Only handle disjunct mem copy - Add implementation - Improve benchmarks - Fix bugs - Add prototypes Changes: https://git.openjdk.org/jdk/pull/20829/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20829&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8338591 Stats: 299 lines in 4 files changed: 286 ins; 6 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/20829.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20829/head:pull/20829 PR: https://git.openjdk.org/jdk/pull/20829
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v11]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg 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 14 additional commits since the last revision: - Merge branch 'master' into fill-performance - Restructure logic after comments - Revert copyright year - Move logic back to AMSI - Remove unused imports - Move logic to ScopedMemoryAccess for improved performance - Make minor updates after comments - Switch to bit checking instead of switch statement - Merge branch 'master' into fill-performance - Fix typo - ... and 4 more: https://git.openjdk.org/jdk/compare/726d0a24...d1641954 - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/5c2ce6ad..d1641954 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=09-10 Stats: 8765 lines in 272 files changed: 5557 ins; 1502 del; 1706 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v10]
On Mon, 2 Sep 2024 09:32:56 GMT, Maurizio Cimadamore
wrote:
>> If I run:
>>
>>
>> @Benchmark
>> public long shift() {
>> return ELEM_SIZE << 56 | ELEM_SIZE << 48 | ELEM_SIZE << 40 |
>> ELEM_SIZE << 32 | ELEM_SIZE << 24 | ELEM_SIZE << 16 | ELEM_SIZE << 8 |
>> ELEM_SIZE;
>> }
>>
>> @Benchmark
>> public long mul() {
>> return ELEM_SIZE * 0x__L;
>> }
>>
>> Then I get:
>>
>> Benchmark (ELEM_SIZE) Mode Cnt Score Error Units
>> TestFill.mul 31 avgt 30 0.586 ? 0.045 ns/op
>> TestFill.shift 31 avgt 30 0.938 ? 0.017 ns/op
>>
>> On my M1 machine.
>
> I found similar small improvements to be had (I wrote about them offline)
> when replacing the bitwise-based tests (e.g. `foo & 4 != 0`) with a more
> explicit check for `remainingBytes >=4`. Seems like bitwise operations are
> not as optimized (or perhaps the assembly instructions for them is overall
> more convoluted - I haven't checked).
I've tried
final long longValue = Byte.toUnsignedLong(value) * 0x0101010101010101L;
But it had the same performance as explicit bit shifting on M1.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20712#discussion_r1741664877
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v12]
> The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Remove unintended file - Changes: - all: https://git.openjdk.org/jdk/pull/20712/files - new: https://git.openjdk.org/jdk/pull/20712/files/d1641954..f0ca9853 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20712&range=10-11 Stats: 65 lines in 1 file changed: 0 ins; 65 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20712.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20712/head:pull/20712 PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338967: Improve performance for MemorySegment::fill [v10]
On Tue, 3 Sep 2024 08:49:58 GMT, Francesco Nigro wrote: >> I've tried >> >> >> final long longValue = Byte.toUnsignedLong(value) * 0x0101010101010101L; >> >> >> But it had the same performance as explicit bit shifting on M1. > > @minborg the ` ELEM_SIZE` is a `Param` field right? Just to be 100% sure of > it... yes it is. But I think the reason we are not seeing any difference is that in the fill benchmarks, we are always using a value of zero. We should keep this trick in mind for later... - PR Review Comment: https://git.openjdk.org/jdk/pull/20712#discussion_r1741814268
Integrated: 8338967: Improve performance for MemorySegment::fill
On Mon, 26 Aug 2024 11:47:11 GMT, Per Minborg wrote: > The performance of the `MemorySegment::fil` can be improved by replacing the > `checkAccess()` method call with calling `checkReadOnly()` instead (as the > bounds of the segment itself do not need to be checked). > > Also, smaller segments can be handled directly by Java code rather than > transitioning to native code. > > Here is how the `MemorySegment::fill` performance is improved by this PR: > >  > > > Operations involving 8 or more bytes are delegated to native code whereas > smaller segments are handled via a switch rake. > > It should be noted that `Arena::allocate` is using `MemorySegment::fil`. > Hence, this PR will also have a positive effect on memory allocation > performance. This pull request has now been integrated. Changeset: 7a418fc0 Author:Per Minborg URL: https://git.openjdk.org/jdk/commit/7a418fc07464fe359a0b45b6d797c65c573770cb Stats: 277 lines in 3 files changed: 274 ins; 0 del; 3 mod 8338967: Improve performance for MemorySegment::fill Reviewed-by: mcimadamore, psandoz - PR: https://git.openjdk.org/jdk/pull/20712
Re: RFR: 8338591: Improve performance of MemorySegment::copy
On Tue, 3 Sep 2024 11:32:26 GMT, Maurizio Cimadamore
wrote:
>> This PR proposes to handle smaller FFM copy operations with Java code rather
>> than transitioning to native code. This will improve performance. In this
>> PR, copy operations involving zero to 63 bytes will be handled by Java code.
>>
>> Here is what it looks like for Windows x64:
>>
>> 
>>
>> Here is another chart for Linux a64:
>>
>> 
>>
>> Other platforms exhibit similar behavior. It should be noted that the gain
>> with this PR is pronounced for certain common sizes that are more likely to
>> appear in code (e.g. 8, 16, 24, and 32)
>>
>> It would be possible to use the same code path for the 7arg
>> `MemorySegment::copy` method if it is similar to:
>>
>>
>> MemorySegment.copy(heapSrcSegment, JAVA_BYTE, 0, heapDstSegment, JAVA_BYTE,
>> 0, ELEM_SIZE);
>>
>>
>> This could be added in a separate PR.
>>
>> This PR has been tested with tier1-3 and passed.
>
> src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
> line 625:
>
>> 623: if (size <= 0) {
>> 624: // Do nothing
>> 625: } else if (size < COPY_NATIVE_THRESHOLD && !src.overlaps(dst)) {
>
> This is basically a conservative test - correct? E.g. we might end up
> cosidering two segments as overlapping even when they are not (because of
> offsets) - but that's ok, I actually think it's a good pragmatic solution.
Yes, it is conservative. We can tolerate overlaps in one direction (but not the
other). We refrain from using these cases as they are unlikely, and checking
would incur a performance price for all other (much more likely) cases.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20829#discussion_r1741982874
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 12:34:43 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add `since` tag in Module/ModuleLayer preview methods src/java.base/share/classes/java/lang/foreign/Arena.java line 32: > 30: > 31: /** > 32: * An arena controls the lifecycle of one or more memory segments, > providing both flexible allocation and timely deallocation. Strictly: "An arena controls the lifecycle of zero or more ...". A newly created Arena, for example, does not control the lifecycle of any segment. - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 12:34:43 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add `since` tag in Module/ModuleLayer preview methods
src/java.base/share/classes/java/lang/foreign/Arena.java line 35:
> 33: *
> 34: * An arena has a {@linkplain #scope() scope}, called the arena scope.
> When the arena is {@linkplain #close() closed},
> 35: * the arena scope becomes not {@linkplain SegmentScope#isAlive() alive}.
> As a result, all the
Suggest "the arena scope is no longer {@linkplain SegmentScope#isAlive()
alive}" instead.
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 12:34:43 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add `since` tag in Module/ModuleLayer preview methods src/java.base/share/classes/java/lang/foreign/Arena.java line 63: > 61: * after the arena has been closed. The cost of providing this > guarantee varies based on the > 62: * number of threads that have access to the memory segments allocated by > the arena. For instance, if an arena > 63: * is always created and closed by one thread, and the memory segments > associated with the arena's scope are always Strictly, if a shared segment is created and is only accessed by a single thread, then we need to track thread usage in order to trivially ensure safety. I think we could reword so that if access is only *allowed* by a single thread, it is trivial. - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 14:49:30 GMT, Per Minborg wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add `since` tag in Module/ModuleLayer preview methods > > src/java.base/share/classes/java/lang/foreign/Arena.java line 63: > >> 61: * after the arena has been closed. The cost of providing this >> guarantee varies based on the >> 62: * number of threads that have access to the memory segments allocated >> by the arena. For instance, if an arena >> 63: * is always created and closed by one thread, and the memory segments >> associated with the arena's scope are always > > ~~Strictly, if a shared segment is created and is only accessed by a single > thread, then we need to track thread usage in order to trivially ensure > safety. I think we could reword so that if access is only *allowed* by a > single thread, it is trivial.~~ ok. So reading on the initial text makes sense. So, my comment above should be disregarded. - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 12:34:43 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add `since` tag in Module/ModuleLayer preview methods src/java.base/share/classes/java/lang/foreign/Arena.java line 100: > 98: * MemorySegment.allocateNative(bytesSize, byteAlignment, scope()); > 99: *} > 100: * More generally implementations of this method must return a > native method featuring the requested size, ... must return a native ~~method~~*segment* featuring ... - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 12:34:43 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add `since` tag in Module/ModuleLayer preview methods src/java.base/share/classes/java/lang/foreign/Arena.java line 89: > 87: > 88: /** > 89: * Returns a native memory segment with the given size (in bytes) and > alignment constraint (in bytes). It is noted that the current documentation does not require a **new** native memory segment to be returned. Would it not be better with: Creates a new native memory segment ... The new shared segment might share actual backing memory though. - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 12:34:43 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add `since` tag in Module/ModuleLayer preview methods
src/java.base/share/classes/java/lang/foreign/Arena.java line 119:
> 117:
> 118: /**
> 119: * {@return the arena scope}
Add a period ('.') after the closing curly bracket.
src/java.base/share/classes/java/lang/foreign/Arena.java line 124:
> 122:
> 123: /**
> 124: * Closes this arena. If this method completes normally, the arena
> scope becomes not {@linkplain SegmentScope#isAlive() alive},
See comment above "not alive" -> "is no longer alive"
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 12:34:43 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add `since` tag in Module/ModuleLayer preview methods
src/java.base/share/classes/java/lang/foreign/Arena.java line 79:
> 77: *
> 78: * Shared arenas, on the other hand, have no owner thread. The segments
> created by a shared arena
> 79: * can be {@linkplain SegmentScope#isAccessibleBy(Thread) accessed} by
> multiple threads. This might be useful when
Suggest "can be {@linkplain SegmentScope#isAccessibleBy(Thread) accessed} by
~~multiple~~ *any* thread"
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 12:34:43 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add `since` tag in Module/ModuleLayer preview methods src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 163: > 161: * segment is derived from the address of the original segment, by > adding an offset (expressed in bytes). The size of > 162: * the sliced segment is either derived implicitly (by subtracting the > specified offset from the size of the original segment), > 163: * or provided explicitly. In other words, a sliced segment has > stricter spatial bounds than those of the original segment: Strictly, a sliced segment can have the *same* spatial bounds as the original segment. - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v22]
On Tue, 15 Nov 2022 12:34:43 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add `since` tag in Module/ModuleLayer preview methods
src/java.base/share/classes/java/lang/foreign/Arena.java line 136:
> 134:
> 135: /**
> 136: * {@return {@code true} if the provided thread can close this arena}
I think this is equivalent and simpler:
{@return if the provided thread can close this arena}.
But I know there are many examples of {@code true} in the JDK.
src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 46:
> 44:
> 45: /**
> 46: * Returns the member layouts associated with this group.
We may use {@return the member layouts associated with this group}.
src/java.base/share/classes/java/lang/foreign/Linker.java line 264:
> 262:
> 263: /**
> 264: * Returns a symbol lookup for symbols in a set of commonly used
> libraries.
Use {@return ...}
-
PR: https://git.openjdk.org/jdk/pull/10872
RFR: 8297145: Add a @sealedGraph tag to ConstantDesc
This PR proposes to opt in for graphic rendering of the sealed hierarchy of the ConstantDesc class. Rendering capability was added via https://bugs.openjdk.org/browse/JDK-8295653 Here is how it would look like: https://user-images.githubusercontent.com/7457876/202216017-ec996722-2956-4b0a-adb5-538bce12c135.png";> - Commit messages: - Add a @sealedGraph tag to ConstantDesc Changes: https://git.openjdk.org/jdk/pull/11187/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11187&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297145 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/11187.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11187/head:pull/11187 PR: https://git.openjdk.org/jdk/pull/11187
RFR: 8297148: Add a @sealedGraph tag to CallSite
This PR proposes to opt in for graphic rendering of the sealed hierarchy of the CallSite class. Rendering capability was added via https://bugs.openjdk.org/browse/JDK-8295653 Here is how it would look like: https://user-images.githubusercontent.com/7457876/202234105-8029ade2-4feb-456c-a010-b80122195f0f.png";> - Commit messages: - Add a @sealedGraph tag to CallSite Changes: https://git.openjdk.org/jdk/pull/11190/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11190&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297148 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/11190.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11190/head:pull/11190 PR: https://git.openjdk.org/jdk/pull/11190
RFR: 8297150: Add a @sealedGraph tag to Reference
This PR proposes to opt in for graphic rendering of the sealed hierarchy of the `Reference` class. Rendering capability was added via https://bugs.openjdk.org/browse/JDK-8295653 Here is how it would look like: https://user-images.githubusercontent.com/7457876/202243138-7f2a799a-dc67-4c4e-9322-c18bf05d73e7.png";> - Commit messages: - Add a @sealedGraph tag to Reference Changes: https://git.openjdk.org/jdk/pull/11191/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11191&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297150 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/11191.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11191/head:pull/11191 PR: https://git.openjdk.org/jdk/pull/11191
Re: RFR: 8297148: Add a @sealedGraph tag to CallSite
On Wed, 16 Nov 2022 16:41:18 GMT, Joe Darcy wrote: > Helpful improvement. > > Are the type names in the diagram links? The diagram is an SVG image rendered via the DOT description language. I think, in theory, it would be possible to add links. I did some experiments with this but decided we might add it later. - PR: https://git.openjdk.org/jdk/pull/11190
RFR: 8296024: Usage of DIrectBuffer::address should be guarded
This PR proposes the introduction of **guarding** of the use of
`DirectBuffer::address` within the JDK. With the introduction of the Foreign
Function and Memory access, it is possible to derive Buffer instances that are
backed by native memory that, in turn, can be closed asynchronously by the user
(and not only via a `Cleaner` when there is no other reference to the `Buffer`
object). If another thread is invoking `MemorySession::close` while a thread is
doing work using raw addresses, the outcome is undefined. This means the JVM
might crash or even worse, silent modification of unrelated memory might occur.
Design choices in this PR:
There is already a method `MemorySession::whileAlive` that takes a runnable and
that will perform the provided action while acquiring the underlying`
MemorySession` (if any). However, using this method entailed relatively large
changes while converting larger/nested code segments into lambdas. This would
also risk introducing lambda capturing. So, instead, a try-with-resources
friendly access method was added. This made is more easy to add guarding and
did not risk lambda capturing. Also, introducing lambdas in certain fundamental
JDK classes might incur bootstrap problems.
The aforementioned TwR is using a "session acquisition" that is not used
explicitly in the try block itself. This raises a warning that is suppressed
using `@SuppressWarnings("try")`. In the future, we might be able to remove
these suppressions by using the reserved variable name `_`. Another alternative
was evaluated where a non-autocloseable resource was used. However, it became
relatively complicated to guarantee that the session was always released and,
at the same time, the existing 'final` block was always executed properly (as
they both might throw exceptions). In the end, the proposed solution was much
simpler and robust despite requiring a non-referenced TwR variable.
In some cases, where is is guaranteed that the backing memory session is
non-closeable, we do not have to guard the use of `DirectBuffer::address`.
These cases have been documented in the code.
On some occasions, a plurality of acquisitions are made. This would never lead
to deadlocks as acquisitions are fundamentally concurrent counters and not
resources that only one thread can "own".
I have added comments (and not javadocs) before the declaration of the
non-public-api `DirectBuffer::address` method, that the use of the returned
address needs to be guarded. It can be discussed if this is preferable or not.
This PR spawns several areas of responsibility and so, I expect more than one
reviewer before promoting the PR.
-
Commit messages:
- Guard usages of DirectBuffer::address
Changes: https://git.openjdk.org/jdk/pull/11260/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8296024
Stats: 277 lines in 26 files changed: 191 ins; 4 del; 82 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded
On Mon, 21 Nov 2022 11:29:07 GMT, Alan Bateman wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> These cases have been documented in the code.
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java line 252:
>
>> 250: try {
>> 251: // 'dst' is guaranteed not to be associated with a
>> closeable session.
>> 252: // Hence, there is no need for acquiring any session.
>
> This comment is will be confusing to anyone reading this code. Is this really
> needed?
The reason for the comment is to make it clear why `DirectBuffer::address` can
be used directly without guarding. This will also reduce the probability of
unnecessary guarding being added in the future. However, if the consensus is
that these comments just adds confusion, I am happy to remove them.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded
On Mon, 21 Nov 2022 11:32:35 GMT, Alan Bateman wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> These cases have been documented in the code.
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> src/java.base/share/classes/java/util/zip/Adler32.java line 102:
>
>> 100: return;
>> 101: if (buffer.isDirect()) {
>> 102: try (var sessionAcquisition =
>> NIO_ACCESS.acquireSessionAsAutoCloseable(buffer)) {
>
> We need to find something better than "sessionAcquisition", it looks very
> messy at all these use sites.
Eventually, I hope most of them will be named `_`.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v2]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> These cases have been documented in the code.
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Cleanup after comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/23a6fd14..9fafafac
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=00-01
Stats: 50 lines in 20 files changed: 6 ins; 0 del; 44 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v3]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Remove comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/9fafafac..9fcf2bb3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=01-02
Stats: 18 lines in 6 files changed: 4 ins; 14 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v3]
On Mon, 21 Nov 2022 12:19:21 GMT, Alan Bateman wrote: >> maybe just `bufferLock` and and just `acquireBuffer` ? > > Would it be possible to re-examine acquireSession returning Runnable and > acquireSessionAsAutoCloseable returning AutoCloseable. The naming is bit > awkward at most of the use sites and maybe we can do better. I think it would be confusing to have overloads with the same name. The `aquireSession(Buffer, boolean)` method is only used once and is used in `IOUtil` so we could rename it to `aquireSessionOrNull` and then rename `acquireSessionAsAutoCloseable` to `acquireSession`. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Rename methods
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/9fcf2bb3..c081b4ae
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=02-03
Stats: 51 lines in 20 files changed: 0 ins; 6 del; 45 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v5]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Cleanup static declarations
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/c081b4ae..eca7c388
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=03-04
Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]
On Mon, 21 Nov 2022 13:32:43 GMT, ExE Boss wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rename methods
>
> src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 41:
>
>> 39: // An example of a guarded use of a memory address is shown here:
>> 40: //
>> 41: // try (var guard = NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {
>
> Suggestion:
>
> // try (var guard = NIO_ACCESS.acquireSession(bb)) {
Are you happy with the proposed renaming now @ExE-Boss ? I think our
propositions crossed in time?
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with two additional
commits since the last revision:
- Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/eca7c388..a0de3bcf
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=04-05
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v7]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has refreshed the contents of this pull request, and previous
commits have been removed. Incremental views are not available. The pull
request now contains six commits:
- Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- Cleanup static declarations
- Rename methods
- Remove comments
- Cleanup after comments
- Guard usages of DirectBuffer::address
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/a0de3bcf..17a72e9f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=06
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=05-06
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]
On Mon, 21 Nov 2022 14:14:14 GMT, Alan Bateman wrote: >> Per Minborg has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> > > src/java.base/share/classes/sun/nio/fs/NativeBuffers.java line 92: > >> 90: * allocated from the heap. >> 91: * >> 92: * The returned NativeBuffer is guaranteed not to be asynchronously >> closeable. > > This class, and the temporary buffer cache in sun.nio.ch.Util, are intended > to be used with try-finally. There isn't any notion of asynchronously close > so maybe it would best to not add this comment to these sources. That is clear to me but I am trying to prevent future redundant guarding. Anyway, I will remove the comments. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v8]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with three additional
commits since the last revision:
- Merge branch 'guard-directbuffer-address' of https://github.com/minborg/jdk
into guard-directbuffer-address
- Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- Remove comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/17a72e9f..88ed3aa2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=07
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=06-07
Stats: 5 lines in 3 files changed: 0 ins; 4 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]
On Mon, 21 Nov 2022 14:07:38 GMT, Alan Bateman wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rename methods
>
> src/java.base/share/classes/java/util/zip/Adler32.java line 105:
>
>> 103: adler = updateByteBuffer(adler,
>> ((DirectBuffer)buffer).address(), pos, rem);
>> 104: } finally {
>> 105: Reference.reachabilityFence(buffer);
>
> The updated naming is a bit better but there are it still feels that that
> there are too many things going on at the use sites ("guard", "session",
> "reachability fence"). Ideally the acquire would return something with an
> accessor for the direct buffer address but that may not be possible. I
> wonder if changing NOP_CLOSE.close to do reachabilityFence(this) would work
> as expected and improve many of these use sites?
This can certainly be done. We could, for example, add a new method to the
`JavaNioAccess` interface:
```AddressAcquisition acquireSession2(Buffer buffer); // to be renamed```
This would allow us to go from:
try (var guard = NIO_ACCESS.acquireSession(buffer)) {
adler = updateByteBuffer(adler,
((DirectBuffer)buffer).address(), pos, rem);
} finally {
Reference.reachabilityFence(buffer);
}
to
try (var guard = NIO_ACCESS.acquireSession2(buffer)) {
adler = updateByteBuffer(adler, guard.address(), pos, rem);
}
Although this looks much simpler and concise, it means "a new object is created
for each invocation" (*). This also allows us to remove the
`@SupressWarnings("try")` annotations.
Here is how the undercarriage might look like:
@Override
public AddressAcquisition acquireSession2(Buffer buffer) {
var session = buffer.session();
if (session == null) {
return AddressAcquisition.create(buffer, () -> {});
}
session.acquire0();
return AddressAcquisition.create(buffer, session::release0);
}
and
sealed interface AddressAcquisition extends AutoCloseable {
long address();
@Override
void close();
static AddressAcquisition create(Buffer buffer, Runnable closeActions) {
return new AddressAcquisitionImpl(buffer, closeActions);
}
final class AddressAcquisitionImpl implements AddressAcquisition {
private final DirectBuffer directBuffer;
private final Runnable closeAction;
public AddressAcquisitionImpl(Buffer buffer,
Runnable closeAction) {
this.directBuffer = (DirectBuffer) buffer;
this.closeAction = closeAction;
}
@Override
public long address() {
return directBuffer.address();
}
@Override
public void close() {
try {
closeAction.run();
} finally {
Reference.reachabilityFence(directBuffer);
}
}
}
}
(*) In reality, a representation of the object might be allocated on the stack
instead.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Rework Acquisition
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/88ed3aa2..0526e3dc
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=08
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=07-08
Stats: 249 lines in 19 files changed: 66 ins; 91 del; 92 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]
On Mon, 21 Nov 2022 20:06:20 GMT, Alan Bateman wrote: >>> > Although this looks much simpler and concise, it means "a new object is >>> > created for each invocation" >>> >>> My comment was actually to see if DirectBuffer could extend AutoCloseable >>> so that the acquire returns "this" for the non-session case. Doing the >>> equivalent for the session case might leak into MemorySessionImpl >>> implementing DirectBuffer which you probably don't want to do. If >>> NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would >>> at least improve some of the use-sites. >> >> Not sure that is simpler. ByteBuffer <: AutoCloseable doesn't seem to make >> sense to me. I'm also not sure how much object allocation (all this stuff >> will become value types) should be the driving factor in these code paths. > > Right, I didn't mean BB <: AC but to see if we avoid needing to wrap it > because this PR is touching several low level and performance critical code > paths. For the faraway places then having the close do a > Reference.reachabilityFence(this) should avoid the surprise that the buffer > has to kept alive even though it appears that the try-with-resources is doing > it already. I have reworked Acquisition. I think we could merge the old and new way in a separate PR. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:23:40 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rework Acquisition
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 914:
>
>> 912: * If so, make a copy to put the dst data in.
>> 913: */
>> 914: @SuppressWarnings("try")
>
> After looking at the implementation some more, I'm not sure this need fixing?
> E.g. this method is just using the address to compute some overlap - and
> return a buffer sliced accordingly. There's no access to the buffer data
> (except for the last part which does a `put`). The access will fail if the
> session is closed from underneath. I don't think this can crash the VM (in
> fact this code did not have a reachability fence to begin with).
Well spotted. I will remove the guarding here.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v10]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Remove redundant guard and fix comment permanently
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/0526e3dc..06c764ca
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=09
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=08-09
Stats: 59 lines in 2 files changed: 19 ins; 26 del; 14 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v11]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Refactor to try-finally handling
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/06c764ca..88ae68b8
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=10
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=09-10
Stats: 677 lines in 20 files changed: 286 ins; 152 del; 239 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v12]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Cleanup
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/88ae68b8..682b6f5a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=11
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=10-11
Stats: 23 lines in 9 files changed: 1 ins; 1 del; 21 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v12]
On Wed, 23 Nov 2022 16:14:52 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Cleanup
>
> src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 125:
>
>> 123: * @see #releaseScope(Buffer, MemorySessionImpl)
>> 124: */
>> 125: MemorySessionImpl acquireScopeOrNull(Buffer buffer);
>
> I think this looks better - but naming-wise it's still a bit problematic.
> This method really acquires the underlying session, not a scope. And also,
> here we have `OrNull`, but the already existing `acquireSession` also has a
> similar semantics w.r.t. null.
>
> I suggest to rename:
>
> * `acquireSession` -> `acquireSessionAsRunnable`
> * `acquireScopeOrNull` -> `acquireSession`
> * `releaseScope` -> `releaseSession`
>
> Also, once we have `acquire/releaseSession`, it is not clear to me that we
> still need `acquireSessionAsRunnable` in the JavaNIOAccess class - it seems
> like you can create the Runnable where it's required (probably IOUtil),
> simply by using straight acquire/release.
The name "scope" was used in anticipation of the new proposed Java 20 naming.
But I can change it back to session again. We could always rename later.
> src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 164:
>
>> 162: int pageSize();
>> 163:
>> 164: sealed interface ScopeAcquisition extends AutoCloseable {
>
> Is this still needed?
Well spotted.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Remove redundant method
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/682b6f5a..0546b23b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=12
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=11-12
Stats: 213 lines in 20 files changed: 12 ins; 105 del; 96 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
On Thu, 24 Nov 2022 11:58:12 GMT, Maurizio Cimadamore
wrote:
>> Separately, also (optional) stylistic: the indenting on this and following
>> class is a bit odd. There is a certain tendency to compress lines - e.g. to
>> reach for one-liners, when in reality the body is not that trivial. If I
>> were to write the code I'd insert a newline after the `{` and I'd also break
>> apart initialization (e.g. no two statements separated by `;` in the same
>> line).
>>
>> Also, I noted that you start the body of the record (e.g. `{`) on a new
>> line, which I also find odd.
>
> (to be clear, some of the above comments refer to the code that was already
> there before your changes)
I also found the above as a bit odd but tried to stick with the existing
implementation and style as much as possible.
-
PR: https://git.openjdk.org/jdk/pull/11260
