RFR[s] 8179305 - Avoid repeated calls to JavaThread::last_frame in InterpreterRuntime

[Ioi Lam]

On 5/1/17 2:03 PM, dean.long at oracle.com wrote:
>
> I wasn't complaining about those, because you don't really need a 
> LastFrameAccessor there.  I was really talking about where you have a 
> LastFrameAccessor but need to escape back to frame using get_frame() 
> because you didn't add a new accessor method:
>
>
>   950       for( BasicObjectLock *kptr = last_frame.get_frame().interpreter_frame_monitor_end();
>   951            kptr < last_frame.get_frame().interpreter_frame_monitor_begin();
>   952            kptr = last_frame.get_frame().next_monitor_in_interpreter_frame(kptr) ) {
>
>  versus something like 
> last_frame.monitor_end()/monitor_begin()/next_monitor().
>
I could add those accessor methods, but frame has lots of methods, and I 
don't want to wrap each of them in case someone uses a new function in 
the future.

An alternative (which I really don't like) is to override the "->" or 
"()" operators like the Handle class:

     last_frame->interpreter_frame_monitor_begin();
or
     last_frame().interpreter_frame_monitor_begin();

Thanks
- Ioi
>
> dl
>
>
> On 5/1/17 12:54 PM, Ioi Lam wrote:
>> Hi Dean,
>>
>> Thanks for the review. I left a few thread->last_frame() that were 
>> inside ifdef ASSERT, but you're right, I should change those as well 
>> to be consistent:
>>
>> diff -r dbee2fa1d3df src/share/vm/interpreter/interpreterRuntime.cpp
>> --- a/src/share/vm/interpreter/interpreterRuntime.cpp Mon May 01 
>> 11:16:01 2017 -0700
>> +++ b/src/share/vm/interpreter/interpreterRuntime.cpp Mon May 01 
>> 12:30:01 2017 -0700
>> @@ -642,7 +642,7 @@
>>  IRT_ENTRY_NO_ASYNC(void, 
>> InterpreterRuntime::monitorenter(JavaThread* thread, BasicObjectLock* 
>> elem))
>>  #ifdef ASSERT
>> - thread->last_frame().interpreter_frame_verify_monitor(elem);
>> + 
>> LastFrameAccessor(thread).get_frame().interpreter_frame_verify_monitor(elem);
>>  #endif
>>
>> @@ -659,7 +659,7 @@
>>  #ifdef ASSERT
>> - thread->last_frame().interpreter_frame_verify_monitor(elem);
>> + 
>> LastFrameAccessor(thread).get_frame().interpreter_frame_verify_monitor(elem);
>>  #endif
>>  IRT_END
>>
>> @@ -667,7 +667,7 @@
>>  IRT_ENTRY_NO_ASYNC(void, InterpreterRuntime::monitorexit(JavaThread* 
>> thread, BasicObjectLock* elem))
>>  #ifdef ASSERT
>> - thread->last_frame().interpreter_frame_verify_monitor(elem);
>> + 
>> LastFrameAccessor(thread).get_frame().interpreter_frame_verify_monitor(elem);
>>  #endif
>>
>> @@ -680,7 +680,7 @@
>>  #ifdef ASSERT
>> - thread->last_frame().interpreter_frame_verify_monitor(elem);
>> + 
>> LastFrameAccessor(thread).get_frame().interpreter_frame_verify_monitor(elem);
>>  #endif
>>  IRT_END
>>
>> What do you think?
>>
>> Thanks
>> - Ioi
>>
>>
>>
>> On 5/1/17 12:14 PM, dean.long at oracle.com wrote:
>>> Looks OK to me, but it seems arbitrary now which calls can use 
>>> LastFrameAccessor methods and which need to go through the 
>>> get_frame() back-door.
>>>
>>> dl
>>>
>>>
>>> On 4/28/17 4:06 AM, Ioi Lam wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8179305
>>>> http://cr.openjdk.java.net/~iklam/jdk10/8179305-avoid-last-frame.v01/
>>>>
>>>> Summary:
>>>>
>>>> JavaThread::last_frame() is an expensive call. We use the helper 
>>>> class LastFrameAccessor holds the value of last_frame:
>>>>
>>>> OLD:
>>>>   methodHandle m (thread, method(thread));// calls 
>>>> JavaThread::last_frame() internally
>>>>   Bytecode_loadconstant ldc(m, bci(thread));// calls 
>>>> JavaThread::last_frame() internally
>>>>
>>>> NEW:
>>>>   LastFrameAccessor last_frame(thread);
>>>>   methodHandle m (thread, last_frame.method());
>>>>   Bytecode_loadconstant ldc(m, last_frame.bci());
>>>>
>>>> Testing:
>>>>
>>>> Preliminary benchmarking shows significant VM start-up improvement:
>>>>
>>>> java -version:
>>>> no CDS = 80.35 ms -> 79.14ms (-1.5%)
>>>> w/ CDS = 53.89ms -> 52.62ms (-2.36%)
>>>>
>>>> clojure sample app:
>>>> no CDS = 1442.46ms -> 1436.11ms (-0.44%)
>>>> w/ CDS = 695.78ms -> 679.52ms (-2.34%)
>>>>
>>>> Thanks
>>>> - Ioi
>>>
>>
>