[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR(M): 8145221: Use trampolines for i2i and i2c entries in Methods that are stored in CDS archi
From: Jiangli Zhou <jiangli.zhou () Oracle ! COM>
Date: 2016-03-21 23:39:44
Message-ID: 8FDCFA83-1844-4B39-9A3B-E7FCC0BA0A49 () oracle ! com
[Download RAW message or body]
Hi Calvin,
> On Mar 18, 2016, at 10:53 AM, Calvin Cheung <calvin.cheung@oracle.com> wrote:
>
> Hi Jiangli,
>
> Thanks for your quick review.
>
> Let me try to answer the remaining questions below.
>
> On 3/17/16, 10:13 PM, Ioi Lam wrote:
> > 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.
> I'll fix that.
> > >
> > > - 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.
> But putting code in the md region and making md region executable is confusing too.
> I'd prefer leaving it as is for now.
Ok. How about adding a new region for the trampoline code, or separating the ‘mc' \
into to ‘writable' and ‘non-writable' regions? I refer to not making the entire \
‘mc' region writable.
Thanks,
Jiangli
> > >
> > > - 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()) \
> > > {
> I believe this is for handling the case when compiler is disabled during dumping.
> I'll work with Ioi to come up with some comments.
> > > - 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?
> IIRC, the CDSAdapterHandlerEntry::init() is being called around 670 times during \
> dump time using the default classlist. So for 64-bit, the size increase for md is \
> around 50k.
> thanks,
> Calvin
> > >
> > > Thanks,
> > > Jiangli
> > >
> > > > On Mar 16, 2016, at 10:36 PM, Calvin Cheung <calvin.cheung@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@oracle.com). I helped porting it to other platforms.
> > > > > > > > > Special thanks to Goetz (goetz.lindenmaier@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
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic