RFR(M): 8145221: Use trampolines for i2i and i2c entries in Methods that are stored in CDS archive
Jiangli Zhou
jiangli.zhou at oracle.com
Fri Apr 8 17:55:43 UTC 2016
> On Apr 8, 2016, at 10:29 AM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
>
>
>
> On 4/7/16, 7:54 PM, Jiangli Zhou wrote:
>>
>> Hi Calvin,
>>
>> Sorry for the delay. Thank you for updating the changes. The update looks good.
> Thanks!
>>
>> The following comment and code in sharedRuntime.cpp are a little confusing. Could you please clarify that?
>> 2563 // handle the case when compiler is disabled during dump time
>> 2564 if (!DumpSharedSpaces && CodeCacheExtensions::skip_compiler_support()) {
> I will update the comment to the following:
> - // handle the case when compiler is disabled during dump time
> + // during dump time, always generate adapters, even if the
> + // compiler has been turned off.
Looks good.
Thanks,
Jiangli
>
> Calvin
>> Thanks,
>> Jiangli
>>> On Mar 31, 2016, at 9:41 PM, Calvin Cheung <calvin.cheung at oracle.com <mailto:calvin.cheung at oracle.com>> wrote:
>>>
>>> Hi Jiangli,
>>>
>>> I've updated the fix which should address all your comments.
>>> webrev:
>>> http://cr.openjdk.java.net/~ccheung/8145221/webrev.02/ <http://cr.openjdk.java.net/%7Eccheung/8145221/webrev.02/>
>>> changes between v.02 and v.01:
>>> http://cr.openjdk.java.net/~ccheung/8145221/webrev.01-02/ <http://cr.openjdk.java.net/%7Eccheung/8145221/webrev.01-02/>
>>>
>>> The trampoline code buffer is now put into the 'md' region which is made executable.
>>> I've file the following RFE to have a separate region for the code buffer:
>>> JDK-8153241: Add a read-write-executable shared region for runtime generated code
>>>
>>> thanks,
>>> Calvin
>>>
>>> On 3/17/16, 9:37 PM, Jiangli Zhou wrote:
>>>>
>>>> Hi Calvin,
>>>>
>>>> The changes look good overall. Following are my comments and questions.
>>>>
>>>> - src/os_cpu/bsd_x86/vm/thread_bsd_x86.cpp
>>>> - src/os_cpu/linux_aarch64/vm/thread_linux_aarch64.cpp
>>>> - src/os_cpu/linux_sparc/vm/thread_linux_sparc.cpp
>>>> - src/os_cpu/linux_x86/vm/thread_linux_x86.cpp
>>>> - src/os_cpu/solaris_sparc/vm/thread_solaris_sparc.cpp
>>>> - src/os_cpu/solaris_x86/vm/thread_solaris_x86.cpp
>>>> - src/os_cpu/windows_x86/vm/thread_windows_x86.cpp
>>>> Please place the new CDS-only code under #if INCLUDE_CDS.
>>>>
>>>> It would be a good idea to assert the address (current pc) is within the generated trampoline entry buffer.
>>>>
>>>> - src/share/vm/interpreter/abstractInterpreter.cpp
>>>> It’s not necessary to put '#include for memory/metaspaceShared.hpp' under #if INCLUDE_CDS.
>>>>
>>>> - src/share/vm/memory/metaspaceShared.cpp
>>>> I see you changed ‘read_only’ to false for ‘mc’ region. Is it possible to place the trampoline code buffer in the ‘md’ region, so the ‘mc’ region does not need to be writable? It’s good to keep the ‘mc’ as read only so it would be easier to detect any ‘accidental’ writes to the region due to future changes. The ‘md’ can be made executable.
>>>>
>>>> - src/share/vm/oops/method.cpp
>>>> Method::unlink_method() is only called from Method::remove_unshareable_info(), which happens at dump time only. Why is ‘if (!DumpSharedSpaces)’ check needed in Method::unlink_method()?
>>>>
>>>> - src/share/vm/oops/method.hpp
>>>> In set_interpreter_entry(), when _i2i_entry and _from_interpreted_entry are not the same as ‘entry’ for shared method?
>>>>
>>>> - src/share/vm/runtime/sharedRuntime.cpp
>>>> Please add a comment for the change at line 2537
>>>> 2537 if (!DumpSharedSpaces && CodeCacheExtensions::skip_compiler_support()) {
>>>> - src/share/vm/oops/constMethod.hpp
>>>>
>>>> ConstMethod does not seem to be the right place for ‘_adapter_trampoline’ since it’s set at runtime.
>>>>
>>>> Do you have estimate about the size increase with the generated trampolines for the shared methods?
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>> On Mar 16, 2016, at 10:36 PM, Calvin Cheung <calvin.cheung at oracle.com <mailto:calvin.cheung at oracle.com>> wrote:
>>>>>
>>>>> Dean, Ioi,
>>>>>
>>>>> Thanks for the review and discussions.
>>>>>
>>>>> Here's an updated webrev:
>>>>> http://cr.openjdk.java.net/~ccheung/8145221/webrev.01/ <http://cr.openjdk.java.net/%7Eccheung/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 <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 <https://bugs.openjdk.java.net/browse/JDK-8145221>
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8145221/webrev.00/ <http://cr.openjdk.java.net/%7Eccheung/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 <mailto:ioi.lam at oracle.com>).
>>>>>>>>>> I helped porting it to other platforms.
>>>>>>>>>> Special thanks to Goetz (goetz.lindenmaier at sap.com <mailto: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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
More information about the hotspot-runtime-dev
mailing list