Add |resident-unique| measurement to Windows

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

unspecified
mozilla45
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(1 attachment)

We currently have a 'private' measurement [1], but this is most likely not what we want (it seems to include the page-file, not just resident memory). It appears the Windows equivalent of USS is the "Private Working Set." This measurement is particularly useful for determining the impact of e10s with multiple content processes.

To measure this we can enumerate pages with |QueryWorkingSet| [2] and filter out shared pages.

[1] https://dxr.mozilla.org/mozilla-central/rev/faf815a0fa9b052a38bce00c0c2aa1e2c9610936/xpcom/base/nsMemoryReporterManager.cpp#318-327
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms684946%28v=vs.85%29.aspx
Blocks: 1222849
Blocks: 1198209
Created attachment 8687438 [details] [diff] [review]
Add |resident-unique| measurement to Windows
Attachment #8687438 - Flags: review?(n.nethercote)
Comment on attachment 8687438 [details] [diff] [review]
Add |resident-unique| measurement to Windows

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

r=me with the changes below. Thanks.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +546,5 @@
> +  }
> +
> +  DWORD infoArraySize = tmpSize + (entries * sizeof(PSAPI_WORKING_SET_BLOCK));
> +  PSAPI_WORKING_SET_INFORMATION* infoArray =
> +      (PSAPI_WORKING_SET_INFORMATION*)malloc(infoArraySize);

Use mozilla::ScopedFreePtr<> so you don't have to explicitly free() below.

You could also use static_cast<> here.

@@ +560,5 @@
> +
> +  entries = static_cast<size_t>(infoArray->NumberOfEntries);
> +  size_t privatePages = 0;
> +  for (size_t i = 0; i < entries; i++) {
> +    // Count shared pages that only one process is using as private

Nit: period at end of sentence.

@@ +578,5 @@
> +}
> +
> +class ResidentUniqueReporter final : public nsIMemoryReporter
> +{
> +  ~ResidentUniqueReporter() {}

This is a duplicate of the same class in the Linux-only code, right? Avoid the duplication by moving it outside the OS-specific section into a |#ifdef HAVE_RESIDENT_UNIQUE_REPORTER| block. For comparison, see VsizeMaxContiguousReporter, PrivateReporter, and others.
Attachment #8687438 - Flags: review?(n.nethercote) → review+

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26143abfbe1a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 6

3 years ago
How good of a candidate for uplift would this patch be? :vladan would like bug 1198209 uplifted, and it's not very useful without this one and bug 1223927.
Flags: needinfo?(erahm)
(In reply to Chris H-C :chutten from comment #6)
> How good of a candidate for uplift would this patch be? :vladan would like
> bug 1198209 uplifted, and it's not very useful without this one and bug
> 1223927.

It applies cleanly (after bug 1223927), we should uplift it.
Flags: needinfo?(erahm)
Comment on attachment 8687438 [details] [diff] [review]
Add |resident-unique| measurement to Windows

Approval Request Comment
[Feature/regressing bug #]: Resident unique reporter for Windows.
[User impact if declined]: Can't A/B test e10s on beta 44.
[Describe test coverage new/current, TreeHerder]: Baked on central for several weeks.
[Risks and why]: None.
[String/UUID change made/needed]: None.
Attachment #8687438 - Flags: approval-mozilla-aurora?

Comment 9

3 years ago
Comment on attachment 8687438 [details] [diff] [review]
Add |resident-unique| measurement to Windows

This has been on Nightly for 2 weeks and is needed for e10s A/B experiment. Aurora44+
Attachment #8687438 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

3 years ago
status-firefox44: --- → affected

Comment 10

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/4936d1c5c1df
status-firefox44: affected → fixed
You need to log in before you can comment on or make changes to this bug.