Thanks for the review, David. /R
On Feb 1, 2013, at 7:59 AM, David Holmes wrote: > 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. >>>>>> >>