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

[dean.long at oracle.com]

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().

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
>>
>