[RFR] 8202427: Enhance os::print_memory_info on Windows
Baesken, Matthias
matthias.baesken at sap.com
Fri May 18 15:10:48 UTC 2018
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