[RFR] 8202427: Enhance os::print_memory_info on Windows

Thomas Stüfe thomas.stuefe at gmail.com
Sat May 19 05:24:06 UTC 2018 

Hi Matthias,

sounds good.

..Thomas

On Fri, May 18, 2018 at 5:10 PM, Baesken, Matthias
<matthias.baesken at sap.com> wrote:
> Hi Thomas ,  thanks  looking into it.
> Indeed  GetProcessMemoryInfo   could fail ,  and  GetProcessMemoryInfo   could fail as well .
>
> MSDN says for both functions .
>
> https://msdn.microsoft.com/de-de/library/windows/desktop/ms683219(v=vs.85).aspx
>
> https://msdn.microsoft.com/de-de/library/windows/desktop/aa366589(v=vs.85).aspx
>
>   ..........
>
> Return value
> If the function succeeds, the return value is nonzero.
> If the function fails, the return value is zero.
>
>
> I add  return code checking for failure cases .   (plus some line feeds in the coding which is desired by Goetz ).
>
>
> For  GlobalMemoryStatusEx  there seem to be  quite a few places  where return code checking is missing ,  this should be addressed in a follow up change :
>
>
> OperatingSystemImpl.c
> 103 GlobalMemoryStatusEx(&ms);
> 113 GlobalMemoryStatusEx(&ms);
> 142 GlobalMemoryStatusEx(&ms);
> 152 GlobalMemoryStatusEx(&ms);
>
>
> os_windows.cpp
> 736 GlobalMemoryStatusEx(&ms);
> 748 GlobalMemoryStatusEx(&ms);
> 1748 GlobalMemoryStatusEx(&ms);
> 3669 GlobalMemoryStatusEx(&ms);
>
>
> Best regards, Matthias
>
>> -----Original Message-----
>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
>> Sent: Freitag, 18. Mai 2018 08:16
>> To: Baesken, Matthias <matthias.baesken at sap.com>
>> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Doerr, Martin
>> <martin.doerr at sap.com>; hotspot-dev at openjdk.java.net; hotspot-
>> runtime-dev at openjdk.java.net
>> Subject: Re: [RFR] 8202427: Enhance os::print_memory_info on Windows
>>
>> Hi Matthias,
>>
>> I took another closer look and have some minor nits. Sorry for not
>> looking better the first time, my head was not clear :)
>>
>> Since that was my fault I leave it up to you if you follow up. The fix
>> in its current form is also fine to me.
>>
>> - I am not a big fan of ">> 20". I would change that to " / M" to make
>> the code more readable.
>> - Strictly speaking GetProcessMemoryInfo could fail (I have never seen
>> that though) and return FALSE. In which case we should probably not
>> output the results.
>>
>> Best Regards, Thomas
>>
>>
>> On Thu, May 17, 2018 at 5:20 PM, Baesken, Matthias
>> <matthias.baesken at sap.com> wrote:
>> >> > > I also think that one number is enough. Adaptive numbers would be
>> great.
>> >
>> >
>> > Hi  Thomas/Goetz/Martin  ,  thanks for looking into it.
>> > Adaptive numbers could be done  in a follow-up as suggested by Thomas ,
>> this would also touch the other platforms .
>> >
>> > I added more line feeds to the output  and  reduced output to just printing
>> M  .
>> >
>> >
>> > Example output from a Windows desktop PC :
>> >
>> > Memory: 4k page, system-wide physical 32520M (20165M free)
>> > TotalPageFile size 34568M (AvailPageFile size 18170M)
>> > current process WorkingSet (physical memory assigned to process): 79M,
>> peak: 79M
>> > current process commit charge ("private bytes"): 148M, peak: 148M
>> >
>> >
>> > new webrev :
>> >
>> > http://cr.openjdk.java.net/~mbaesken/webrevs/8202427.1/
>> >
>> >
>> > Best regards, Matthias
>> >
>> >
>> >
>> >> -----Original Message-----
>> >> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
>> >> Sent: Mittwoch, 16. Mai 2018 16:35
>> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
>> >> Cc: Baesken, Matthias <matthias.baesken at sap.com>; Doerr, Martin
>> >> <martin.doerr at sap.com>; hotspot-dev at openjdk.java.net; hotspot-
>> >> runtime-dev at openjdk.java.net
>> >> Subject: Re: [RFR] 8202427: Enhance os::print_memory_info on Windows
>> >>
>> >> On Wed, May 16, 2018 at 3:48 PM, Lindenmaier, Goetz
>> >> <goetz.lindenmaier at sap.com> wrote:
>> >> > Hi Matthias,
>> >> >
>> >> > I appreciate the additional information in the hs_err file.
>> >> > Could you please add an example output (of the final version) to the
>> bug
>> >> description?
>> >> >
>> >> > As Martin, I would like more line feeds, both in the code and the output.
>> >> > Currently, the output in the hs_err file looks like this (where the first
>> line is
>> >> too long):
>> >> >     Memory: 4k page, system-wide physical 16776692k [16383M]
>> (8795032k
>> >> [8588M] free), TotalPageFile size 49949460k [48778M] (AvailPageFile size
>> >> 38942152k [38029M]), user-mode portion of virtual address-space
>> 2097024k
>> >> [2047M] (6940k [6M] free)
>> >> >     current process WorkingSet (physical memory assigned to process):
>> >> 991804k [968M], peak: 991804k [968M]
>> >> >
>> >> >     current process commit charge ("private bytes"): 1523632k [1487M],
>> >> peak: 1523632k [1487M]
>> >> >
>> >> > I also think that one number is enough. Adaptive numbers would be
>> great.
>> >> > I know about
>> >> >     src/hotspot/share/memory/metaspace/metaspaceCommon.hpp:
>> >> print_human_readable_size()
>> >> > for this, but this seems a bit hidden in metaspace.
>> >> >
>> >>
>> >> Guys, lets not worry about human readable numbers for now. We can
>> move
>> >> print_human_readable_size() out from metaspace to something generic
>> >> and use it to improve this code (and others) in a later patch. That
>> >> was my original intention when adding print_human_readable_size().
>> >>
>> >> The patch is good. Thanks for doing this.
>> >>
>> >> ..Thoams
>> >>
>> >> > I don't like usage of _M_IX86. Common in this file seems #ifndef
>> _WIN64
>> >> for 32-bit windows
>> >> > dependent code.  But don't care here.
>> >> >
>> >> > Best regards,
>> >> >   Goetz.
>> >> >
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net]
>> On
>> >> >> Behalf Of Baesken, Matthias
>> >> >> Sent: Mittwoch, 16. Mai 2018 15:13
>> >> >> To: Doerr, Martin <martin.doerr at sap.com>; 'hotspot-
>> >> >> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>; hotspot-
>> >> >> runtime-dev at openjdk.java.net
>> >> >> Subject: RE: [RFR] 8202427: Enhance os::print_memory_info on
>> Windows
>> >> >>
>> >> >> Ping :   could I get a second review ?
>> >> >>
>> >> >> Bug :
>> >> >> https://bugs.openjdk.java.net/browse/JDK-8202427
>> >> >> Change :
>> >> >> http://cr.openjdk.java.net/~mbaesken/webrevs/8202427.0/
>> >> >>
>> >> >>
>> >> >> > We could also  just print  the  MB  value  , let's see what other think.
>> >> >> > Another option might be to have a flexible output   (kB for  smaller
>> >> >> memory
>> >> >> > values , MB   (or GB)  for larger ) ?
>> >> >>
>> >> >> Martin suggested to just print  the  MB values,   what do you think ?
>> >> >>
>> >> >>
>> >> >> Thanks, Matthias
>> >> >>
>> >> >>
>> >> >> > -----Original Message-----
>> >> >> > From: Baesken, Matthias
>> >> >> > Sent: Mittwoch, 2. Mai 2018 13:00
>> >> >> > To: Doerr, Martin <martin.doerr at sap.com>; 'hotspot-
>> >> >> > dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>; hotspot-
>> >> >> > runtime-dev at openjdk.java.net
>> >> >> > Subject: RE: [RFR] 8202427: Enhance os::print_memory_info on
>> >> Windows
>> >> >> >
>> >> >> > Hi Martin,  thanks for your input .
>> >> >> > I add  hotspot-runtime-dev  .
>> >> >> >
>> >> >> > >
>> >> >> > > I wonder if we really need the sizes in kB in addition to MB. Maybe
>> >> other
>> >> >> > > reviewers would like to comment on this, too.
>> >> >> > >
>> >> >> >
>> >> >> > We could also  just print  the  MB  value  , let's see what other think.
>> >> >> > Another option might be to have a flexible output   (kB for  smaller
>> >> >> memory
>> >> >> > values , MB   (or GB)  for larger ) ?
>> >> >> >
>> >> >> > Best regards, Matthias
>> >> >> >
>> >> >> >
>> >> >> > > -----Original Message-----
>> >> >> > > From: Doerr, Martin
>> >> >> > > Sent: Mittwoch, 2. Mai 2018 12:53
>> >> >> > > To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>> >> >> > > dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
>> >> >> > > Subject: RE: [RFR] 8202427: Enhance os::print_memory_info on
>> >> Windows
>> >> >> > >
>> >> >> > > Hi Matthias,
>> >> >> > >
>> >> >> > > looks like a nice enhancement. We can get substantially more
>> >> >> information.
>> >> >> > >
>> >> >> > > I wonder if we really need the sizes in kB in addition to MB. Maybe
>> >> other
>> >> >> > > reviewers would like to comment on this, too.
>> >> >> > >
>> >> >> > > We should have line breaks.
>> >> >> > >
>> >> >> > > Best regards,
>> >> >> > > Martin
>> >> >> > >
>> >> >> > >
>> >> >> > > -----Original Message-----
>> >> >> > > From: hotspot-dev [mailto:hotspot-dev-
>> bounces at openjdk.java.net]
>> >> On
>> >> >> > > Behalf Of Baesken, Matthias
>> >> >> > > Sent: Montag, 30. April 2018 16:53
>> >> >> > > To: 'hotspot-dev at openjdk.java.net' <hotspot-
>> >> dev at openjdk.java.net>
>> >> >> > > Subject: [RFR] 8202427: Enhance os::print_memory_info on
>> Windows
>> >> >> > >
>> >> >> > > On Windows,
>> >> >> > > the os::print_memory_info  misses a few  memory-related infos
>> that
>> >> >> might
>> >> >> > > be interesting :
>> >> >> > > - current and peak WorkingSet size  (= physical memory assigned to
>> >> the
>> >> >> > > process)
>> >> >> > > - current and peak commit charge  (also known as "private bytes" in
>> >> >> > Windows
>> >> >> > > tools)
>> >> >> > > - on 32bit Windows :
>> >> >> > > user-mode portion/free user mode portion  of virtual address-
>> space
>> >> >> > > (Total/AvailVirtual) because it shows how close do we get to the 2-
>> 4
>> >> GB
>> >> >> per
>> >> >> > > process border.
>> >> >> > >
>> >> >> > >
>> >> >> > > - the current naming of "swap/free-swap" memory is a bit
>> misleading;
>> >> >> > > in the Windows world swap is a special page file used for UWP
>> apps.
>> >> >> > > (see Windows Internals :  "The swap file"  (chapter 5 Memory
>> >> >> > management)
>> >> >> > > Windows 8 added another page file called a swap file. It is ...
>> >> exclusively
>> >> >> > used
>> >> >> > > for UWP (Universal Windows Platform) apps.
>> >> >> > > Our output (ullTotalPageFile and ullAvailPageFile) is NOT about the
>> >> UWP
>> >> >> > > related values, it is about page file sizes
>> >> >> > > (so calling it TotalPageFile/AvailPageFile in "Windows-speak" or just
>> >> >> virtual
>> >> >> > > memory might be more appropriate).
>> >> >> > >
>> >> >> > >
>> >> >> > > https://msdn.microsoft.com/de-
>> >> >> > > de/library/windows/desktop/aa366770(v=vs.85).aspx
>> >> >> > >
>> >> >> > > documents it in the following way:
>> >> >> > > ullTotalPageFile:
>> >> >> > > The current committed memory limit for the system or the current
>> >> >> process,
>> >> >> > > whichever is smaller,...
>> >> >> > >
>> >> >> > > ullAvailPageFile :
>> >> >> > > The maximum amount of memory the current process can commit,
>> in
>> >> >> > bytes.
>> >> >> > > This value is equal to or smaller than the system-wide available
>> >> commit
>> >> >> > value
>> >> >> > >
>> >> >> > >
>> >> >> > >
>> >> >> > > Aditionally I suggest having output of the various memory-values
>> in M
>> >> >> > > (megabyte) as well , the k  (kilobyte) output sometimes gives very
>> >> high
>> >> >> and
>> >> >> > > unreadable numbers).
>> >> >> > >
>> >> >> > >
>> >> >> > > Could you please  review my change ?
>> >> >> > >
>> >> >> > >
>> >> >> > > Bug :
>> >> >> > >
>> >> >> > > https://bugs.openjdk.java.net/browse/JDK-8202427
>> >> >> > >
>> >> >> > >
>> >> >> > > Change :
>> >> >> > >
>> >> >> > > http://cr.openjdk.java.net/~mbaesken/webrevs/8202427.0/
>> >> >> > >
>> >> >> > >
>> >> >> > > Thanks, Matthias
>> >> >> > >
>> >> >

More information about the hotspot-dev mailing list