Closed Bug 988332 Opened 7 years ago Closed 7 years ago

Add USS & RSS reports to the Gecko Profiler

Categories

(Core :: Gecko Profiler, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: nbp, Assigned: nbp)

References

Details

(Keywords: perf, Whiteboard: [c=profiling p= s=2014.05.23.t u=])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Report RSS in Gecko's profiles. (obsolete) — Splinter Review
This patch should add RSS in the memory profiles.

I still have to check this patch.
Where could I add automated test for it?
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #8413880 - Flags: feedback?(bgirard)
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.
See Also: → 964096
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.
Attachment #8413880 - Flags: feedback?(bgirard) → feedback+
does not read the value in the signal code*
Duplicate of this bug: 964096
Keywords: perf
Priority: -- → P2
Whiteboard: [c=profiling p= s= u=]
(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.
Attachment #8413880 - Attachment is obsolete: true
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.
Attachment #8423544 - Flags: feedback?(bgirard)
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.
Attachment #8423544 - Flags: feedback?(bgirard) → feedback+
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
Attachment #8423544 - Attachment is obsolete: true
Attachment #8423633 - Flags: review?(bgirard)
Attachment #8423633 - Flags: review?(bgirard) → review+
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
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 }
https://hg.mozilla.org/mozilla-central/rev/e689f0c19d21
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 1013343
Whiteboard: [c=profiling p= s= u=] → [c=profiling p= s=2014.05.23.t u=]
Blocks: 1014071
You need to log in before you can comment on or make changes to this bug.