Request for review (XS): 8006563: Remove unused ProfileVM_lock (original) (raw)
Rickard Bäckman rickard.backman at oracle.com
Fri Feb 1 01:59:30 PST 2013
- Previous message: Review Request: 7140852: Add test for 7022100
- Next message: RFR (S) 8007142: Add utility classes for writing better multiprocess tests in jtreg
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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.holmes at 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 ossolaris.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/ossolaris.cpp: 4265 GetThreadPCCallback cb(ProfileVMlock); 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 != Interruptlock&& !(this == Safepointlock&& contains(locks, Terminatorlock)&& SafepointSynchronize::issynchronizing())) { This way it is more obvious SafepointSynchronize::issynchronizing()) is the !(...) group. -Aleksey.
- Previous message: Review Request: 7140852: Add test for 7022100
- Next message: RFR (S) 8007142: Add utility classes for writing better multiprocess tests in jtreg
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]