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 [v4]
On Mon, 21 Nov 2022 17:00:52 GMT, Maurizio Cimadamore 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. > >> > 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. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]
On Mon, 21 Nov 2022 16:34:46 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. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]
On Mon, 21 Nov 2022 16:08:20 GMT, Per Minborg 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. - 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 [v4]
On Mon, 21 Nov 2022 13:02:48 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 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?
-
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:43:56 GMT, Per Minborg wrote:
>> 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?
This change was missed in commit
.
-
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 [v4]
On Mon, 21 Nov 2022 13:02:48 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 one additional
> commit since the last revision:
>
> Rename methods
src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 94:
> 92: * required to guarantee safety.
> 93: * {@snippet lang = java:
> 94: * var handler = acquireSessionOrNoOp(buffer);
Suggestion:
* var handler = acquireSessionOrNull(buffer, async);
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)) {
-
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
