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

[Jiangli Zhou]

> On Mar 21, 2016, at 9:40 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> 
> 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())) {

Ok.

BTW, one of my comments in first email suggested to not map the ‘mc’ space RW. We should either allocate the trampoline buffer in the ‘md’ space. Another solution would be to add a new read-write-executable space for the trampoline code. So the above check would check if the 'add.pc()' is in the ‘md’ space or the new space. I’m in favor of adding a new read-write-executable space for the runtime generated code.


Thanks,
Jiangli

> 
> 
> [...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   }
> 
>