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

[dean.long at oracle.com]

Perfectly reasonable.  Thanks.

dl


On 5/1/17 6: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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>