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

[Ioi Lam]

Hi Coleen, Dean & David

Thanks you so much for the review. I have pushed the changes.

- Ioi

On 5/2/17 4:35 PM, coleen.phillimore at oracle.com wrote:
>
> This change looks really good!
> Coleen
>
>
> On 5/1/17 9:26 PM, Ioi Lam wrote:
>> OK, I'll shorten them to:
>>
>>     oop*callee_receiver*(Symbol* signature) {
>>       return _last_frame.interpreter_callee_receiver(signature);
>>     }
>>     BasicObjectLock**monitor_begin*() const {
>>       return _last_frame.interpreter_frame_monitor_end();
>>     }
>>     BasicObjectLock**monitor_end*() const {
>>       return _last_frame.interpreter_frame_monitor_begin();
>>     }
>>     BasicObjectLock**next_monitor*(BasicObjectLock* current) const {
>>       return _last_frame.next_monitor_in_interpreter_frame(current);
>>     }
>>
>> What do you think?
>> - Ioi
>>
>> On 5/1/17 6:18 PM, dean.long at oracle.com wrote:
>>>
>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>