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

[Deneau, Tom]

Tom --

I agree it makes sense to cache a flag in the thread state for this.
Should the flag be in JavaThread or in jvmti_thread_state?
I assume it's slightly faster to access it if it's in JavaThread.
Should it be a new flag or an unused bit in some existing flag?

Is the following the general template for generating the nodes to read this flag?

  Node* thread = transform_later(new (C, 1) ThreadLocalNode());
  Node* adr = basic_plus_adr(top(), thread, in_bytes(JavaThread::new_flag_offset()));
  Node* new_flag = make_load(NULL, adr, TypeOopPtr::BOTTOM, T_OBJECT, NoAlias, false);

I don't really understand the first argument to basic_plus_adr()

I'll fix the other two can_post_exceptions cases you mentioned.

I can easily remove the global flag, I had it mostly for testing.

-- Tom D.


-----Original Message-----
From: Thomas.Rodriguez at Sun.COM [mailto:Thomas.Rodriguez at Sun.COM] 
Sent: Wednesday, December 09, 2009 4:04 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

Why can't you use a flag in the JavaThread instead of performing a runtime call every time?  That just seems too expensive and complicated for every exception.  I don't think we want this under a flag either.  It should be the default behaviour.

By the way, another thing I think you want to fix is this code in runtime.cpp:

    if (JvmtiExport::can_post_exceptions()) {
      // "Full-speed catching" is not necessary here,                                                         
      // since we're notifying the VM on every catch.                                                         
      // Force deoptimization and the rest of the lookup                                                     
      // will be fine.                                                                                       
      deoptimize_caller_frame(thread, true);
    }

It should also be checking that someone actually wants to see the exception.  There's similar code in c1_Runtime1.cpp.  I don't the client compiler needs any other changes to work properly with debug enabled.

tom

On Dec 9, 2009, at 1:31 PM, Deneau, Tom wrote:

> Webrev is at  http://cr.openjdk.java.net/~tdeneau/6902182/webrev.01/
> 
> This webrev changes two places in the compiler where code for exception
> throws is being JITted.
> 
>   * previously these checked jvmti_can_post_exceptions() at compile
>     time, compiling in a slow path if true
> 
>   * now they call a new jvmtiExport::must_post_exception_events at
>     run time, and take a faster path if must_post_exception_events is
>     false.
> 
> 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.
> 
> A similar change of calling must_post_exception_events instead of
> jvmti_can_post_exceptions is made in the exception runtime code in
> trace_exception in opto/runtime.cpp
> 
> Throughput Numbers from a JIRA-based web workload (JIRA makes fairly heavy use
> of exception throws and catches).  Bigger is better.
> 
>   unmodified hotspot, no jdwp agent       69.7
>   unmodified hotspot, with jdwp agent     11.8 
>   modified hotspot, with jdwp agent       65.7
> 	
> All changes are platform-independent.
> 
> -- Tom Deneau
>   tom.deneau at amd.com
> 
> 
>