Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]
On Mon, 28 Nov 2022 19:35:24 GMT, Paul Sandoz wrote: > I prefer methods that do not expose the scope implementation so access is > limited to just to the acquire/release methods, but i am unsure of the > performance implications. These methods might not reliably inline, and we may > need to ensure that first (which is also separately a good thing). I think it > requires that the shared secret fields are stable and that there is only one > implementation of `JavaNioAccess`, which there is, but we can enforce via > sealing. Something to consider as a further iteration. I agree. A similar approach was done in a previous incarnation of the PR whereby we exposed a `ScopeAcqusition` that held an internal copy of the buffer and provided means to release. We feared that would cause performance regression. But your comment made me think of another way of doing acquire/release which would not expose the session and that would always be performant. I will explore this idea a bit more. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]
On Mon, 28 Nov 2022 10:47:47 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~~ *try-finally* 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~~ TF is using a ~~"session acquisition" that is not
>> used explicitly in the try block itself~~ session used in the *finally*
>> block. ~~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 `_`.~~
>>
>> 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:
>
> Re-introduce yet another address vairable
The approach looks good, and almost the least intrusive (see comment).
src/java.base/share/classes/java/nio/Buffer.java line 838:
> 836:
> 837: @Override
> 838: public void releaseSession(Buffer buffer,
> MemorySessionImpl scope) {
I prefer methods that do not expose the scope implementation so access is
limited to just to the acquire/release methods, but i am unsure of the
performance implications. These methods might not reliably inline, and we may
need to ensure that first (which is also separately a good thing). I think it
requires that the shared secret fields are stable and that there is only one
implementation of `JavaNioAccess`, which there is, but we can enforce via
sealing. Something to consider as a further iteration.
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]
On Mon, 21 Nov 2022 12:05:31 GMT, Alan Bateman wrote: >> 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. > > I'd prefer to see this comment removed from all places that are obviously > interacting with the direct buffer cache. These usages are try-finally to > acquire and return the temporary direct buffer cache back to the cache. > Talking about closable sessions here is definitely confusing. > Thanks for persisting with it, latest version looks okay to me and the naming > issue can be sorted out after the JEP is integrated. IMHO the renaming is not super important - the underlying abstraction managing the segment lifetime is still called MemorySession, even after https://git.openjdk.org/jdk/pull/10872. And, acquire/release are methods on MemorySession - so I think the current name might be more precise even after we integrate the latest API changes. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]
On Mon, 28 Nov 2022 10:47:47 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:
>
> Re-introduce yet another address vairable
Thanks for persisting with it, latest version looks okay to me and the naming
issue can be sorted out after the JEP is integrated.
-
Marked as reviewed by alanb (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]
> 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:
Re-introduce yet another address vairable
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/95c40581..eedf41ae
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=11260=15
- incr: https://webrevs.openjdk.org/?repo=jdk=11260=14-15
Stats: 2 lines in 1 file changed: 1 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
