Closed Bug 1561881 Opened 5 months ago Closed 5 months ago

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

Categories

(Core :: Gecko Profiler, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

(Regression)

Details

Attachments

(1 file)

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.

Assignee: nobody → felash

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.

Type: defect → enhancement
Priority: -- → P3

(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.

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

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!

(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.

Pushed by jwajsberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05f2566c282a
Always capture the memory counters when profiling r=gerald
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.