RFR(M): 7147848: [macosx] com.sun.management.UnixOperatingSystem uses hardcoded dummy values

[Dmitry Samersoff]

Looks good for me.

-Dmitry

On 2012-04-11 20:24, Staffan Larsen wrote:
> Apparently something went wrong with the webrev. Here is new one:
>
> http://cr.openjdk.java.net/~sla/7147848/webrev.02/
>
> Thanks Dmitry,
> /Staffan
>
> On 11 apr 2012, at 12:33, Dmitry Samersoff wrote:
>
>> Staffan,
>>
>> MacosxOperatingSystem.c
>>
>> Probably something went wrong with webrev:
>>
>> 48 if (kr != KERN_SUCCESS) {
>> 49 return 0;
>> 50 }
>>
>>
>> 133 if (gettimeofday(&now, NULL)) {
>> 134 return -1;
>> 135 }
>>
>> -Dmitry
>>
>>
>> On 2012-04-11 11:52, Staffan Larsen wrote:
>>> Thank you all for your comments!
>>>
>>> New webrev here: http://cr.openjdk.java.net/~sla/7147848/webrev.01/
>>>
>>> Changes:
>>> - Fixed Dmitry's comments
>>> - Updated TestTotalSwap.sh for Darwin (thanks Mandy)
>>>
>>> I have verified the values manually by running a Java program (see
>>> attachment) and comparing to top and other system utilities. I've also
>>> compared output with a Linux system to compare magnitude of numbers. I
>>> have run the various regression tests.
>>>
>>> Thanks,
>>> /Staffan
>>>
>>>
>>>
>>>
>>> On 10 apr 2012, at 21:57, Mandy Chung wrote:
>>>
>>>> Staffan, Roger,
>>>>
>>>> There isn't any undocumented semantics other than what the
>>>> specification for com.sun.management.OperatingSystemMXBean specifies
>>>> (that is indicated by its method name):
>>>> http://docs.oracle.com/javase/7/docs/jre/api/management/extension/index.html
>>>>
>>>> As Roger suggests, you can do some sanity tests and compare the result
>>>> with native commands (top or other CLI). There are a few regression
>>>> tests in jdk/test/com/sun/management. In particular, you might want to
>>>> update test/com/sun/management/TestTotalSwap.sh to check the swap
>>>> space with a suitable macosx command, if there is one.
>>>>
>>>> FYI. I'm not familiar with Mac OS X API and didn't review the code.
>>>>
>>>> Mandy
>>>>
>>>> On 4/10/2012 10:51 AM, rhoover wrote:
>>>>> Scott, Steffan
>>>>>
>>>>> (I am he one who put in the original lines of: return -1.0; // not
>>>>> available being replaced by this patch)
>>>>>
>>>>> I was reluctant to implement these functions because the linux code
>>>>> was quite involved and it appeared to me that there might be some
>>>>> additional semantics to what was implemented than what was indicated
>>>>> by the function names alone.
>>>>>
>>>>> I have not compared the code with the 'top' source, but it looks
>>>>> plausible. As a sanity test, the function values being returned could
>>>>> be printed by a java program and visually compared with the output of
>>>>> 'top' as a system is loaded up. It might also be wise to run the same
>>>>> java program on other platforms to make sure that the magnitude of
>>>>> the numbers is in the same ballpark.
>>>>>
>>>>> On Apr 10, 2012, at 10:21 AM, Scott Kovatch wrote:
>>>>>
>>>>>> Regarding Apple, Roger Hoover would be a good person to look at
>>>>>> this, as he's spent more time in the Darwin levels of the VM. I
>>>>>> think he's still partially attached to the OpenJDK work.
>>>>>>
>>>>>> -- Scott
>>>>>>
>>>>>> On Apr 10, 2012, at 8:58 AM, Daniel D. Daugherty wrote:
>>>>>>
>>>>>>> Staffan,
>>>>>>>
>>>>>>> I reviewed it and I think it looks OK. I tried looking at the code
>>>>>>> in MacosxOperatingSystem.c relative to the Linux version, but I think
>>>>>>> it is easily possible to miss something subtle here.
>>>>>>>
>>>>>>> You might try a direct ping to Mandy Chung since M&M was her area.
>>>>>>> You might also try a direct ping to Mike Swingler to get an Apple
>>>>>>> reviewer.
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 4/10/12 3:30 AM, Staffan Larsen wrote:
>>>>>>>> Any takers for this review? (added core-libs-dev as well)
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> /Staffan
>>>>>>>>
>>>>>>>> On 3 apr 2012, at 15:39, Staffan Larsen wrote:
>>>>>>>>
>>>>>>>>> Please review the following fix:
>>>>>>>>>
>>>>>>>>> webrev:
>>>>>>>>> http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>
>>>>>>>>> <http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>>
>>>>>>>>> <http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>>
>>>>>>>>> <http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>>>
>>>>>>>>> bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7147848
>>>>>>>>>
>>>>>>>>> This fix implements the missing functionality in
>>>>>>>>> UnixOperatingSystem for Mac OS X. Any feedback on the
>>>>>>>>> implementation is welcome as I am not very familiar with the APIs
>>>>>>>>> in Mac OS X.
>>>>>>>>>
>>>>>>>>> I have verified that the changes build on all platforms through
>>>>>>>>> JPRT. The correctness has been verified manually by looking in
>>>>>>>>> JConsole and running the tests in
>>>>>>>>> test/java/lang/management/OperatingSystemMXBean
>>>>>>>>> test/com/sun/management/OperatingSystemMXBean.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> /Staffan
>>>
>>
>>
>> --
>> Dmitry Samersoff
>> Java Hotspot development team, SPB04
>> * There will come soft rains ...
>


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...