On 1/02/2013 4:54 PM, Rickard Bäckman wrote:
That was the idea.
However, can I have Ok for checking this into hs24 while waiting?

Sorry - ignore the hs25 comment - been looking at too many JDK review requests.

Yes this seems fine for hs24.

David

Thanks
/R

On Jan 21, 2013, at 11:33 PM, David Holmes wrote:

On 22/01/2013 12:09 AM, Rickard Bäckman wrote:
Yes, that code has changed. Checked in to hs24.

Okay but this is a review for hs25 ;-) So I assume that change will be there "real 
soon now". :)

David

/R

21 jan 2013 kl. 02:59 skrev David Holmes<david.hol...@oracle.com>:

On 18/01/2013 11:45 PM, Rickard Bäckman wrote:
Aleksey,

thanks for your review!

a) It was before on of my own changes used in os_solaris.cpp (I think, for 
synchronization support for Suspend/Resume).
I don't think we wanted something external to mess with that lock.

Seems to be used here:

./os/solaris/vm/os_solaris.cpp:

4265   GetThreadPC_Callback  cb(ProfileVM_lock);

Is this code already undergoing removal as part of the JFR changes?

Thanks,
David
-----


b) I've changed the indentation slightly.
Updated webrev at http://cr.openjdk.java.net/~rbackman/8006563.2/ (or at least 
currently copying…)

/R

On Jan 18, 2013, at 2:12 PM, Aleksey Shipilev wrote:

On 01/18/2013 04:58 PM, Rickard Bäckman wrote:
http://cr.openjdk.java.net/~rbackman/8006563/

Looks good to me (not a Reviewer), modulo:
a) Are we sure this thing is not acquired in some weird way, i.e.
through JVMTI, SA, or whatnot?
b) The formatting of the predicate does not follow it's structure, I
think it should be:
     ...
     this != Interrupt_lock&&
     !(this == Safepoint_lock&&
       contains(locks, Terminator_lock)&&
       SafepointSynchronize::is_synchronizing())) {

This way it is more obvious SafepointSynchronize::is_synchronizing()) is
the !(...) group.

-Aleksey.


Reply via email to