On Wed, 21 May 2025 14:07:28 GMT, Kevin Rushforth <[email protected]> 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.
>
> 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?
It may depend on hardward arch ???
> 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.
Yes 4-bytes min or more like 16 if it helps the compiler to use avx
instructions for fill / copy ?
> 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?
True.
> 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?
Yes. (I skipped that code as edges were left dirty until the next init() call
to reset it unconditionally, resize was just a realloc to trim array)
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2122065109
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2122082408
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2105901398
PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2122053941
