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

[dean.long at oracle.com]

This looks fine, but why not shorten those names like we do for bcp(), 
bci(), etc?

dl

On 5/1/17 5:09 PM, Ioi Lam wrote:
> Hi Dean,
>
> I've wrapped those 4 methods. I also added 'const' to the wrapper 
> methods where possible. Here's the delta from the last posted webrev:
>
> http://cr.openjdk.java.net/~iklam/jdk10/8179305-avoid-last-frame.v02.delta/
>
> Thanks
> - Ioi
>
> On 5/1/17 3:58 PM, dean.long at oracle.com wrote:
>>
>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>