Closed Bug 1014071 Opened 6 years ago Closed 6 years ago

Add USS reports to the Gecko Profiler

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: nbp, Assigned: nbp)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file)

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.
Attachment #8426340 - Attachment description: Add support for USS on Linux. r= → Add support for USS on Linux.
Attachment #8426340 - Flags: review?(bgirard)
Depends on: 988332
Blocks: 976024
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.
Attachment #8426340 - Flags: review?(bgirard) → review+
Whiteboard: [MemShrink]
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?
Attachment #8426340 - Flags: review?(n.nethercote)
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.
(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.
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
Attachment #8426340 - Flags: review?(n.nethercote) → review+
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.
Flags: needinfo?(nicolas.b.pierron)
> 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!
Flags: needinfo?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/150dc6f78470
Assignee: nobody → nicolas.b.pierron
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 1035396
You need to log in before you can comment on or make changes to this bug.