On Mon, 14 Dec 2020 15:48:07 GMT, Claes Redestad <[email protected]> wrote:
>> This patch fixes a problem with type profile pollution when segments of
>> different kinds are used on the same memory access var handle, or on the
>> same `MemoryAccess` static method.
>>
>> In principle, argument profiling should kick in for VarHandles and
>> MethodHandles, and that should be enough at least to avoid pollution when
>> using var handles directly. In reality, this is not the case; as Vlad
>> discovered after relatively intense debugging session (thanks!), the
>> VarHandle implementation code has to cast the incoming segment to the
>> `MemorySegmentProxy` internal interface. This creates problems for C2, as
>> concrete segment implementations have _two_ interface types: `MemorySegment`
>> and the internal `MemorySegmentProxy` class. Side casts from one to the
>> other are not supported well, and can cause loss of type profiling
>> infomation.
>>
>> To solve this problem we realized, in hindisght, that `MemorySegmentProxy`
>> didn't really needed to be an interface and that it could easily be
>> converted to an abstract class. Alone this solves 50% of the issues, since
>> that makes direct var handle access robust to pollution issues. The
>> remaining problems (using accessors in `MemoryAccess` class) can be
>> addressed the usual way, by adding argument type profiling for the methods
>> in that class (similarly to what we've done for `ScopedMemoryAccess`).
>>
>> Here are some numbers before the patch:
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> LoopOverPollutedSegments.heap_segment_floats_VH avgt 30 11.535 ?
>> 0.039 ms/op
>> LoopOverPollutedSegments.heap_segment_floats_static avgt 30 10.860 ?
>> 0.162 ms/op
>> LoopOverPollutedSegments.heap_segment_ints_VH avgt 30 11.479 ?
>> 0.202 ms/op
>> LoopOverPollutedSegments.heap_segment_ints_static avgt 30 10.562 ?
>> 0.027 ms/op
>> LoopOverPollutedSegments.heap_unsafe avgt 30 0.240 ?
>> 0.003 ms/op
>> LoopOverPollutedSegments.native_segment_VH avgt 30 11.603 ?
>> 0.154 ms/op
>> LoopOverPollutedSegments.native_segment_static avgt 30 10.613 ?
>> 0.128 ms/op
>> LoopOverPollutedSegments.native_unsafe avgt 30 0.243 ?
>> 0.003 ms/op
>>
>> As you can see there is quite a big difference between unsafe access and all
>> the other modes. Here are the results after the patch:
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> LoopOverPollutedSegments.heap_segment_floats_VH avgt 30 0.244 ?
>> 0.002 ms/op
>> LoopOverPollutedSegments.heap_segment_floats_static avgt 30 0.301 ?
>> 0.001 ms/op
>> LoopOverPollutedSegments.heap_segment_ints_VH avgt 30 0.245 ?
>> 0.003 ms/op
>> LoopOverPollutedSegments.heap_segment_ints_static avgt 30 0.302 ?
>> 0.004 ms/op
>> LoopOverPollutedSegments.heap_unsafe avgt 30 0.242 ?
>> 0.003 ms/op
>> LoopOverPollutedSegments.native_segment_VH avgt 30 0.246 ?
>> 0.004 ms/op
>> LoopOverPollutedSegments.native_segment_static avgt 30 0.295 ?
>> 0.006 ms/op
>> LoopOverPollutedSegments.native_unsafe avgt 30 0.245 ?
>> 0.003 ms/op
>>
>> That is, the situation is back to normal. Thanks to @JornVernee and
>> @iwanowww for the help!
>
> src/java.base/share/classes/jdk/internal/access/foreign/MemorySegmentProxy.java
> line 32:
>
>> 30:
>> 31: /**
>> 32: * This proxy interface is required to allow instances of the {@code
>> MemorySegment} interface (which is defined inside
>
> "This abstract base class.."?
>
> I don't mind the current name, but should the class be renamed
> `AbstractMemorySegmentProxy`?
I'll fix the doc - since this is a transitional artifact (will disappear when
API is finalized an in java.base) I'd prefer to avoid the renaming
-------------
PR: https://git.openjdk.java.net/jdk16/pull/19
