[jdk16] RFR: 8258242: Type profile pollution occurs when memory segments of different kinds are used

[Maurizio Cimadamore]

On Mon, 14 Dec 2020 15:48:07 GMT, Claes Redestad <redestad at openjdk.org> 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