1014071 - Add USS reports to the Gecko Profiler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nicolas B. Pierron [:nbp]]

Bug 988332 was supposed to add both, but I preferred to make only a patch
for RSS as we already had a cross-platform way to handle RSS, but no fast
cross-platform way to handle USS.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Add support for USS on Linux.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8426340 [details] [diff] [review]
Add support for USS on Linux.

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

::: tools/profiler/platform-macos.cc
@@ +245,5 @@
>        sample->rssMemory = nsMemoryReporterManager::ResidentFast();
>      } else {
>        sample->rssMemory = 0;
>      }
> +    sample->ussMemory = 0;

Can you add a comment here and win32 saying that it's not supported on that platform?

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +60,5 @@
>    return NS_ERROR_FAILURE;
>  }
>  
> +static nsresult
> +GetProcSelfSmapsPrivate(int64_t* aN)

Maybe you should get someone from Memshrink to review this? I could check all the man pages but I'd likely still miss a gotchas.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8426340 [details] [diff] [review]
Add support for USS on Linux.

njn: Is the current approach good enough for the sampling profiler? Is there any efficient way to get the USS on Mac and Windows?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8426340 [details] [diff] [review]
Add support for USS on Linux.

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

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +69,5 @@
> +  // 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.
> +
> +  FILE* f = fopen("/proc/self/smaps", "r");

FYI: the implementation of /proc/*/smaps walks the process's page table and inspects each page table entry (and per-physical page structure, if present).  I don't know if it potentially costs enough CPU time to be a concern.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Jed Davis [:jld] from comment #4)
> Comment on attachment 8426340 [details] [diff] [review]
> Add support for USS on Linux.
> 
> Review of attachment 8426340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nsMemoryReporterManager.cpp
> @@ +69,5 @@
> > +  // 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.
> > +
> > +  FILE* f = fopen("/proc/self/smaps", "r");
> 
> FYI: the implementation of /proc/*/smaps walks the process's page table and
> inspects each page table entry (and per-physical page structure, if
> present).  I don't know if it potentially costs enough CPU time to be a
> concern.

Knowing that we might also iterate over the stack of multiple threads, I guess this is about the same order of magnitude. Also note that this is an optional probe, which is only enabled when we use the "memory" profile.

I don't know to which extend we can call it fast :/

I guess, later we can split it in "fast-memory" profile & "slow-memory" profile if this is not practical on B2G.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8426340 [details] [diff] [review]
Add support for USS on Linux.

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

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1534,5 @@
>  }
>  
> +#if defined(XP_LINUX)
> +/*static*/
> +int64_t

Nit: move the int64_t up to the same line as the /* static */. Bonus points if you do it for ResidentFast() as well.

@@ +1538,5 @@
> +int64_t
> +nsMemoryReporterManager::ResidentUniqueFast()
> +{
> +  int64_t amount = 0;
> +  GetProcSelfSmapsPrivate(&amount);

Check this for failure, and return 0 in the failure case. And please do likewise for ResidentFast()!

::: xpcom/base/nsMemoryReporterManager.h
@@ +152,5 @@
>  
> +#if defined(XP_LINUX)
> +  // Convenience function to get USS easily from other code.  This is useful
> +  // when debugging unshared memory pages for forked processes.
> +  static int64_t ResidentUniqueFast();

Just call it ResidentUnique() -- unlike ResidentFast(), there isn't a non-fast alternative.

Comment 7

[Nicolas B. Pierron [:nbp]]

https://tbpl.mozilla.org/?tree=Try&showall=1&rev=6c45c95a0c82

Comment 8

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/150dc6f78470

Comment 9

[Jan Keromnes [:janx]]

Nicolas, what are your thoughts on exposing `ResidentUnique()` to JavaScript with nsIMemoryReporter.idl? This would allow me to query USS from the devtools memory actor for bug 976024.

Comment 10

[Nicholas Nethercote [inactive]]

> Nicolas, what are your thoughts on exposing `ResidentUnique()` to JavaScript
> with nsIMemoryReporter.idl? This would allow me to query USS from the
> devtools memory actor for bug 976024.

(Nicholas here, not Nicolas)

That sounds fine. If you write such a patch please ask me to review it. Thanks!

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/150dc6f78470