Request for review (XS): 8006563: Remove unused ProfileVM_lock
Rickard Bäckman
rickard.backman at oracle.com
Fri Feb 1 01:59:30 PST 2013
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 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.
>>>>>>
>>
More information about the hotspot-runtime-dev
mailing list