Closed Bug 1592887 Opened 5 years ago Closed 5 years ago

Base Profiler profile is deleted before it can be used

Categories

(Core :: Gecko Profiler, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

When Gecko Profiler starts, it retrieves the existing profile from the Base Profiler (if it is running) and stores it, until the Gecko Profiler itself becomes full, at which point the Base profile is discarded (because it implicitly is older than the oldest Gecko data).

The test in ClearExpiredExitProfiles() just checks if the Gecko profile range doesn't include 0 (zero) anymore, which means we have started to overwrite the oldest Gecko data.

In bug 1576551, we started using the new buffer storage BlocksRingBuffer.
For convenience, BlocksRingBuffer considers index 0 to be special (similar to a nullptr), and so the first valid entry starts at 1.

This means that the very first time we check for expired data, the buffer range is already starting at 1 (and not 0), so we just discard the Base profile!

The buffer range starts at 1 (the first valid entry, 0 is reserved as
null marker). So if it now starts after 1, it means we have started to
overwrite our oldest data, and we should get rid of Base profiles if any.

This bug is only fixing the issue.
I've added a note in bug 1586939 to add regression tests so that this doesn't happen again (or at least gets noticed quickly).

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b96cb18aa920
Gecko Profiler range starts at 1, discard Base Profile when >1 - r=florian
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

I assume we're not particularly worried about this bug on Beta71 at this point, but feel free to nominate for uplift if you feel strongly that we do.

Thank you Ryan. Nobody noticed the issue for a while, it's a still-half-secret feature for Gecko devs, so I don't think we need to uplift.

Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: