Re: RFR: 7165755 OS Information much longer on linux than other platforms
Right!
Well Staffan Larsen was, but he's gone on vacation this week. I'll dig up
another.
I'm looking to push this to hotspot-rt as well as 7u6, which as you probably
know is here:
http://hg.openjdk.java.net/hsx/hsx23.2/hotspot/
Regards,
Nils Loodin
On May 12, 2012, at 14:22 , David Holmes wrote:
> Hi Nils,
>
> On 11/05/2012 11:45 PM, Nils Loodin wrote:
>> And, to be clear, I updated a few lines after the conversation with
>> David. Here's the lastest:
>> http://cr.openjdk.java.net/~nloodin/7165755/webrev.03
>> <http://cr.openjdk.java.net/~nloodin/7165755/webrev.0>
>
> I'm okay with this change. Do you have a second reviewer for this final
> format?
>
>> Also, David, could you sponsor my push if you're happy?
>
> Where are you pushing to? hotspot-rt?
>
> I can do the push once my home directory server comes back online :(
>
> And once I see a second reviewer.
>
> Thanks,
> David
>
>
>> Dear regards,
>> Nils Loodin
>>
>> On May 11, 2012, at 14:07 , Nils Loodin wrote:
>>
>>> Lists accidentally dropped of here.
>>>
>>> For the official record, Davd Holmes, are you hereby happy with the
>>> state of my changes and ready to stand by that as an official reviewer? :)
>>>
>>> Regards,
>>> Nils Loodin
>>>
>>> Begin forwarded message:
>>>
>>>> *From: *David Holmes >>> <mailto:david.hol...@oracle.com>>
>>>> *Subject: **Re: RFR: 7165755 OS Information much longer on linux than
>>>> other platforms*
>>>> *Date: *May 11, 2012 12:52:12 GMT+02:00
>>>> *To: *Nils Loodin >>> <mailto:nils.loo...@oracle.com>>
>>>>
>>>> On 11/05/2012 5:22 PM, Nils Loodin wrote:
>>>>>> Missed one:
>>>>>>
>>>>>> 39 #include "os_linux.hpp"
>>>>>>
>>>>> Gah. Indeed.
>>>>>
>>>>>>
>>>>>> Do we want the initial st->print("OS:") on the brief info the way we
>>>>>> have on the full info?
>>>>> I judged no, due to the fact that this would (well in our case anyway,
>>>>> but I thought generally) be used by other tools to get a brief info.
>>>>> They would then have that string as a label in a gui, or something else.
>>>>> Or if they want to print somewhere, they should print that string on
>>>>> their own.
>>>>
>>>> Ok.
>>>>
>>>>> About OS_xx.cpp including OS_xx.hpp, I feel I'm missing something.. can
>>>>> you please help me say why that's a no-no? And what does it have to do
>>>>> with the old included implementation?
>>>>
>>>> It isn't that it is wrong, it's that it hasn't been necessary. So I
>>>> was wondering why it was now necessary, as if it had been necessary I
>>>> would have expected it to be done when the old include system got
>>>> converted. I think these files only need include the generic os.hpp
>>>> and that it turn will include the platform specific ones.
>>>>
>>>>> Changing the above, are you ok with the changes?
>>>>
>>>> Yes.
>>>>
>>>> Though we should probably be having this conversation on the open
>>>> lists ;-)
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>
>>>>>>
>>>>>> David
>>>>>
>>>>> Regards,
>>>>> Nils Loodin
>>>>>
>>>>>
>>>>>
>>>
>>
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Hi Nils,
On 11/05/2012 11:45 PM, Nils Loodin wrote:
And, to be clear, I updated a few lines after the conversation with
David. Here's the lastest:
http://cr.openjdk.java.net/~nloodin/7165755/webrev.03
<http://cr.openjdk.java.net/~nloodin/7165755/webrev.0>
I'm okay with this change. Do you have a second reviewer for this final
format?
Also, David, could you sponsor my push if you're happy?
Where are you pushing to? hotspot-rt?
I can do the push once my home directory server comes back online :(
And once I see a second reviewer.
Thanks,
David
Dear regards,
Nils Loodin
On May 11, 2012, at 14:07 , Nils Loodin wrote:
Lists accidentally dropped of here.
For the official record, Davd Holmes, are you hereby happy with the
state of my changes and ready to stand by that as an official reviewer? :)
Regards,
Nils Loodin
Begin forwarded message:
*From: *David Holmes mailto:david.hol...@oracle.com>>
*Subject: **Re: RFR: 7165755 OS Information much longer on linux than
other platforms*
*Date: *May 11, 2012 12:52:12 GMT+02:00
*To: *Nils Loodin mailto:nils.loo...@oracle.com>>
On 11/05/2012 5:22 PM, Nils Loodin wrote:
Missed one:
39 #include "os_linux.hpp"
Gah. Indeed.
Do we want the initial st->print("OS:") on the brief info the way we
have on the full info?
I judged no, due to the fact that this would (well in our case anyway,
but I thought generally) be used by other tools to get a brief info.
They would then have that string as a label in a gui, or something else.
Or if they want to print somewhere, they should print that string on
their own.
Ok.
About OS_xx.cpp including OS_xx.hpp, I feel I'm missing something.. can
you please help me say why that's a no-no? And what does it have to do
with the old included implementation?
It isn't that it is wrong, it's that it hasn't been necessary. So I
was wondering why it was now necessary, as if it had been necessary I
would have expected it to be done when the old include system got
converted. I think these files only need include the generic os.hpp
and that it turn will include the platform specific ones.
Changing the above, are you ok with the changes?
Yes.
Though we should probably be having this conversation on the open
lists ;-)
Cheers,
David
David
Regards,
Nils Loodin
Re: RFR: 7165755 OS Information much longer on linux than other platforms
And, to be clear, I updated a few lines after the conversation with David.
Here's the lastest:
http://cr.openjdk.java.net/~nloodin/7165755/webrev.03
Also, David, could you sponsor my push if you're happy?
Dear regards,
Nils Loodin
On May 11, 2012, at 14:07 , Nils Loodin wrote:
> Lists accidentally dropped of here.
>
> For the official record, Davd Holmes, are you hereby happy with the state of
> my changes and ready to stand by that as an official reviewer? :)
>
> Regards,
> Nils Loodin
>
> Begin forwarded message:
>
>> From: David Holmes
>> Subject: Re: RFR: 7165755 OS Information much longer on linux than other
>> platforms
>> Date: May 11, 2012 12:52:12 GMT+02:00
>> To: Nils Loodin
>>
>> On 11/05/2012 5:22 PM, Nils Loodin wrote:
>>>> Missed one:
>>>>
>>>> 39 #include "os_linux.hpp"
>>>>
>>> Gah. Indeed.
>>>
>>>>
>>>> Do we want the initial st->print("OS:") on the brief info the way we
>>>> have on the full info?
>>> I judged no, due to the fact that this would (well in our case anyway,
>>> but I thought generally) be used by other tools to get a brief info.
>>> They would then have that string as a label in a gui, or something else.
>>> Or if they want to print somewhere, they should print that string on
>>> their own.
>>
>> Ok.
>>
>>> About OS_xx.cpp including OS_xx.hpp, I feel I'm missing something.. can
>>> you please help me say why that's a no-no? And what does it have to do
>>> with the old included implementation?
>>
>> It isn't that it is wrong, it's that it hasn't been necessary. So I was
>> wondering why it was now necessary, as if it had been necessary I would have
>> expected it to be done when the old include system got converted. I think
>> these files only need include the generic os.hpp and that it turn will
>> include the platform specific ones.
>>
>>> Changing the above, are you ok with the changes?
>>
>> Yes.
>>
>> Though we should probably be having this conversation on the open lists ;-)
>>
>> Cheers,
>> David
>>
>>
>>>>
>>>> David
>>>
>>> Regards,
>>> Nils Loodin
>>>
>>>
>>>
>
Fwd: RFR: 7165755 OS Information much longer on linux than other platforms
Lists accidentally dropped of here.
For the official record, Davd Holmes, are you hereby happy with the state of my
changes and ready to stand by that as an official reviewer? :)
Regards,
Nils Loodin
Begin forwarded message:
> From: David Holmes
> Subject: Re: RFR: 7165755 OS Information much longer on linux than other
> platforms
> Date: May 11, 2012 12:52:12 GMT+02:00
> To: Nils Loodin
>
> On 11/05/2012 5:22 PM, Nils Loodin wrote:
>>> Missed one:
>>>
>>> 39 #include "os_linux.hpp"
>>>
>> Gah. Indeed.
>>
>>>
>>> Do we want the initial st->print("OS:") on the brief info the way we
>>> have on the full info?
>> I judged no, due to the fact that this would (well in our case anyway,
>> but I thought generally) be used by other tools to get a brief info.
>> They would then have that string as a label in a gui, or something else.
>> Or if they want to print somewhere, they should print that string on
>> their own.
>
> Ok.
>
>> About OS_xx.cpp including OS_xx.hpp, I feel I'm missing something.. can
>> you please help me say why that's a no-no? And what does it have to do
>> with the old included implementation?
>
> It isn't that it is wrong, it's that it hasn't been necessary. So I was
> wondering why it was now necessary, as if it had been necessary I would have
> expected it to be done when the old include system got converted. I think
> these files only need include the generic os.hpp and that it turn will
> include the platform specific ones.
>
>> Changing the above, are you ok with the changes?
>
> Yes.
>
> Though we should probably be having this conversation on the open lists ;-)
>
> Cheers,
> David
>
>
>>>
>>> David
>>
>> Regards,
>> Nils Loodin
>>
>>
>>
Re: RFR: 7165755 OS Information much longer on linux than other platforms
On 8/05/2012 6:03 PM, Nils Loodin wrote: Hrm, sorry about that people, The webrev number should have been bumped from 00 to 01. :) http://cr.openjdk.java.net/~nloodin/7165755/webrev.01/ Why are all the os_XXX.cpp files now explicitly #including their os_XXX.hpp counterpart? Was this something previously missing from the great includeDB conversion? src/os/posix/vm/os_posix.cpp 115 //Isn't there on OSX 116 #ifdef __APPLE__ Shouldn't that be ifndef ? Any why test __APPLE__ instead of for BSD/OSX eg TARGET_OS_FAMILY_bsd? Otherwise seems okay. David - Nisse On May 8, 2012, at 07:37 , David Holmes wrote: On 4/05/2012 6:26 PM, Nils Loodin wrote: Updated this with pulling out some shared code to os_posix.cpp. Some methods were different enough that this wasn't possible though. Do you like this better? Webrev: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ I don't see any os_posix.cpp changes here? David - On May 2, 2012, at 14:08 , Nils Loodin wrote: On May 2, 2012, at 14:00 , David Holmes wrote: Hi Nils, On 2/05/2012 9:56 PM, Nils Loodin wrote: Ignoring Windows (with prints very little) I'd say it is the printing of /proc/meminfo that is the main difference. Not sure why printing that was necessary ... but if we are going to remove it I think we need to know why it was added. Yes, that's the reason. Note that nothing is removed. The method still prints exactly the same info, but I introduced another method to print briefer info, to be kinder to tool developers. The current one prints /proc/meminfo. You turned that code into print_full_memory_info but in the main routine you call print_memory_info. Was that a mistake? YES! Glad you caught that :) Guess (or hope) it would have been caught in dump testing otherwise :) Regards, Nils Loodin David I really don't want to change the output for say, hs_err files, where I believe this info is used. This can make it hard for tool writers to get a summary that look good and similar for multiple platforms (sizing of gui fields, having to parse info in the tool code etc) Lookin at the code, it's in some serious need of refactoring. It would be nice with a method to get a "brief" os info for these kinds of tools that looks similar on all platforms. This is my suggested change: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ Seems to me some of this could be factored into the top-level OS class if we shoehorn Windows into the same shape as the other OSes ;-) This was my first attempt also, but then a lot of empty windows-methods ensued, which was kind of ugly. Or at least perhaps put some of the common stuff into os_posix.cpp ? There's a thought! I'll investigate that route, it could get things to look nicer. Cheers, David Regards, Nils Loodin
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Hrm, sorry about that people, The webrev number should have been bumped from 00 to 01. :) http://cr.openjdk.java.net/~nloodin/7165755/webrev.01/ Nisse On May 8, 2012, at 07:37 , David Holmes wrote: > On 4/05/2012 6:26 PM, Nils Loodin wrote: >> Updated this with pulling out some shared code to os_posix.cpp. >> Some methods were different enough that this wasn't possible though. >> >> Do you like this better? >> >> Webrev: >> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ > > I don't see any os_posix.cpp changes here? > > David > - > > >> On May 2, 2012, at 14:08 , Nils Loodin wrote: >> >>> >>> On May 2, 2012, at 14:00 , David Holmes wrote: >>> Hi Nils, On 2/05/2012 9:56 PM, Nils Loodin wrote: >> Ignoring Windows (with prints very little) I'd say it is the printing of >> /proc/meminfo that is the main difference. Not sure why printing that >> was necessary ... but if we are going to remove it I think we need to >> know why it was added. > Yes, that's the reason. > Note that nothing is removed. The method still prints exactly the same > info, but I introduced another method to print briefer info, to be kinder > to tool developers. The current one prints /proc/meminfo. You turned that code into print_full_memory_info but in the main routine you call print_memory_info. Was that a mistake? >>> >>> YES! Glad you caught that :) Guess (or hope) it would have been caught in >>> dump testing otherwise :) >>> >>> Regards, >>> Nils Loodin >>> >>> David > I really don't want to change the output for say, hs_err files, where I > believe this info is used. > >> >>> This can make it hard for tool writers to get a summary that look good >>> and similar for multiple platforms (sizing of gui fields, having to >>> parse info in the tool code etc) >>> Lookin at the code, it's in some serious need of refactoring. It would >>> be nice with a method to get a "brief" os info for these kinds of tools >>> that looks similar on all platforms. >>> >>> This is my suggested change: >>> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ >> >> Seems to me some of this could be factored into the top-level OS class >> if we shoehorn Windows into the same shape as the other OSes ;-) > This was my first attempt also, but then a lot of empty windows-methods > ensued, which was kind of ugly. > >> Or at least perhaps put some of the common stuff into os_posix.cpp ? > There's a thought! > I'll investigate that route, it could get things to look nicer. > > >> Cheers, >> David > Regards, > Nils Loodin > >>> >>
Re: RFR: 7165755 OS Information much longer on linux than other platforms
On 4/05/2012 6:26 PM, Nils Loodin wrote: Updated this with pulling out some shared code to os_posix.cpp. Some methods were different enough that this wasn't possible though. Do you like this better? Webrev: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ I don't see any os_posix.cpp changes here? David - On May 2, 2012, at 14:08 , Nils Loodin wrote: On May 2, 2012, at 14:00 , David Holmes wrote: Hi Nils, On 2/05/2012 9:56 PM, Nils Loodin wrote: Ignoring Windows (with prints very little) I'd say it is the printing of /proc/meminfo that is the main difference. Not sure why printing that was necessary ... but if we are going to remove it I think we need to know why it was added. Yes, that's the reason. Note that nothing is removed. The method still prints exactly the same info, but I introduced another method to print briefer info, to be kinder to tool developers. The current one prints /proc/meminfo. You turned that code into print_full_memory_info but in the main routine you call print_memory_info. Was that a mistake? YES! Glad you caught that :) Guess (or hope) it would have been caught in dump testing otherwise :) Regards, Nils Loodin David I really don't want to change the output for say, hs_err files, where I believe this info is used. This can make it hard for tool writers to get a summary that look good and similar for multiple platforms (sizing of gui fields, having to parse info in the tool code etc) Lookin at the code, it's in some serious need of refactoring. It would be nice with a method to get a "brief" os info for these kinds of tools that looks similar on all platforms. This is my suggested change: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ Seems to me some of this could be factored into the top-level OS class if we shoehorn Windows into the same shape as the other OSes ;-) This was my first attempt also, but then a lot of empty windows-methods ensued, which was kind of ugly. Or at least perhaps put some of the common stuff into os_posix.cpp ? There's a thought! I'll investigate that route, it could get things to look nicer. Cheers, David Regards, Nils Loodin
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Staffan, OK. -Dmitry On 2012-05-04 14:11, Staffan Larsen wrote: > Note that there is _lots_ of old cruft in os_bsd.cpp that are leftovers from > the bsd port and needs to be cleaned up. > Basically the file is a copy of os_linux.cpp with things removed by #ifdefs. > > I don't think it is in the scope of Nils' current work to do this cleanup. > > /Staffan > > On 4 maj 2012, at 11:41, Dmitry Samersoff wrote: > >> Nils, >> >> os_bsd.cpp: >> >> 2359: // Print warning if unsafe chroot environment detected >> constant below is >>unstable_chroot_error >> >> 2373: Never heard about neither about /etc/XXX-release on BSD nor >> about mandrake or redhat bsd. >> Please fix it as well. >> >> 2409: st->print(os::Bsd::glibc_version()) >> glibc_version() looks a little bit weird as >> none of BSD has glibc. >> >> >> -Dmitry >> >> >> >> On 2012-05-04 12:26, Nils Loodin wrote: >>> Updated this with pulling out some shared code to os_posix.cpp. >>> Some methods were different enough that this wasn't possible though. >>> >>> Do you like this better? >>> >>> Webrev: >>> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ >>> >>> On May 2, 2012, at 14:08 , Nils Loodin wrote: >>> On May 2, 2012, at 14:00 , David Holmes wrote: > Hi Nils, > > On 2/05/2012 9:56 PM, Nils Loodin wrote: >>> Ignoring Windows (with prints very little) I'd say it is the printing >>> of /proc/meminfo that is the main difference. Not sure why printing >>> that was necessary ... but if we are going to remove it I think we need >>> to know why it was added. >> Yes, that's the reason. >> Note that nothing is removed. The method still prints exactly the same >> info, but I introduced another method to print briefer info, to be >> kinder to tool developers. > > The current one prints /proc/meminfo. You turned that code into > print_full_memory_info but in the main routine you call > print_memory_info. Was that a mistake? YES! Glad you caught that :) Guess (or hope) it would have been caught in dump testing otherwise :) Regards, Nils Loodin > > David > >> I really don't want to change the output for say, hs_err files, where I >> believe this info is used. >> >>> This can make it hard for tool writers to get a summary that look good and similar for multiple platforms (sizing of gui fields, having to parse info in the tool code etc) Lookin at the code, it's in some serious need of refactoring. It would be nice with a method to get a "brief" os info for these kinds of tools that looks similar on all platforms. This is my suggested change: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ >>> >>> Seems to me some of this could be factored into the top-level OS class >>> if we shoehorn Windows into the same shape as the other OSes ;-) >> This was my first attempt also, but then a lot of empty windows-methods >> ensued, which was kind of ugly. >> >>> Or at least perhaps put some of the common stuff into os_posix.cpp ? >> There's a thought! >> I'll investigate that route, it could get things to look nicer. >> >> >>> Cheers, >>> David >> Regards, >> Nils Loodin >> >>> >> >> >> -- >> Dmitry Samersoff >> Java Hotspot development team, SPB04 >> * There will come soft rains ... > -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Note that there is _lots_ of old cruft in os_bsd.cpp that are leftovers from the bsd port and needs to be cleaned up. Basically the file is a copy of os_linux.cpp with things removed by #ifdefs. I don't think it is in the scope of Nils' current work to do this cleanup. /Staffan On 4 maj 2012, at 11:41, Dmitry Samersoff wrote: > Nils, > > os_bsd.cpp: > > 2359: // Print warning if unsafe chroot environment detected > constant below is >unstable_chroot_error > > 2373: Never heard about neither about /etc/XXX-release on BSD nor > about mandrake or redhat bsd. > Please fix it as well. > > 2409: st->print(os::Bsd::glibc_version()) > glibc_version() looks a little bit weird as > none of BSD has glibc. > > > -Dmitry > > > > On 2012-05-04 12:26, Nils Loodin wrote: >> Updated this with pulling out some shared code to os_posix.cpp. >> Some methods were different enough that this wasn't possible though. >> >> Do you like this better? >> >> Webrev: >> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ >> >> On May 2, 2012, at 14:08 , Nils Loodin wrote: >> >>> >>> On May 2, 2012, at 14:00 , David Holmes wrote: >>> Hi Nils, On 2/05/2012 9:56 PM, Nils Loodin wrote: >> Ignoring Windows (with prints very little) I'd say it is the printing of >> /proc/meminfo that is the main difference. Not sure why printing that >> was necessary ... but if we are going to remove it I think we need to >> know why it was added. > Yes, that's the reason. > Note that nothing is removed. The method still prints exactly the same > info, but I introduced another method to print briefer info, to be kinder > to tool developers. The current one prints /proc/meminfo. You turned that code into print_full_memory_info but in the main routine you call print_memory_info. Was that a mistake? >>> >>> YES! Glad you caught that :) Guess (or hope) it would have been caught in >>> dump testing otherwise :) >>> >>> Regards, >>> Nils Loodin >>> >>> David > I really don't want to change the output for say, hs_err files, where I > believe this info is used. > >> >>> This can make it hard for tool writers to get a summary that look good >>> and similar for multiple platforms (sizing of gui fields, having to >>> parse info in the tool code etc) >>> Lookin at the code, it's in some serious need of refactoring. It would >>> be nice with a method to get a "brief" os info for these kinds of tools >>> that looks similar on all platforms. >>> >>> This is my suggested change: >>> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ >> >> Seems to me some of this could be factored into the top-level OS class >> if we shoehorn Windows into the same shape as the other OSes ;-) > This was my first attempt also, but then a lot of empty windows-methods > ensued, which was kind of ugly. > >> Or at least perhaps put some of the common stuff into os_posix.cpp ? > There's a thought! > I'll investigate that route, it could get things to look nicer. > > >> Cheers, >> David > Regards, > Nils Loodin > >>> >> > > > -- > Dmitry Samersoff > Java Hotspot development team, SPB04 > * There will come soft rains ...
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Nils, os_bsd.cpp: 2359: // Print warning if unsafe chroot environment detected constant below is unstable_chroot_error 2373: Never heard about neither about /etc/XXX-release on BSD nor about mandrake or redhat bsd. Please fix it as well. 2409: st->print(os::Bsd::glibc_version()) glibc_version() looks a little bit weird as none of BSD has glibc. -Dmitry On 2012-05-04 12:26, Nils Loodin wrote: > Updated this with pulling out some shared code to os_posix.cpp. > Some methods were different enough that this wasn't possible though. > > Do you like this better? > > Webrev: > http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ > > On May 2, 2012, at 14:08 , Nils Loodin wrote: > >> >> On May 2, 2012, at 14:00 , David Holmes wrote: >> >>> Hi Nils, >>> >>> On 2/05/2012 9:56 PM, Nils Loodin wrote: > Ignoring Windows (with prints very little) I'd say it is the printing of > /proc/meminfo that is the main difference. Not sure why printing that was > necessary ... but if we are going to remove it I think we need to know > why it was added. Yes, that's the reason. Note that nothing is removed. The method still prints exactly the same info, but I introduced another method to print briefer info, to be kinder to tool developers. >>> >>> The current one prints /proc/meminfo. You turned that code into >>> print_full_memory_info but in the main routine you call print_memory_info. >>> Was that a mistake? >> >> YES! Glad you caught that :) Guess (or hope) it would have been caught in >> dump testing otherwise :) >> >> Regards, >> Nils Loodin >> >> >>> >>> David >>> I really don't want to change the output for say, hs_err files, where I believe this info is used. > >> This can make it hard for tool writers to get a summary that look good >> and similar for multiple platforms (sizing of gui fields, having to >> parse info in the tool code etc) >> Lookin at the code, it's in some serious need of refactoring. It would >> be nice with a method to get a "brief" os info for these kinds of tools >> that looks similar on all platforms. >> >> This is my suggested change: >> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ > > Seems to me some of this could be factored into the top-level OS class if > we shoehorn Windows into the same shape as the other OSes ;-) This was my first attempt also, but then a lot of empty windows-methods ensued, which was kind of ugly. > Or at least perhaps put some of the common stuff into os_posix.cpp ? There's a thought! I'll investigate that route, it could get things to look nicer. > Cheers, > David Regards, Nils Loodin >> > -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Updated this with pulling out some shared code to os_posix.cpp. Some methods were different enough that this wasn't possible though. Do you like this better? Webrev: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ On May 2, 2012, at 14:08 , Nils Loodin wrote: > > On May 2, 2012, at 14:00 , David Holmes wrote: > >> Hi Nils, >> >> On 2/05/2012 9:56 PM, Nils Loodin wrote: Ignoring Windows (with prints very little) I'd say it is the printing of /proc/meminfo that is the main difference. Not sure why printing that was necessary ... but if we are going to remove it I think we need to know why it was added. >>> Yes, that's the reason. >>> Note that nothing is removed. The method still prints exactly the same >>> info, but I introduced another method to print briefer info, to be kinder >>> to tool developers. >> >> The current one prints /proc/meminfo. You turned that code into >> print_full_memory_info but in the main routine you call print_memory_info. >> Was that a mistake? > > YES! Glad you caught that :) Guess (or hope) it would have been caught in > dump testing otherwise :) > > Regards, > Nils Loodin > > >> >> David >> >>> I really don't want to change the output for say, hs_err files, where I >>> believe this info is used. >>> > This can make it hard for tool writers to get a summary that look good > and similar for multiple platforms (sizing of gui fields, having to parse > info in the tool code etc) > Lookin at the code, it's in some serious need of refactoring. It would be > nice with a method to get a "brief" os info for these kinds of tools that > looks similar on all platforms. > > This is my suggested change: > http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ Seems to me some of this could be factored into the top-level OS class if we shoehorn Windows into the same shape as the other OSes ;-) >>> This was my first attempt also, but then a lot of empty windows-methods >>> ensued, which was kind of ugly. >>> Or at least perhaps put some of the common stuff into os_posix.cpp ? >>> There's a thought! >>> I'll investigate that route, it could get things to look nicer. >>> >>> Cheers, David >>> Regards, >>> Nils Loodin >>> >
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Does anybody know if there are any tests that verify the actual contents of the dump? /Mikael On 2012-05-02 14:08, Nils Loodin wrote: On May 2, 2012, at 14:00 , David Holmes wrote: Hi Nils, On 2/05/2012 9:56 PM, Nils Loodin wrote: Ignoring Windows (with prints very little) I'd say it is the printing of /proc/meminfo that is the main difference. Not sure why printing that was necessary ... but if we are going to remove it I think we need to know why it was added. Yes, that's the reason. Note that nothing is removed. The method still prints exactly the same info, but I introduced another method to print briefer info, to be kinder to tool developers. The current one prints /proc/meminfo. You turned that code into print_full_memory_info but in the main routine you call print_memory_info. Was that a mistake? YES! Glad you caught that :) Guess (or hope) it would have been caught in dump testing otherwise :) Regards, Nils Loodin David I really don't want to change the output for say, hs_err files, where I believe this info is used. This can make it hard for tool writers to get a summary that look good and similar for multiple platforms (sizing of gui fields, having to parse info in the tool code etc) Lookin at the code, it's in some serious need of refactoring. It would be nice with a method to get a "brief" os info for these kinds of tools that looks similar on all platforms. This is my suggested change: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ Seems to me some of this could be factored into the top-level OS class if we shoehorn Windows into the same shape as the other OSes ;-) This was my first attempt also, but then a lot of empty windows-methods ensued, which was kind of ugly. Or at least perhaps put some of the common stuff into os_posix.cpp ? There's a thought! I'll investigate that route, it could get things to look nicer. Cheers, David Regards, Nils Loodin
Re: RFR: 7165755 OS Information much longer on linux than other platforms
On May 2, 2012, at 14:00 , David Holmes wrote: > Hi Nils, > > On 2/05/2012 9:56 PM, Nils Loodin wrote: >>> Ignoring Windows (with prints very little) I'd say it is the printing of >>> /proc/meminfo that is the main difference. Not sure why printing that was >>> necessary ... but if we are going to remove it I think we need to know why >>> it was added. >> Yes, that's the reason. >> Note that nothing is removed. The method still prints exactly the same info, >> but I introduced another method to print briefer info, to be kinder to tool >> developers. > > The current one prints /proc/meminfo. You turned that code into > print_full_memory_info but in the main routine you call print_memory_info. > Was that a mistake? YES! Glad you caught that :) Guess (or hope) it would have been caught in dump testing otherwise :) Regards, Nils Loodin > > David > >> I really don't want to change the output for say, hs_err files, where I >> believe this info is used. >> >>> This can make it hard for tool writers to get a summary that look good and similar for multiple platforms (sizing of gui fields, having to parse info in the tool code etc) Lookin at the code, it's in some serious need of refactoring. It would be nice with a method to get a "brief" os info for these kinds of tools that looks similar on all platforms. This is my suggested change: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ >>> >>> Seems to me some of this could be factored into the top-level OS class if >>> we shoehorn Windows into the same shape as the other OSes ;-) >> This was my first attempt also, but then a lot of empty windows-methods >> ensued, which was kind of ugly. >> >>> Or at least perhaps put some of the common stuff into os_posix.cpp ? >> There's a thought! >> I'll investigate that route, it could get things to look nicer. >> >> >>> Cheers, >>> David >> Regards, >> Nils Loodin >>
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Hi Nils, On 2/05/2012 9:56 PM, Nils Loodin wrote: Ignoring Windows (with prints very little) I'd say it is the printing of /proc/meminfo that is the main difference. Not sure why printing that was necessary ... but if we are going to remove it I think we need to know why it was added. Yes, that's the reason. Note that nothing is removed. The method still prints exactly the same info, but I introduced another method to print briefer info, to be kinder to tool developers. The current one prints /proc/meminfo. You turned that code into print_full_memory_info but in the main routine you call print_memory_info. Was that a mistake? David I really don't want to change the output for say, hs_err files, where I believe this info is used. This can make it hard for tool writers to get a summary that look good and similar for multiple platforms (sizing of gui fields, having to parse info in the tool code etc) Lookin at the code, it's in some serious need of refactoring. It would be nice with a method to get a "brief" os info for these kinds of tools that looks similar on all platforms. This is my suggested change: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ Seems to me some of this could be factored into the top-level OS class if we shoehorn Windows into the same shape as the other OSes ;-) This was my first attempt also, but then a lot of empty windows-methods ensued, which was kind of ugly. Or at least perhaps put some of the common stuff into os_posix.cpp ? There's a thought! I'll investigate that route, it could get things to look nicer. Cheers, David Regards, Nils Loodin
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Hey David! > Ignoring Windows (with prints very little) I'd say it is the printing of > /proc/meminfo that is the main difference. Not sure why printing that was > necessary ... but if we are going to remove it I think we need to know why it > was added. Yes, that's the reason. Note that nothing is removed. The method still prints exactly the same info, but I introduced another method to print briefer info, to be kinder to tool developers. I really don't want to change the output for say, hs_err files, where I believe this info is used. > >> This can make it hard for tool writers to get a summary that look good and >> similar for multiple platforms (sizing of gui fields, having to parse info >> in the tool code etc) >> Lookin at the code, it's in some serious need of refactoring. It would be >> nice with a method to get a "brief" os info for these kinds of tools that >> looks similar on all platforms. >> >> This is my suggested change: >> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ > > Seems to me some of this could be factored into the top-level OS class if we > shoehorn Windows into the same shape as the other OSes ;-) This was my first attempt also, but then a lot of empty windows-methods ensued, which was kind of ugly. > Or at least perhaps put some of the common stuff into os_posix.cpp ? There's a thought! I'll investigate that route, it could get things to look nicer. > Cheers, > David Regards, Nils Loodin
Re: RFR: 7165755 OS Information much longer on linux than other platforms
Hi Nils, On 2/05/2012 7:38 PM, Nils Loodin wrote: When looking at the info from os::print_os_info() in src/os/linux/vm/os_linux.cpp:2024 (and other files for other platforms) the length of the text is radically longer for linux than other platforms. Ignoring Windows (with prints very little) I'd say it is the printing of /proc/meminfo that is the main difference. Not sure why printing that was necessary ... but if we are going to remove it I think we need to know why it was added. This can make it hard for tool writers to get a summary that look good and similar for multiple platforms (sizing of gui fields, having to parse info in the tool code etc) Lookin at the code, it's in some serious need of refactoring. It would be nice with a method to get a "brief" os info for these kinds of tools that looks similar on all platforms. This is my suggested change: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ Seems to me some of this could be factored into the top-level OS class if we shoehorn Windows into the same shape as the other OSes ;-) Or at least perhaps put some of the common stuff into os_posix.cpp ? Cheers, David Regards, Nils Loodin
RFR: 7165755 OS Information much longer on linux than other platforms
When looking at the info from os::print_os_info() in src/os/linux/vm/os_linux.cpp:2024 (and other files for other platforms) the length of the text is radically longer for linux than other platforms. This can make it hard for tool writers to get a summary that look good and similar for multiple platforms (sizing of gui fields, having to parse info in the tool code etc) Lookin at the code, it's in some serious need of refactoring. It would be nice with a method to get a "brief" os info for these kinds of tools that looks similar on all platforms. This is my suggested change: http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/ Regards, Nils Loodin
