On Fri, 6 Jun 2025 14:37:40 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. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Request alignment when reallocating segment Code looks good to me, just some minor nitpicks. Note that I'm not an expert when it comes to FFM, but everything looks very reasonable to me. Will do some more tests with some applications. modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 53: > 51: arena = Arena.ofConfined(); > 52: > 53: // note: may throw OOME: This note is probably outdated? You removed it at least below, so probably can do here as well? modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 69: > 67: /** > 68: * Gets the length of this array. > 69: * Minor: The javadoc below has no empty line between the description and the return, so maybe remove here as well? modules/javafx.graphics/src/main/java/com/sun/marlin/OffHeapArray.java line 94: > 92: /** > 93: * Increments the number of bytes currently being used. > 94: * Curr used + incr used must be <= length Typo? Can be named to: `Current used + increment must be <= length` maybe. ------------- PR Review: https://git.openjdk.org/jfx/pull/1814#pullrequestreview-2908451282 PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2134678482 PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2134678600 PR Review Comment: https://git.openjdk.org/jfx/pull/1814#discussion_r2134678845
