RFR: 8344332: (bf) Migrate DirectByteBuffer to use java.lang.ref.Cleaner [v3]

[Alan Bateman]

On Mon, 20 Jan 2025 16:48:49 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> DirectByteBuffers are still using old `jdk.internal.ref.Cleaner` implementation. That implementation carries a doubly-linked list, and so makes DBB suffer from the same issue fixed for generic `java.lang.ref.Cleaner` users with [JDK-8343704](https://bugs.openjdk.org/browse/JDK-8343704). See the bug for the reproducer.
>> 
>> We can migrate DBBs to use `java.lang.ref.Cleaner`.
>> 
>> There are two pecularities during this rewrite.
>> 
>> First, the old ad-hoc `Cleaner` implementation used to exit the VM when cleaning action failed. I presume it was to avoid memory leak / accidental reuse of the buffer. I moved the relevant block to `Deallocator` directly. Unfortunately, I cannot easily test it.
>> 
>> Second is quite a bit hairy. Old DBB cleaning code was hooked straight into `Reference` processing loop. This was possible because we could infer that the weak references we are processing were DBB cleaning actions, since old `Cleaner` was the only use of this code. With standard `Cleaner`, we have lost this association, and so we cannot really do this from the reference processing loop. With the patched version, we now rely on normal `Cleaner` thread to do cleanups for us. Because of this, there is a new outpacing opportunity window where reference processing might have been over, but cleaner thread has not reacted yet.
>> 
>> Tests show that DirectBufferAlloc tests are still surviving this, possibly due to exponential sleep-backoff already built in. See the reclamation path in `Bits.unreserveMemory`: https://github.com/openjdk/jdk/blob/c207cc7e705d3f449f2387324d86cfb31ce40c44/src/java.base/share/classes/java/nio/Bits.java#L106-L186
>> 
>> Additional testing:
>>  - [x] Linux x86_64 server fastdebug, `java/nio java/io`
>>  - [x] Linux AArch64 server fastdebug, `java/nio java/io`
>>  - [ ] Linux x86_64 server fastdebug, `all`
>>  - [ ] Linux AArch64 server fastdebug, `all`
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Visibility and whitespace

No objection to move this to use jl.ref.Cleaner. Note that the long standing recommendation has always been to cache/reuse direct buffers rather than discard like the reproducer does. Now that the FFM API is permanent then we should redirect developers looking for deterministic release to use the new API and they can get a BB view if needed.

I assume you'll update the copyright header before this change is integrated.

src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template line 86:

> 84:                 Bits.unreserveMemory(size, capacity);
> 85:             } catch (Throwable x) {
> 86:                 // Old jdk.internal.ref.Cleaner behavior: when it fails, VM exits.

I'd prefer not reference a class that no longer exists, instead just say that it preserves long standing behavior exit the VM. In the case of DBB, I can't think what exceptions/errors are possible here.

src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template line 100:

> 98:         private static final Cleaner CLEANER = Cleaner.create();
> 99: 
> 100:         public static Cleanable register(Object buffer, Runnable action) {

Why is this public?

-------------

PR Review: https://git.openjdk.org/jdk/pull/22165#pullrequestreview-2562893777
PR Review Comment: https://git.openjdk.org/jdk/pull/22165#discussion_r1922695881
PR Review Comment: https://git.openjdk.org/jdk/pull/22165#discussion_r1922696432