1224685 - Add |resident-unique| measurement to Windows

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

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

Comment 1

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b2114d84c82

Comment 2

[Eric Rahm [:erahm]]

Attachment: Add |resident-unique| measurement to Windows

Comment 3

[Nicholas Nethercote [inactive]]

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.

Comment 4

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26143abfbe1a2bea884e3c92cf6e561fb33b635a
Bug 1224685 - Add |resident-unique| measurement to Windows. r=njn

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26143abfbe1a

Comment 6

[Chris H-C :chutten]

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.

Comment 7

[Eric Rahm [: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.

Comment 8

[Eric Rahm [: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.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4936d1c5c1df

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4936d1c5c1df