On Mon, 9 Jun 2025 15:25:22 GMT, Andy Goryachev <ango...@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
