RFR: 8344332: (bf) Migrate DirectByteBuffer to use java.lang.ref.Cleaner [v3]
Uwe Schindler
uschindler at openjdk.org
Mon Jan 20 18:06:35 UTC 2025
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
Marked as reviewed by uschindler (Author).
This looks fine for me. Older Lucene versions won't break, as they use `sun.misc.Unsafe#invokeCleaner()` which behaves as before.
Code that deep reflected and made `DirectByteBuffer#cleaner()` accessible was broken for long time anyways (since Java 9), so the new names and class types should not affect uptodate code.
I'd remove the `catch (Throwable)` => exit VM code completely. The called methods have no checked exceptions. If the deallocation fails for some (internal) reason it its already broken and a crush will come sooner or later anyways.
-------------
PR Review: https://git.openjdk.org/jdk/pull/22165#pullrequestreview-2562957601
PR Comment: https://git.openjdk.org/jdk/pull/22165#issuecomment-2603003680
More information about the core-libs-dev
mailing list