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

[Vladimir Kozlov]

What webrev tool you are using?

If you use one from code review page (link in the page http://cr.openjdk.java.net/ )

here is doc:
http://blogs.sun.com/jcc/resource/webrev-doc.html

try next command to see if they list empty files and .hgtags:

hg outgoing
hg status

Vladimir

Deneau, Tom wrote:
> 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
>>>
>>>
>>>
>>>
>>>
>>
>>
> 
>