Request Review: 6902182: Starting with jdwp agent should not incur performance penalty

[Tom Rodriguez]

On Dec 15, 2009, at 2:16 PM, Deneau, Tom wrote:

> So is it true that the only compiled cases where you would *not* go thru
> OptoRuntime::handle_exception_C_helper would be those where the catch
> was in the same method as the throw (or logically in the same method
> after inlining)?

That should be true.

> 
> Also, would OptoRuntime::handle_exception_C_helper still be used even
> if some of the intermediate frames are interpreted?

Every frame that the exception passes through has it's exception lookup logic consulted and interpreter and compiled frames have different copies of this logic.  OptoRuntime::handle_exception_C is used for every C2 compiled frame.

tom

> 
> -- Tom D.
> 
> 
> -----Original Message-----
> From: Thomas.Rodriguez at Sun.COM [mailto:Thomas.Rodriguez at Sun.COM] 
> Sent: Tuesday, December 15, 2009 3:30 PM
> To: Deneau, Tom
> Cc: hotspot-compiler-dev at openjdk.java.net
> Subject: Re: Request Review: 6902182: Starting with jdwp agent should not incur performance penalty
> 
> It's the core of the exception dispatch logic when an exception is being passed from a callee to a compiled caller.  So for any exception which propagates through multiple frames you'll likely end up in this code.  The forced deopt is a expensive for several reasons.  The obvious one is the work of deopt itself and the fact that you end up back in the interpreter at least for a little while.  The less obvious one is that the deoptimize_caller_frame logic goes through VM_DeoptimizeFrame which forces a safepoint to perform the deoptimization so it's very heavyweight.  I just noticed this recently while looking at someone who was seeing very high safepoint rates.  In this case the safepoint isn't actually required since the requesting thread is the same as the thread being deoptimized.  Skipping the deoptimize_frame call piggy backed nicely onto your changes.  We can fix the forced safepoint logic later.
> 
> tom
> 
> On Dec 15, 2009, at 1:18 PM, Deneau, Tom wrote:
> 
>> While making these changes for 6902182, I noticed the following:
>> 
>> I had at first only made the codegen changes in parse2.cpp and
>> graphKit.cpp.  I had a microbenchmark that pretty much did only throws
>> and catches (the catches were not in the same method as the throws but
>> maybe thru inlining the compiler could see the catches, I'm not sure).
>> With the codegen changes in parse2.cpp and graphKit.cpp, this
>> microbenchmark then got the same performance both with and without the
>> jdwp agent which was good.
>> 
>> I then tested against the JIRA webapp based workload which was where
>> we had first noticed the big performance impact of the jdwp agent
>> (performance with the jdwp agent was about 1/5 the performance without
>> the agent). We knew the JIRA server threw and caught a decent number
>> of exceptions, we could see 3% of its profile time in
>> Throwable::fill_in_stack_trace.  Anyway, with this JIRA workload the
>> parse2.cpp and graphKit.cpp changes had only a very small effect.  It
>> was only after fixing the logic in
>> OptoRuntime::handle_exception_C_helper in opto/runtime.cpp that we saw
>> a decent recovery.
>> 
>> Could someone explain why this would be so, I guess I'm really asking
>> in what cases OptoRuntime::handle_exception_C_helper gets used
>> vs. those cases where we just stay in JITted code?
>> 
>> -- Tom D.
>> 
>> 
>> -----Original Message-----
>> From: Thomas.Rodriguez at Sun.COM [mailto:Thomas.Rodriguez at Sun.COM] 
>> Sent: Monday, December 14, 2009 6:51 PM
>> To: Deneau, Tom
>> Cc: hotspot-compiler-dev at openjdk.java.net
>> Subject: Re: Request Review: 6902182: Starting with jdwp agent should not incur performance penalty
>> 
>> 
>> On Dec 14, 2009, at 1:26 PM, Deneau, Tom wrote:
>> 
>>> New webrev is at  http://cr.openjdk.java.net/~tdeneau/6902182/webrev.02/
>>> 
>>> This rev changes
>>> 
>>> * two places in the compiler where code for exception throws is
>>>   being JITted (see parse2.cpp, graphKit.cpp).  I was thinking the
>>>   common code in these two should be extracted to one place but I
>>>   wasn't sure whether that belonged in graphKit.cpp or in
>>>   macro.cpp.
>> 
>> GraphKit is base class containing most of the idiomatic code generation so you could put it there.
>> 
>>> 
>>> * trace_exception in opto/runtime.cpp
>>> 
>>> * exception_handler_for_pc_helper in c1/c1_Runtime1.cpp
>> 
>> In the two, why don't you use the thread that's in a local instead of calling JavaThread::current()?
>> 
>> In parse2.cpp why don't you just fall through instead of duplicating that code?
>> 
>> otherwise this looks good to me.
>> 
>> tom
>> 
>>> 
>>> 
>>> Previously these places checked jvmti_can_post_exceptions() which only
>>> looked at whether the jvmti capabilities for exceptions were enabled,
>>> taking a slow path if true.
>>> 
>>> Now they check a new flag JavaThread::_must_post_exception_events.
>>> This new flag is updated by calling jvmtiExport::must_post_exception_events
>>> whenever the jvmti situation for a thread might have changed.
>>> 
>>> jvmtiExport::must_post_exception_events uses logic similar to that
>>> used by jvmtiExport::post_exception_throw and returns false if
>>> jvmtiExport::post_exception_throw wouldn't have done anything.
>>> 
>>> I would appreciate it if someone familiar with the jvmti codepaths
>>> could review this to make sure that the must_post_exception_events
>>> flag is being updated in all the necessary places.  Right now, it gets
>>> updated in
>>> * JavaThread::set_jvmti_thread_state
>>> * JvmtiEventControllerPrivate::recompute_enabled
>>> 
>>> 
>>> 
>>> 
>>> 
>> 
>> 
>> 
> 
> 
>