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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

[Bob Owen (:bobowen)]

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

[1] https://searchfox.org/mozilla-central/rev/c52d5f8025b5c9b2b4487159419ac9012762c40c/xpcom/threads/ThreadEventQueue.cpp#207
[2] https://searchfox.org/mozilla-central/rev/c52d5f8025b5c9b2b4487159419ac9012762c40c/dom/storage/StorageDBThread.cpp#483

Comment 1

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

Thank you Bob.

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

Comment 2

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

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

Comment 3

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

Attachment: Bug 1609907 - Remove AUTO_PROFILER_THREAD_SLEEP before mozilla::CondVar waits - r?mstange

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

Comment 4

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

Attachment: Bug 1609907 - Add AUTO_PROFILER_THREAD_SLEEP around wait_for in DEBUG OffTheBooksCondVar::Wait - r?mstange

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91ff8fda2596
Remove AUTO_PROFILER_THREAD_SLEEP before mozilla::CondVar waits - r=mstange
https://hg.mozilla.org/integration/autoland/rev/84811ffff4f3
Add AUTO_PROFILER_THREAD_SLEEP around wait_for in DEBUG OffTheBooksCondVar::Wait - r=mstange

Comment 6

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/91ff8fda2596
https://hg.mozilla.org/mozilla-central/rev/84811ffff4f3