On Wed, 21 May 2025 13:24:32 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> PR to replace the use of sun.misc.Unsafe memory access methods in the Marlin
> rasterizer with FFM.
>
> I broke this up into the following commits. The bulk of the work is in the
> first two:
>
> 1. Encapsulate all off-heap access in OffHeapArray -- All of the memory
> allocation and management was already done in the OffHeapArray class, but the
> users of OffHeapArray, primarily Renderer and RendererNoAA, called Unsafe
> methods directly. I encapsulated all of the calls to Unsafe in OffHeapArray.
> The main impact on calling code is that the base address is no longer
> accessible or needed. Instead, the `(put|get)(Byte|Int)` methods take only an
> offset. This commit was straight refactoring with no behavioral changes.
> 2. Initial FFM implementation -- I changed the memory management and access
> methods to use FFM. Each OffHeap array uses a shared Arena to manage the
> single memory segment allocated at construction time. The resize method
> creates a new Arena and memory segment, copying the data from the old and
> then closing it
> 3. Set `used` to 0 in `dispose()` -- While testing and instrumenting the
> code, I discovered that the Renderer dispose methods resize the edges array
> back to the default size without clearing the "used" field. The used field
> will be cleared before the next time it is accessed, but clearing it in
> dispose allows optimizing resize to not copy any data.
> 4. Remove '--sun-misc-unsafe-memory-access=allow' from test and app
> execution, since it is no longer needed. This also enables `-Werror` for the
> `javafx.graphics` module.
> 5. ~~Temporary debug prints that will be removed before making this "rfr"~~
>
> Additional commits address review comments.
@bourgesl @prrace @nlisker @minborg Can you take an initial look at this? It
isn't finished (there are a few open questions), but it is working.
modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 36:
> 34: // KCR: BEGIN DEBUG
> 35: import java.util.concurrent.atomic.AtomicInteger;
> 36: // KCR: END DEBUG
I'll revert all of the debugging code.
modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 46:
> 44: // FFM stuff
> 45: private static final ValueLayout.OfByte BYTE_LAYOUT =
> ValueLayout.JAVA_BYTE;
> 46: private static final ValueLayout.OfInt INT_LAYOUT =
> ValueLayout.JAVA_INT.withOrder(ByteOrder.BIG_ENDIAN);
@minborg Is this the best layout to use? I chose it thinking that, since we
don't need to access this memory from native code, it might perform better to
use Java's big endian byte order. Is this correct?
modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 50:
> 48: static {
> 49: // KCR FIXME: get this from FFM
> 50: SIZE_INT = 4;
I will address this.
modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 55:
> 53: /* members */
> 54: private MemorySegment segment;
> 55: // private long address;
I'll remove this (it's a leftover).
modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 78:
> 76: // note: may throw OOME:
> 77: // KCR FIXME: Set a MemoryLayout
> 78: this.segment = arena.allocate(len);
I'll choose a memory layout and also specify 4-byte alignment.
modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 145:
> 143: if (this.used > 0) {
> 144: MemorySegment.copy(segment, 0, newSegment, 0,
> Math.min(this.used, len));
> 145: }
@bourgesl I presume that there is never a need to copy more than `used` bytes?
modules/javafx.graphics/src/main/java/com/sun/marlin/Renderer.java line 650:
> 648: // KCR: Double-check this
> 649: // Clear used bytes in edges array
> 650: edges.setUsed(0);
@bourgesl I presume that clearing `used` is correct here?
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1814#issuecomment-2897972083
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2100387719
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2100395691
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2100385901
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2100386573
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2100397246
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2100399348
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2100402618
