On Wed, 21 May 2025 14:07:28 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.
>
> 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

Reply via email to