1476168 - Update /proc/self/smaps consumers to use MemoryMapping.h, and parse once per memory reporter cycle

Status: NEW

(Core :: XPCOM, enhancement)

Description

[Kris Maglione [:kmag]]

There are currently a bunch of places where we read /proc/self/smaps or /maps that should probably be updated to use the MemoryMapping.h helper instead.

More importantly, we have two separate places where we read /proc/self/smaps during each memory reporting cycle, and may need more in the future. We should really have a helper to read and parse it once per cycle, and allow different reporters to use the cached data as necessary.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1476168: Only parse /proc/self/smaps once per memory reporter cycle.

Review commit: https://reviewboard.mozilla.org/r/258950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258950/

Comment 2

[Eric Rahm [:erahm]]

Comment on attachment 8994377 [details]
Bug 1476168: Only parse /proc/self/smaps once per memory reporter cycle.

https://reviewboard.mozilla.org/r/258950/#review266490

::: xpcom/base/nsMemoryReporterManager.cpp:109
(Diff revision 1)
>    // statm's "shared" value actually counts pages backed by files, which has
>    // little to do with whether the pages are actually shared. /proc/self/smaps
>    // on the other hand appears to give us the correct information.
>  
> -  nsTArray<MemoryMapping> mappings(1024);
> -  MOZ_TRY(GetMemoryMappings(mappings));
> +  auto* mgr = nsMemoryReporterManager::GetOrCreate();
> +  const auto& mappings = mgr->GetMemoryMappings();

This isn't quite right. You can query this value directly outside of a `CollectReports` call via [nsIMemoryReporterManager.residentUnique](https://searchfox.org/mozilla-central/source/xpcom/base/nsIMemoryReporter.idl#388). Multiple calls would all return the same cached value.

What I was envisioning is merging the `ThreadsReporter` and the `ResidentUniqueReporters` (at least on Linux), and then we could pass in `mappings` to `GetProcSelfSmapsPrivate`.

::: xpcom/base/nsMemoryReporterManager.cpp:1850
(Diff revision 1)
>    // be called from the main thread.
>    if (!NS_IsMainThread()) {
>      MOZ_CRASH();
>    }
>  
> +  ClearMemoryMappings();

Having to add all these in is pretty tedious and error prone. In this case we should not clear if `mPendingProcessesState`.

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8994377 [details]
Bug 1476168: Only parse /proc/self/smaps once per memory reporter cycle.

https://reviewboard.mozilla.org/r/258950/#review266490

> This isn't quite right. You can query this value directly outside of a `CollectReports` call via [nsIMemoryReporterManager.residentUnique](https://searchfox.org/mozilla-central/source/xpcom/base/nsIMemoryReporter.idl#388). Multiple calls would all return the same cached value.
> 
> What I was envisioning is merging the `ThreadsReporter` and the `ResidentUniqueReporters` (at least on Linux), and then we could pass in `mappings` to `GetProcSelfSmapsPrivate`.

Yeah, I realized that when I was looking at bug 1477662. :/

Comment 4

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.