On Thu, 25 Jul 2024 22:53:36 GMT, Phil Race <[email protected]> wrote:
> Migrate from using Unsafe to FFM's MemorySegment API for allocating and
> setting native memory.
> This code is used by Metal, OpenGL and D3D, so I manually tested SwingSet2
> and J2Demo as well as running all the usual tests.
> I also did some micro-benchmarking on the performance of Unsafe vs
> MemorySegment.
> The performance of either is more than sufficient for us .. ie they could be
> 10x slower and we wouldn't even notice.
> But they are in the same ballpark, and if one or the other is clearly faster
> it is the new FFM code.
src/java.desktop/share/classes/sun/java2d/pipe/RenderBuffer.java line 73:
> 71: private final long baseAddress;
> 72: private long curOffset;
> 73: private final int capacity;
You could perhaps drop the `capacity` field, and use `segment.byteSize()` in
it's place.
src/java.desktop/share/classes/sun/java2d/pipe/RenderBuffer.java line 175:
> 173: int offsetInBytes = offset * SIZEOF_SHORT;
> 174: int lengthInBytes = length * SIZEOF_SHORT;
> 175: MemorySegment.copy(x, offsetInBytes, segment, JAVA_SHORT,
> curOffset, length);
This doesn't look right. When copying from an array like this, the source
offset is an array index, not an offset in bytes. So I believe the second
argument here should just be `offset`.
src/java.desktop/share/classes/sun/java2d/pipe/RenderBuffer.java line 176:
> 174: int lengthInBytes = length * SIZEOF_SHORT;
> 175: MemorySegment.copy(x, offsetInBytes, segment, JAVA_SHORT,
> curOffset, length);
> 176: position(position() + lengthInBytes);
If you want, you could use `MemoryLayout::scale` here, i.e.
Suggestion:
position(JAVA_SHORT.scale(position(), length));
(up to preference though)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20339#discussion_r1692278576
PR Review Comment: https://git.openjdk.org/jdk/pull/20339#discussion_r1692274831
PR Review Comment: https://git.openjdk.org/jdk/pull/20339#discussion_r1692286004
