On Mon, 26 Aug 2024 12:50:53 GMT, Francesco Nigro <d...@openjdk.org> wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typo
>
> src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
> line 200:
>
>> 198: switch ((int) length) {
>> 199: case 0 : checkReadOnly(false); checkValidState();
>> break; // Explicit tests
>> 200: case 1 : set(JAVA_BYTE, 0, value); break;
>
> beware using a switch, because if this code if is too big to be inlined (or
> we're unlucky) will die due to branch-mispredict in case the different "small
> fills" are unstable/unpredictable.
> Having a test which feed different fill sizes per each iteration + counting
> branch misses, will reveal if the improvement is worthy even with such cases
It is true, that this is a compromise where we give up inline space, code-cache
space, and added complexity against the prospect of better small-size
performance. Depending on the workload, this may or may not pay off. In the
(presumably common) case where we allocate/fill small segments of constant
sizes, this is likely a win. Writing a dynamic performance test sounds like a
good idea.
> test/micro/org/openjdk/bench/java/lang/foreign/TestFill.java line 86:
>
>> 84: @Benchmark
>> 85: public void buffer_fill() {
>> 86: // Hopefully, the creation of the intermediate array will be
>> optimized away.
>
> This maybe won't....why not making the byte array a static final?
The size of the array varies with the length of the array. It seams that escape
analysis works here though.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20712#discussion_r1731249923
PR Review Comment: https://git.openjdk.org/jdk/pull/20712#discussion_r1731252199
