Open Bug 1476168 Opened 4 years ago Updated 1 year ago

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

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

People

(Reporter: kmag, Assigned: kmag)

Details

Attachments

(1 file)

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.
Blocks: 1477662
No longer blocks: 1477662
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`.
Attachment #8994377 - Flags: review?(erahm) → review-
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. :/
You need to log in before you can comment on or make changes to this bug.