Reduce impact of ThreadRegistry locking in sampler thread
Categories
(Core :: Gecko Profiler, task, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox99 | --- | fixed |
People
(Reporter: mozbugz, Assigned: mozbugz)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Example profile: https://share.firefox.dev/3sFjL36
"FileIO (non-main thread)" markers are generated from non-main threads (in this case "IndexDB #21", tid 2405483). But they target the main thread, so the marker code needs to check if that target thread is actively being sampled. This involves locking the ThreadRegistry, in order to access the thread data including its current is-being-profiled state (the locking is necessary to prevent that thread from ending, which would destroy the data we're trying to read).
Meanwhile the sampler thread regularly runs its sampling loop, which also locks the ThreadRegistry in order to sample all to-be-sampled threads, and this takes a fairly significant time.
In the example profile, notice how the FileIO marker row has gaps (around 300-600 microseconds each) around each sampling loop (the small blue boxes in the "Parent Process" track above).
And presumably, this long locking in the sampler could block other similar operations that need to access another thread's data.
I can think of a few possible solutions:
-
The sampler thread could unlock the
ThreadRegistrybetweeen sampling each thread. This would allow other threads to do their own work, like writing these FileIO markers.
However, this would be more complex to handle in the sampler, since it would need to be able to deal with the list of threads potentially changing, and so it would need to keep track of which threads have been sampled so far, and find the next candidate (if any) to continue sampling. -
Change the
ThreadRegistrylocking mechanism to a readers-writer lock.
This would allow simultaneous reads (to access individualThreadRegistrationobjects), in our case from the sampler and from markers, while preventing writes (to add/removeThreadRegistrations to/from the list) when a thread starts or stops. -
Add locks around
ThreadRegistrationobjects, that would at least prevent their destruction -- Maybe through a thread-safeRefPtr? This would allow the sampler to lock theThreadRegistryfor only a short time, just to read the list and lock/reference individualThreadRegistrations to be sampled.
Option #2 feels like the easiest, and could be done using mozilla::RWLock or std::shared_mutex.
|
Assignee | |
Comment 1•2 years ago
|
||
This is a profiler-specific shared lock (aka readers-writer lock) implemented on top of RWLockImpl.
Similar to BaseProfilerMutex, it records which thread is currently holding the exclusive lock.
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 2•2 years ago
|
||
This allows multiple threads to access ThreadRegistrations through the thread registry, as long as the registry itself is not modified. So in particular, ThreadRegistrations are guaranteed to stay alive as long as a non-exclusive lock is held.
This ensures cross-thread markers are not blocked while the sampler is running.
Depends on D139916
Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/169754a1f337 BaseProfilerSharedMutex and exclusive&shared RAII locks - r=canaltinova https://hg.mozilla.org/integration/autoland/rev/d758cab0d5cf Lock the ThreadRegistry non-exclusively when only reading the thread list, exclusively when adding/removing threads - r=canaltinova
|
||
Comment 4•2 years ago
|
||
Backed out for causing cpp failures at TestBaseProfiler.cpp.
Backout link: https://hg.mozilla.org/integration/autoland/rev/ca206077a39582eabf096dc6492d7c9e07318f26
Failure log: https://treeherder.mozilla.org/logviewer?job_id=369898714&repo=autoland&lineNumber=2309
|
|
Assignee | |
Comment 5•2 years ago
|
||
Sorry about that.
The test was slightly wrong: It should have updated the state before unlocking the mutex, so that the other thread would see the expected state when acquiring the lock.
Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e1ddbd49a3db BaseProfilerSharedMutex and exclusive&shared RAII locks - r=canaltinova https://hg.mozilla.org/integration/autoland/rev/8a0cdf481812 Lock the ThreadRegistry non-exclusively when only reading the thread list, exclusively when adding/removing threads - r=canaltinova
|
||
Comment 7•2 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/e1ddbd49a3db
https://hg.mozilla.org/mozilla-central/rev/8a0cdf481812
Description
•