Closed Bug 1609907 Opened 11 months ago Closed 7 months ago

Double call to profiler_thread_sleep/wake in ThreadEventQueue::GetEvent on opt builds


(Core :: Gecko Profiler, defect, P2)




Tracking Status
firefox-esr68 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- fixed


(Reporter: bobowen, Assigned: gerald)




(Keywords: regression)


(2 files)

I happened to spot this when my debugger landed on the assembly for this.

In opt builds CondVar::Wait already uses AUTO_PROFILER_THREAD_SLEEP, so using it at [1] causes profiler_thread_sleep/wake to be called twice in succession.
(Looks like these get inlined in PGO builds on Windows.)

There are MOZ_ASSERTs in SetSleeping and SetAwake to try and catch this, but as CondVar::Wait doesn't use it in debug builds they are not triggered.

I suppose the question is should we just remove the one at [1] or the one in CondVar::Wait.
If we remove the one at [1] then the CondVar::Wait one should also be defined in debug builds, to catch any other instances of this ... the first other use I looked at had the same problem [2].


Flags: needinfo?(mstange)
Flags: needinfo?(mstange) → needinfo?(gsquelart)
Keywords: regression
Regressed by: 1503468

Thank you Bob.

Keeping the NI:myself to come back to this when possible in the next couple of weeks...

Priority: -- → P2

There are more intermittent failures around MOZ_ASSERT(mSleep == AWAKE) (e.g., bug 1530419), so I should fix this one here and see if that helps...

Assignee: nobody → gsquelart
Blocks: 1530419
Flags: needinfo?(gsquelart)
No longer blocks: 1530419
Depends on: 1530419

CondVar already calls AUTO_PROFILER_THREAD_SLEEP, which shouldn't be called recursively.

mozilla::Monitor also uses CondVar, so it shouldn't be surrounded by AUTO_PROFILER_THREAD_SLEEP either.

Depends on D72850

In non-DEBUG builds, OffTheBooksCondVar::Wait has AUTO_PROFILER_THREAD_SLEEP, but it's not in DEBUG builds.
profiler_thread_sleep does a MOZ_ASSERT(mSleep == AWAKE) in DEBUG builds, so the double call that happens in non-DEBUG wouldn't trigger the assertion!

This patch adds AUTO_PROFILER_THREAD_SLEEP in DEBUG OffTheBooksCondVar::Wait, to catch more misuses during development.

Depends on D72851

Pushed by
Remove AUTO_PROFILER_THREAD_SLEEP before mozilla::CondVar waits - r=mstange
Add AUTO_PROFILER_THREAD_SLEEP around wait_for in DEBUG OffTheBooksCondVar::Wait - r=mstange
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
You need to log in before you can comment on or make changes to this bug.