RFR(M): 8145221: Use trampolines for i2i and i2c entries in Methods that are stored in CDS archive

[Ioi Lam]

On 3/18/16 6:43 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
>> On Mar 18, 2016, at 4:04 PM, Ioi Lam <ioi.lam at oracle.com 
>> <mailto:ioi.lam at oracle.com>> wrote:
>>
>>
>>
>> On 3/18/16 11:52 AM, Jiangli Zhou wrote:
>>>
>>>>>
>>>>> 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?
>>>
>>>
>>> We should assert that the address is in the generated trampoline 
>>> code, but not at a random place in the shared space. If the control 
>>> dumps to an arbitrary location, it might manifest itself into a bug 
>>> that’s hard to debug.
>>
>> I still don't understand where you want to put the assertion. Could 
>> you tell me which file/function you want to add the assert, and what 
>> should be put in the assert?
>
>
> For example, assert the pc() is in trampoline code if we get an 
> address in the shared space in the following code in 
> JavaThread::pd_get_top_frame().
>
>    69     if (UseSharedSpaces && MetaspaceShared::is_in_shared_space(addr.pc())) {
>    70       // In the middle of a trampoline call. Bail out for safety.
>    71       // This happens rarely so shouldn't affect profiling.
>    72       return false;
>    73     }
> Something like the following?
>
> assert(addr.pc() >= MetaspaceShared::cds_i2i_entry_code_buffers() &&
>            addr.pc() < MetaspaceShared::cds_i2i_entry_code_buffers() + 
> MetaspaceShared::cds_i2i_entry_code_buffer_size(),
>            “Unexpected");
>
The address range you listed is just part of the trampolines (for the 
"standard" method entries). More trampolines can be allocated here -- 
these are entries that are associated with specific method signatures.

3021 void CDSAdapterHandlerEntry::init() {
3022   assert(DumpSharedSpaces, "used during dump time only");
3023   _c2i_entry_trampoline = 
(address)MetaspaceShared::misc_code_space_alloc(SharedRuntime::trampoline_size());
3024   _adapter_trampoline = 
(AdapterHandlerEntry**)MetaspaceShared::misc_data_space_alloc(sizeof(AdapterHandlerEntry*));
3025 };

In addition, addr.pc() can also be one of the vtable patching methods.

So, instead of adding an assert, I think it's better to narrow the check 
to something like:

      if (UseSharedSpaces && 
MetaspaceShared::is_in_shared_misc_code_space(addr.pc())) {


[...snip...]

>>
>>>>> - 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   }
>>>
>>> I see set_interpreter_entry() is called from Method initializing 
>>> code and link_method(). For a shared method, we should change the if 
>>> check to be an assert. Would there be any case where the entry is 
>>> not the same as the one in the shared method?
>>
>> I am actually not sure if for shared methods the i2i_entry and 
>> from_interpreter_entry would be set to any other values. E.g., in the 
>> future, someone may add a new tracing facility that requires 
>> modifying these 2 fields.
>>
>> I'd rather play safe so we don't overwrite into the shared Method 
>> most of the time, but if it does happen it will continue to work.
>
> This part of the code is more critical. So, we should have a tighter 
> control of it. If someone is setting the the entry to a different 
> valve than the one in the Method, we need to know what is the new 
> entry and understand why. The assertion would force us to exam each 
> new case like the one you described in the above. Is that reasonable?
>
Sounds good. I think we should add the assert so we will catch such new 
cases in non-product builds. But for safety, in product builds, we 
should set the values as requested. We don't want assumptions made by 
this optimization to affect functional correctness.

475   void set_interpreter_entry(address entry) {
         if (is_shared()) {
           assert(_i2i_entry != entry && _from_interpreted_entry != 
entry, "must be");
         }
476     if (_i2i_entry != entry) {
477       _i2i_entry = entry;
478     }
479     if (_from_interpreted_entry != entry) {
480       _from_interpreted_entry = entry;
481     }
482   }