RFR(M): 8145221: Use trampolines for i2i and i2c entries in Methods that are stored in CDS archive

[Ioi Lam]

Hi Calvin,

The changes look good to me. Thanks for fixing the issues.

- Ioi

On 3/16/16 10:36 PM, Calvin Cheung wrote:
> Dean, Ioi,
>
> Thanks for the review and discussions.
>
> Here's an updated webrev:
> http://cr.openjdk.java.net/~ccheung/8145221/webrev.01/
>
> Changes in this webrev:
> - in thread_*.cpp, added check in JavaThread::pd_get_top_frame(). 
> Return if it is in the middle of a trampoline call.
> - moved the guarantee from metaspaceShared_sparc.cpp to the common 
> metaspaceShared.cpp;
> - changed the MIN_SHARED_MISC_DATA_SIZE and MIN_SHARED_MISC_CODE_SIZE 
> and related comment in metaspaceShared.hpp
>
> The last 2 changes are required due to the test failure with the 
> hotspot/test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java 
> test.
>
> Testing:
>     JPRT with testset hotspot
>     jtreg tests under hotspot/runtime on all platforms
>
> thanks,
> Calvin
>
> p.s. I corrected the bug id in the subject line from 8145521 to 8145221
>
>
> On 2/25/16, 10:03 AM, Ioi Lam wrote:
>>
>>
>> On 2/22/16 12:50 PM, Dean Long wrote:
>>> Hi Ioi, comments inlined below.
>>>
>>> On 2/20/2016 5:22 PM, Ioi Lam wrote:
>>>> Hi Dean,
>>>>
>>>> Thanks for raising this issue.
>>>>
>>>> My first impression is this probably shouldn't matter (or somehow 
>>>> we can get away with this).
>>>>
>>>> Today, the CDS archive already contains executable code in the 
>>>> "misc code" section. On Linux/x64, this is typically in the address 
>>>> range 0x802400000-0x802409000. The code is used to dynamically 
>>>> patch the C++ vtables of Metadata types that are stored in the CDS 
>>>> archive. So even today, there's a chance that the suspended PC 
>>>> lands in this section.
>>>>
>>>
>>> OK, so you're saying that the trampolines don't make things any 
>>> worse :-)
>>
>> Well, theoretically the i2i and c2i entries would be executed more 
>> frequently than the vtable patching code, so if there was a problem, 
>> it could be aggravated.
>>>
>>>> The code is generated inside 
>>>> MetaspaceShared::generate_vtable_methods and looks like this when 
>>>> you run with -Xshare:on:
>>>>
>>>> (gdb) x/5i 0x802400000
>>>>    0x802400000:    mov    $0x0,%eax
>>>>    0x802400005:    jmpq   0x8024084d0
>>>>    0x80240000a:    mov    $0x1,%eax
>>>>    0x80240000f:    jmpq   0x8024084d0
>>>>    0x802400014:    mov    $0x2,%eax
>>>>    ....
>>>> (gdb) x/16i 0x8024084d0
>>>>    0x8024084d0:    push   %rsi
>>>>    0x8024084d1:    push   %rdi
>>>>    0x8024084d2:    mov    %rax,%rdi
>>>>    0x8024084d5:    shr    $0x8,%rdi
>>>>    0x8024084d9:    shl    $0x3,%rdi
>>>>    0x8024084dd:    movabs $0x802000000,%rsi
>>>>    0x8024084e7:    add    %rdi,%rsi
>>>>    0x8024084ea:    mov    (%rsi),%rsi
>>>>    0x8024084ed:    pop    %rdi
>>>>    0x8024084ee:    mov    %rsi,(%rdi)
>>>>    0x8024084f1:    and    $0xff,%rax
>>>>    0x8024084f8:    shl    $0x3,%rax
>>>>    0x8024084fc:    add    %rsi,%rax
>>>>    0x8024084ff:    pop    %rsi
>>>>    0x802408500:    mov    (%rax),%rax
>>>>    0x802408503:    jmpq   *%rax
>>>>
>>>> In JDK9, the profiler takes a sample by first calling into 
>>>> JavaThread::pd_get_top_frame_for_profiling:
>>>>
>>>> (gdb) where
>>>> #0  frame::safe_for_sender (this=0x7ffec63895f0, 
>>>> thread=0x7ffff08ce000)
>>>> #1  0x00007ffff69d72a7 in JavaThread::pd_get_top_frame 
>>>> (this=0x7ffff08ce000, fr_addr=0x7ffec63897b0, 
>>>> ucontext=0x7ffec6287d00, isInJava=true)
>>>> #2  0x00007ffff69d70be in 
>>>> JavaThread::pd_get_top_frame_for_profiling (this=0x7ffff08ce000, 
>>>> fr_addr=0x7ffec63897b0, ucontext=0x7ffec6287d00,
>>>>     isInJava=true)
>>>>
>>>> I tried manually setting frame::_pc to 0x802400000, and it would 
>>>> cause frame::safe_for_sender() to return false, and thus would 
>>>> prevent the profiler from doing any stack walking.
>>>>
>>>
>>> Was the reason safe_for_sender() failed something that we can rely 
>>> on 100%?  I looked at safe_for_sender, and it's not obvious why it 
>>> would fail, unless the frame pointer was invalid, and I think that 
>>> depends on the platform and flags like PreserveFramePointer.  How 
>>> about filing a separate bug to track this issue?
>>>
>>
>> OK, I fill a new bug https://bugs.openjdk.java.net/browse/JDK-8150663 
>> to track this.
>>
>> Thanks
>> - Ioi
>>
>>> dl
>>>
>>>> Also, both the CDS vtable patching code and the i2i trampolines are 
>>>> tail calls, so you will never find a PC in them except for the 
>>>> top-most frame. This means the check in 
>>>> JavaThread::pd_get_top_frame is enough. There's no need to do 
>>>> additional checks after we have started stack walking.
>>>>
>>>> I think the chance of landing in the CDS vtable patching methods, 
>>>> or in the trampolines, is pretty small, so that shouldn't bother 
>>>> the sampling profiler too much.
>>>>
>>>> Anyway, as part of this bug, we should add a regression test with 
>>>> the profiler enabled, and keep calling a method in the CDS archive 
>>>> in a tight loop (and manually disable compilation of that method 
>>>> using command-line options). The test should never crash, and if 
>>>> possible, it should also check that we don't have too many 
>>>> "unknown" samples.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>> On 2/19/16 4:39 PM, Dean Long wrote:
>>>>> We have profiling code that will suspend a thread at random points 
>>>>> and try to walk the stack.
>>>>> I think frame::sender() will get confused if you happen to catch a 
>>>>> thread in the trampoline,
>>>>> and frame::_pc is in metadata and not the code cache.
>>>>>
>>>>> dl
>>>>>
>>>>> On 2/19/2016 2:19 PM, Calvin Cheung wrote:
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8145221
>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8145221/webrev.00/
>>>>>>
>>>>>> This optimization reduces the size of the RW region of the CDS 
>>>>>> archive. It also reduces the amount of pages in the RW region 
>>>>>> that are actually written into during runtime.
>>>>>>
>>>>>> The original prototype on the x86_64 platform was done by Ioi 
>>>>>> (ioi.lam at oracle.com).
>>>>>> I helped porting it to other platforms.
>>>>>> Special thanks to Goetz (goetz.lindenmaier at sap.com) who provided 
>>>>>> the changes to the ppc platform.
>>>>>>
>>>>>> Testing:
>>>>>>     JPRT with testset hotspot
>>>>>>     maunal testing on X86_64, x86_32 and solaris/sparcv9 with 
>>>>>> -XX:+PrintAssembly -XX:+PrintInterpreter
>>>>>>     built on the Zero platform
>>>>>>     tested on the openjdk aarch64 platform
>>>>>>     refworkload on various platform (please refer to bug report 
>>>>>> for data)
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>
>>>>
>>>
>>