RFR[s] 8179305 - Avoid repeated calls to JavaThread::last_frame in InterpreterRuntime
Ioi Lam
ioi.lam at oracle.com
Tue May 2 01:26:23 UTC 2017
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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list