RFR(M): 8145221: Use trampolines for i2i and i2c entries in Methods that are stored in CDS archive
Calvin Cheung
calvin.cheung at oracle.com
Fri Apr 1 04:41:52 UTC 2016
Hi Jiangli,
I've updated the fix which should address all your comments.
webrev:
http://cr.openjdk.java.net/~ccheung/8145221/webrev.02/
changes between v.02 and v.01:
http://cr.openjdk.java.net/~ccheung/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 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
>>>>>>
>>>>>
>>>>
>>>
>
More information about the hotspot-runtime-dev
mailing list