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

[Deneau, Tom]

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