1561881 - We should capture memory counters even when the "memory" feature is unchecked

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P3)

Description

[Julien Wajsberg [:julienw]]

Since bug 1520238 we don't capture memory counters unless the "memory" feature is enabled. We should capture them even when it's disabled because they're super cheap.

For context, the "memory" feature is to capture rss/uss which we don't use. We should remove it altogether.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Sorry for having inadvertently changed this behavior, I thought it was a good idea then! (to control memory counting with the "memory" feature.)

Alternatively, you could enable "memory" by default (move it from StartupExtraDefaultFeatures() to DefaultFeatures() in platform.cpp).

And we already have bug 1521929 to remove Rss&USS collection.
As a first step here you could just #ifdef 0-out the bit of RSS&USS collection in SamplerThread::Run().
Happy to help.

But if you think we're not going to turn off "memory" ever again, I guess your suggestion works too.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: Bug 1561881 - Always capture the memory counters when profiling

Comment 3

[Florian Quèze [:florian]]

(In reply to Gerald Squelart [:gerald] from comment #1)

But if you think we're not going to turn off "memory" ever again, I guess your suggestion works too.

My assumptions were that:

• memory counters are cheap so we want them enabled all the time;
• rss/uss is unused and expensive, and should be removed;
• we would like to keep the memory feature (disabled by default) to collect memory allocation stacks, which can add overhead. Greg is working on capturing allocation stacks and expects to land some of this soon.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you for the details, good to know the memory feature flag will live on! (Guessing that's Greg's DMD work.)

Comment 5

[Julien Wajsberg [:julienw]]

Alternatively, you could enable "memory" by default (move it from StartupExtraDefaultFeatures() to DefaultFeatures() in platform.cpp).

I don't think we want to, as that's expensive :)

we would like to keep the memory feature (disabled by default) to collect memory allocation stacks, which can add overhead. Greg is working on capturing allocation stacks and expects to land some of this soon.

Good point!

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Julien Wajsberg [:julienw] from comment #5)

Alternatively, you could enable "memory" by default (move it from StartupExtraDefaultFeatures() to DefaultFeatures() in platform.cpp).

I don't think we want to, as that's expensive :)

Sorry for the confusion. I was suggesting we enable "memory" by default to collect memory counters, but not RSS&USS anymore.

Anyway, the new solution (always collect memory counters, keep memory feature for upcoming DMD stuff) sounds good.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05f2566c282a
Always capture the memory counters when profiling r=gerald

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/05f2566c282a