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 Mar 18 05:13:50 UTC 2016
Hi Jiangli,
Since I wrote some of the original code, I'll try to answer some of the
questions below
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.
I am not sure what this means. Could you explain?
>
> - 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()?
We can change the code to
assert(DumpSharedSpaces, "...");
// Set the values to what they should be at run time. Note that
// this Method can no longer be executed during dump time.
_i2i_entry = Interpreter::entry_for_cds_method(this);
_from_interpreted_entry = _i2i_entry;
I think unlink_method used to be called during run time as well. See
this assert later in unlink_method
// In case of DumpSharedSpaces, _method_data should always be NULL.
//
// During runtime (!DumpSharedSpaces), when we are cleaning a
// shared class that failed to load, this->link_method() may
// have already been called (before an exception happened), so
// this->_method_data may not be NULL.
assert(!DumpSharedSpaces || _method_data == NULL, "unexpected method
data?");
So we need to remove the "!DumpSharedSpaces ||" and fix the comments.
> - 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?
The purpose of the check is to avoid writing the same value into the
shared Method (which would cause the pages to be copied-on-write, even
if the value does not change).
475 void set_interpreter_entry(address entry) {
476 if (_i2i_entry != entry) {
477 _i2i_entry = entry;
478 }
479 if (_from_interpreted_entry != entry) {
480 _from_interpreted_entry = entry;
481 }
482 }
> - 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.
The pointer ConstMethod::_adapter_trampoline never changes at run time.
Only the content pointed to by _adapter_trampoline can change.
Thanks
- Ioi
> 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> 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
More information about the hotspot-runtime-dev
mailing list