RFR: 8334137: Marlin: replace sun.misc.Unsafe memory access methods with FFM [v5]

[Kevin Rushforth]

On Mon, 9 Jun 2025 15:25:22 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Kevin Rushforth has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review comments
>
> modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 47:
> 
>> 45:     /* members */
>> 46:     private MemorySegment segment;
>> 47:     private long length;
> 
> just curious: why `long`?  are we expecting the length to exceed 2 billion?

This is preexisting. The goal was to keep the same interface that the users of OffHeapMemory currently use, so I do not want to change or revisit this. Other than encapsulation, I made no changes to the OffHeap interface.

> modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 79:
> 
>> 77:      * @return number of used bytes
>> 78:      */
>> 79:     int getUsed() {
> 
> wait, length is `long`, but used is `int`?

I noticed this too. This is preexisting, so I don't want to change it as part of this PR (to minimize changes). It might or might not be worth a follow-up.

> modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 97:
> 
>> 95:      */
>> 96:     void incrementUsed(int increment) {
>> 97:         this.used += increment;
> 
> if used > 2B, it will overflow, right?

Yes. It seems worth checking and throw an exception. Other invariants could also be enforced such as used <= length.

I'll file a follow-up issue for this, since this behavior is preexisting.

> modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 129:
> 
>> 127: 
>> 128:     void free() {
>> 129:         arena.close();
> 
> since we don't assign arena to `null`, I assume that the code expects:
> 
> 1. `free()` will be only called once
> 2. no access to this OffHeapArray happens after `free()` is called
> 
> right?

Correct. Note that `free` is only called by the cleaner once the user of the offheap array is no longer using it (i.e., when its cleaner object is unreachable).

An exception would occur if `free` were called more than once or if there was an attempt to access the memory segment after the call to `free`.

> modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 157:
> 
>> 155:         return segment.get(INT_LAYOUT, offset);
>> 156:     }
>> 157: 
> 
> extra newline?

removed

> modules/javafx.graphics/src/main/java/com/sun/marlin/Renderer.java line 399:
> 
>> 397: 
>> 398:         final long SIZE_INT = 4L;
>> 399:         long addr   = edgePtr;
> 
> minor: extra spaces?

Preexisting. I don't want to reformat it.

> modules/javafx.graphics/src/main/java/com/sun/marlin/RendererNoAA.java line 379:
> 
>> 377:             // note: throw IOOB if neededSize > 2Gb:
>> 378:             final long edgeNewSize = ArrayCacheConst.getNewLargeSize(
>> 379:                                         _edges.getLength(),
> 
> L377 is unclear - is this a TODO to throw IOOBE or a description of what would happen?
> (this is a separate issue from FFM changes, really)

Yes, this is preexisting and not something to look at as part of this PR.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2135952740
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2135953668
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2135959177
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2136152003
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2136135909
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2135959845
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2135970787