RFR(M): 8145221: Use trampolines for i2i and i2c entries in Methods that are stored in CDS archive
Ioi Lam
ioi.lam at oracle.com
Fri Apr 1 05:45:41 UTC 2016
Oops, sorry, I missed that. Thanks for clarifying.
- Ioi
On 3/31/16 10:43 PM, Calvin Cheung wrote:
> Hi Ioi,
>
> On 3/31/16, 10:09 PM, Ioi Lam wrote:
>> Hi Calvin,
>>
>> The new changes look good. I just have one comment:
>>
>> 36 #if INCLUDE_CDS
>> 37 #include "memory/metaspaceShared.hpp"
>> 38 #endif
> Did you mean the above code in abstractInterpreter.cpp?
> I removed it in webrev.02:
> http://cr.openjdk.java.net/~ccheung/8145221/webrev.01-02/src/share/vm/interpreter/abstractInterpreter.cpp.sdiff.html
>
>
> thanks,
> Calvin
>
>>
>> I think this is unnecessary and adds to the clutter. All other files
>> that include this file do not have such an #if macro. E.g.,
>> universe.cpp and allocation.cpp.
>>
>> Thanks
>> - Ioi
>>
>>
>> On 3/31/16 9:41 PM, Calvin Cheung wrote:
>>> 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