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

[Deneau, Tom]

Vladimir --

I will take care of these.

A question about webrev, yes, there were some files that in earlier webrevs I had changed, but then undid the cnanges before the final webrev.  For example opto/runtime.hpp.

Is there any way to tell webrev to exclude certain files or in particular exclude files that don't have any differences.

-- Tom



-----Original Message-----
From: Vladimir.Kozlov at Sun.COM [mailto:Vladimir.Kozlov at Sun.COM] 
Sent: Tuesday, December 15, 2009 5:07 PM
To: Deneau, Tom
Cc: Thomas.Rodriguez at Sun.COM; hotspot-compiler-dev at openjdk.java.net
Subject: Re: Request Review: 6902182: Starting with jdwp agent should not incur performance penalty

Tom,

I looked on C2 changes. They are good but I have few notes:

Less code lines in graphKit.cpp

!     Node* chk = _gvn.transform( new (C, 3) CmpINode(must_post_flag, intcon(0)) );
!     Node* tst = _gvn.transform( new (C, 2) BoolNode(chk, BoolTest::eq) );

In parse2 I think we should move BailoutToInterpreterForThrows logic
above jvmti_can_post_exceptions() otherwise you will have
two uncommon traps on both paths.

runtime.hpp has empty changes in webrev (merge problem?).

In thread.cpp I don't think you need to use method call to initialize
_must_post_exception_events_flag.

Also in thread.cpp closing parentheses in new methods are of by one space.

In thread.hpp move _must_post_exception_events_flag just after new comment:

+   // support for cached flag that indicates whether exceptions need to be posted for this thread
+   // if this is false, we can avoid deoptimizing when events are thrown
+   // this gets set to reflect whether jvmtiExport::post_exception_throw would actually do anything
+  private:
+   int    _must_post_exception_events_flag;
+
+  public:
+   int   must_post_exception_events_flag()  { return _must_post_exception_events_flag; }

I would move must_post_exception_events_flag_offset() down where others offsets are calculated.


.hgtags file in webrev ?


Thanks,
Vladimir

Deneau, Tom wrote:
> In http://cr.openjdk.java.net/~tdeneau/6902182/webrev.03
> the issues raised by Tom Rodriguez below have been addressed.
> 
>    
>    * there is now a common routine in graphKit.cpp called from both
>      GraphKit::builtin_throw and from parse2.cpp
> 
>    * in parse2.cpp, it falls thru, if the uncommon trap is not taken
> 
>    * uses local thread variable in the two runtime routines.
> 
> -- 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
>>
>>
>>
>>
>>
> 
> 
>