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

[dean.long at oracle.com]

I was expecting you to wrap just the methods that InterpreterRuntime uses.

dl


On 5/1/17 2:18 PM, Ioi Lam wrote:
>
>
> 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
>>>>
>>>
>>
>