Bug 1755823 Comment 0 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

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](https://share.firefox.dev/3sFjL36), 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:

1. The sampler thread could unlock the `ThreadRegistry` betweeen 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.

2. Change the `ThreadRegistry` locking mechanism to a readers-writer lock.
This would allow simultaneous reads (to access individual `ThreadRegistration` objects), in our case from the sampler and from markers, while preventing writes (to add/remove `ThreadRegistration`s to/from the list) when a thread starts or stops.

3. Add locks around `ThreadRegistration` objects, that would at least prevent their destruction -- Maybe through a thread-safe `RefPtr`? This would allow the sampler to lock the `ThreadRegistry` for only a short time, just to read the list and lock/reference individual `ThreadRegistration`s to be sampled.

Option #2 feels like the easiest, and could be done using [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex) (or equivalent if available in Gecko).
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](https://share.firefox.dev/3sFjL36), 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:

1. The sampler thread could unlock the `ThreadRegistry` betweeen 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.

2. Change the `ThreadRegistry` locking mechanism to a readers-writer lock.
This would allow simultaneous reads (to access individual `ThreadRegistration` objects), in our case from the sampler and from markers, while preventing writes (to add/remove `ThreadRegistration`s to/from the list) when a thread starts or stops.

3. Add locks around `ThreadRegistration` objects, that would at least prevent their destruction -- Maybe through a thread-safe `RefPtr`? This would allow the sampler to lock the `ThreadRegistry` for only a short time, just to read the list and lock/reference individual `ThreadRegistration`s to be sampled.

Option #2 feels like the easiest, and could be done using [mozilla::RWLock](https://searchfox.org/mozilla-central/rev/9bed2623831804ac086bbb80cb666e5c3b00416a/xpcom/threads/RWLock.h#22-44) or [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex).

Back to Bug 1755823 Comment 0