988332 - Add USS & RSS reports to the Gecko Profiler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

[Nicolas B. Pierron [:nbp]]

We should add a new reporting kind "memory" which reports the usage of the program around each sample.  This would be useful to associate memory bump with pieces of code.

Our interest was to track how many pages are mutated by GC and caused a process on B2G to make a copy of an existing page.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Report RSS in Gecko's profiles.

This patch should add RSS in the memory profiles.

I still have to check this patch.
Where could I add automated test for it?

Comment 2

[Benoit Girard (:BenWa)]

This bug is very similar to bug 964096. However the patch here is better since it does read the value in the signal code.

There a coming heap profiling tool that will give better measurement about memory usage. However I still think it's worth taking this change because it's small, simple and can be correlated with samples.

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 8413880 [details] [diff] [review]
Report RSS in Gecko's profiles.

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

Looks great, just need to write out the value along with the sample in ThreadProfile::StreamJSObject. Also this should only be done for Sampler that belong to the main thread otherwise we're just going to track redundant memory usage data.

Comment 4

[Benoit Girard (:BenWa)]

does not read the value in the signal code*

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Benoit Girard (:BenWa) from comment #4)
> does not read the value in the signal code*

Should I move the collection of the RSS to the SignalSender, instead of the SignalHandler?
I am not sure to see where I can collect it else where.

And as a late reply from irc, I have no ETA for it yet.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Report RSS in Gecko's profiles. r=

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8423544 [details] [diff] [review]
Report RSS in Gecko's profiles. r=

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

This patch move the collection outside of the signal handler, and outside the sections where the thread is blocked.
I still have to add a filter to only report it when we are on the main thread.

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 8423544 [details] [diff] [review]
Report RSS in Gecko's profiles. r=

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

Good to land once we only record it for the main thread. Actually you should only record it for the first thread that is signaled since we may not be profiling the main thread.

::: tools/profiler/ProfileEntry.h
@@ +127,5 @@
>    int            mGeneration;
>    int            mPendingGenerationFlush;
>    void* const    mStackTop;
> +
> +  // Only Linux is using a singal sender, instead of stopping the thread, so we

signal* twice

::: tools/profiler/platform-linux.cc
@@ +319,5 @@
>  
> +        // Profile from the singal sender for information which is not signal
> +        // safe, and will have low variation between the emission of the signal
> +        // and the signal handler catch.
> +        ProfilerSignalThread(sCurrentThreadProfile);

Ohh this kind of sucks that we have to stick it on the ThreadProfile for linux only. The right thing to do would be to replace sCurrentThreadProfile by a struct that would contain the ThreadProfile and this kind of information. mRssMemory doesn't belong in ThreadProfile really.

But I'll allow it and probably just clean it up in the next refactoring of the profiler I do.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Report RSS in Gecko's profiles.

Delta:
 - Add flag to detect if the current profiled thread is the first profiled
 thread, to record resident memory only once per profile.
 - s/singal/signal/g

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8423633 [details] [diff] [review]
Report RSS in Gecko's profiles.

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

::: tools/profiler/TableTicker.cpp
@@ +654,5 @@
>    }
>  
> +  // rssMemory is equal to 0 when we are not recording.
> +  if (sample && sample->rssMemory == 0) {
> +    currThreadProfile.addTag(ProfileEntry('R', static_cast<float>(sample->rssMemory)));

self-nit: the above condition should be
    sample->rssMemory != 0

Comment 12

[Nicolas B. Pierron [:nbp]]

I tried it inside the devtools of the browser, and this seems to be working so far:

> var profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> profiler.StartProfiler(10000000, 1, ["memory"], 1);
> var profileObj = profiler.getProfileData();
> profileObj.threads[0]
> * Object { name: "GeckoMain", tid: 10017, samples: Array[16359], markers: Array[28] }
> profileObj.threads[0].samples[0]
> * Object { frames: Array[8], responsiveness: 61.…, time: 58.…, rss: 527671296 }
> profileObj.threads[0].samples[1]
> * Object { frames: Array[8], responsiveness: 62.…, time: 59.…, rss: 527675392 }
> profileObj.threads[0].samples[2]
> * Object { frames: Array[9], responsiveness: 63.…, time: 60.…, rss: 527675392 }

Comment 13

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e689f0c19d21

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e689f0c19d21