On Thu, 5 Jun 2025 19:05:34 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> This PR builds on a concept John Rose told me about some time ago. Instead
>> of combining memory operations of various sizes, a single large and skewed
>> memory operation can be made to clean up the tail of remaining bytes.
>>
>> This has the effect of simplifying and shortening the code. The number of
>> branches to evaluate is reduced.
>>
>> It should be noted that the performance of the fill operation affects the
>> allocation of new segments (as they are zeroed out before being returned to
>> the client code).
>>
>> This PR passes tier1, tier2, and tier3 on multiple platforms.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use a fixed threashold for fill
src/java.base/share/classes/jdk/internal/foreign/SegmentBulkOperations.java
line 55:
> 53:
> 54: // All the threshold values below MUST be a power of two and greater
> or equal to 2^3.
> 55: private static final int NATIVE_THRESHOLD_FILL = 32;
Is there any reason you removed this property? While I understand that
performance can regress for bigger sizes, taking away the property seems
harsh/asymmetric?
test/micro/org/openjdk/bench/java/lang/foreign/SegmentBulkFill.java line 104:
> 102: @Benchmark
> 103: public void heapSegmentFillUnsafe() {
> 104: SCOPED_MEMORY_ACCESS.setMemory(heapSegment.sessionImpl(),
> heapSegment.unsafeGetBase(), heapSegment.unsafeGetOffset(),
> heapSegment.byteSize(), (byte) 0);
I think I would prefer just using Unsafe here. You can do that by saving the
segment address in a `long` non-final field, and then use that field as the
unsafe offset for the unsafe access operation. No need to use internal routines
and/or ScopedMemoryAccess. I understand that, perhaps, using ScopedMemoryAccess
allows you to include the cost of the liveness check, but I think using plain
Unsafe as a reference is what this (and other similar benchmarks) should be
doing.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25383#discussion_r2138344248
PR Review Comment: https://git.openjdk.org/jdk/pull/25383#discussion_r2138343109
