Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]
On Mon, 21 Nov 2022 15:29:11 GMT, Per Minborg wrote: > That is clear to me but I am trying to prevent future redundant guarding. > Anyway, I will remove the comments. The faraway use sites look much better now. The performance sensitive usages need close attention. Can you summarise the changes to DatagramChannel, are you creating a NoOpScopeAcquisition for every send/receive? This is low level code where we do do something different to avoid the try-with-resources. - 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 [v6]
On Mon, 21 Nov 2022 14:02:38 GMT, Per Minborg 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. > > 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. - 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