Add "resident-peak" memory measurement on Unix

RESOLVED FIXED in Firefox 39

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
This has been suggested several times recently. It can be read from the "VmHWM" entry in /proc/<pid>/status.
(Assignee)

Comment 1

2 years ago
Created attachment 8579805 [details] [diff] [review]
(part 1) - Test |residentUnique| in test_memoryReporters.xul

It is the only distinguished amount in nsIMemoryReporterManager that isn't
being tested.
Attachment #8579805 - Flags: review?(erahm)
(Assignee)

Comment 2

2 years ago
Created attachment 8579806 [details] [diff] [review]
(part 2) - Add a "resident-peak" distinguished amount and memory reporter on Linux
Attachment #8579806 - Flags: review?(erahm)
Doesn't getrusage give the same value in ru_maxrss?
(Assignee)

Comment 4

2 years ago
(In reply to Mike Hommey [:glandium] from comment #3)
> Doesn't getrusage give the same value in ru_maxrss?

Oh, I didn't know about getrusage. It gives the same result and is easier to use, so I'll switch.
(Assignee)

Comment 5

2 years ago
Created attachment 8579838 [details] [diff] [review]
(part 2) - Add a "resident-peak" distinguished amount and memory reporter on Linux

Now using getrusage().
Attachment #8579838 - Flags: review?(erahm)
(Assignee)

Updated

2 years ago
Attachment #8579806 - Attachment is obsolete: true
Attachment #8579806 - Flags: review?(erahm)
Note that getrusage is POSIX, you can also use it on OSX as well (or even #ifdef XP_UNIX, likely)
Comment on attachment 8579805 [details] [diff] [review]
(part 1) - Test |residentUnique| in test_memoryReporters.xul

Review of attachment 8579805 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8579805 - Flags: review?(erahm) → review+
Comment on attachment 8579838 [details] [diff] [review]
(part 2) - Add a "resident-peak" distinguished amount and memory reporter on Linux

Review of attachment 8579838 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. It would be nice to have this supported on OSX/BSD as well. Can you file a follow up for that?
Attachment #8579838 - Flags: review?(erahm) → review+
(Assignee)

Comment 9

2 years ago
> It would be nice to have this supported on OSX/BSD as well. Can you
> file a follow up for that?

I'll just do it in this patch, by moving the peak stuff in nsMemoryReporterManager.cpp into a XP_UNIX block.
(Assignee)

Updated

2 years ago
Summary: Add "resident-peak" memory measurement on Linux → Add "resident-peak" memory measurement on Unix
(Assignee)

Comment 10

2 years ago
Created attachment 8580322 [details] [diff] [review]
(part 2) - Add a "resident-peak" distinguished amount and memory reporter on Unix

ResidentPeakDistinguishedAmount() is now in the XP_UNIX section, and I'm
handling the different units for different Unices, and I'm handling the
possibility of 0, in case Solaris returns that.
Attachment #8580322 - Flags: review?(erahm)
(Assignee)

Updated

2 years ago
Attachment #8579838 - Attachment is obsolete: true
Comment on attachment 8580322 [details] [diff] [review]
(part 2) - Add a "resident-peak" distinguished amount and memory reporter on Unix

Review of attachment 8580322 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, one super minor suggestion.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +810,5 @@
> +ResidentPeakDistinguishedAmount(int64_t* aN)
> +{
> +  struct rusage usage;
> +  if (0 == getrusage(RUSAGE_SELF, &usage)) {
> +    // The units for ru_maxrrs:

nit: ru_maxrss

@@ +1923,5 @@
> +
> +/*static*/ int64_t
> +nsMemoryReporterManager::ResidentPeak()
> +{
> +#ifdef HAVE_RESIDENT_PEAK_REPORTER

min--: This could just call GetResidentPeak and drop the #ifdefs.
Attachment #8580322 - Flags: review?(erahm) → review+
(Assignee)

Comment 12

2 years ago
> > +/*static*/ int64_t
> > +nsMemoryReporterManager::ResidentPeak()
> > +{
> > +#ifdef HAVE_RESIDENT_PEAK_REPORTER
> 
> min--: This could just call GetResidentPeak and drop the #ifdefs.

I thought that too, but this one is static and GetResidentPeak() is not.
(Assignee)

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/228183a22063
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecc82272861
https://hg.mozilla.org/mozilla-central/rev/228183a22063
https://hg.mozilla.org/mozilla-central/rev/3ecc82272861
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.