Replace RegisteredThread with ThreadRegistration
Categories
(Core :: Gecko Profiler, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox93 | --- | fixed |
People
(Reporter: mozbugz, Assigned: mozbugz)
References
Details
Attachments
(30 files, 1 obsolete file)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
Bug 1721939 introduces new classes to register threads and provide safe access to thread-relevant data, but these are not used yet (outside of unit tests).
This bug here will replace the old RegisteredThread
with the new ThreadRegistration
(and friends).
The goal is to have trusted access to thread data, such that it will be possible to capture stack samples without locking the main profiler mutex (bug 1716959).
Assignee | ||
Comment 1•3 years ago
|
||
Thread-related APIs that are currently in ProfilerState.h will move here, and will use the new ThreadRegistration classes introduced in bug 1721939.
In this patch the header is empty apart from a few #includes, and users of "ProfilerState.h"'s thread functions now #include "ProfilerThreadState.h" instead. So there are no actual code changes yet.
Depends on D120818
Assignee | ||
Comment 2•3 years ago
|
||
ProfilerThreadRegistry (un)register functions have been moved to platform.cpp because they need to interact closely with functions and classes in that file.
profiler_{,un}register_thread are now simple calls to ThreadRegistration::{R,Unr}egisterThread, which call ThreadRegistry::{R,Unr}egister, which now integrate that old bodies of profiler_{,un}register_thread.
So from this point, threads may use either profiler_{,un}register_thread or ThreadRegistration equivalents, and this will (un)register with both the new ThreadRegistration classes and the legacy RegisteredThread class into CorePS. (Later patches will remove RegisteredThread completely.)
Bonus: Since the stack top is now adjusted when constructing ThreadRegistrationInfo (before giving it to the legacy ThreadInfo), there is no more need for GetStackTop()
.
Depends on D121843
Assignee | ||
Comment 3•3 years ago
|
||
ThreadRegistration already knows if it's registered, so profiler_is_active_and_thread_is_registered can use that now.
Depends on D121844
Assignee | ||
Comment 4•3 years ago
|
||
The ProfilingStack object inside ThreadRegistrationData is guaranteed to live while the thread is registered (and alive), and all accesses are guaranteed to be done either:
- On-thread, so during that time ThreadRegistrationData and its ProfilingStack cannot be destroyed.
- From another thread, but with the Profiler lock and/or per-thread lock, which also guarantees that ThreadRegistrationData and its ProfilingStack cannot be destroyed.
RacyRegisteredThread brought some doubts about end-of-thread accesses, that why there's was an intermediate ref-counted ProfilingStackOwner to keep ProfilingStack alive where needed. Now we can remove it.
Also the separate TLS (Thread Local Storage) for ProfilingStackOwner can be removed, since we can reach the ProfilingStack through the ThreadRegistration TLS for the same cost.
Depends on D121845
Assignee | ||
Comment 5•3 years ago
|
||
Now {,Racy}RegisteredThread don't contain any data, but provide the same API that goes through ThreadRegistration.
Until they are removed, {,Racy}RegisteredThread are given private access to ThreadRegistration{,Data} for easier access.
This means that we can now change uses of RegisteredThread to use ThreadRegistration directly instead, and the data will be the same through either APIs.
Also, RacyRegisteredThread::ThreadId() was unused so it can be removed.
Depends on D121846
Assignee | ||
Comment 6•3 years ago
|
||
Depends on D121847
Assignee | ||
Comment 7•3 years ago
|
||
This will help with replacing the old {,Racy}RegisteredThread with the new ThreadRegistration classes, while still being able to access the old classes (until they are not needed anymore).
Depends on D121848
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D121849
Assignee | ||
Comment 9•3 years ago
|
||
This is mostly a copy of the old PlatformData. That old PlatformData will be removed in a later patch, once it's not used anymore.
Depends on D121850
Assignee | ||
Comment 10•3 years ago
|
||
Depends on D121959
Assignee | ||
Comment 11•3 years ago
|
||
Also moved profiler_thread_is_sleeping to ProfilerThreadState.h.
Depends on D121960
Assignee | ||
Comment 12•3 years ago
|
||
Depends on D121961
Assignee | ||
Comment 13•3 years ago
|
||
Because ProfiledThreadData is always related to IsBeingProfiled, they are set and cleared together.
In particular, this is used to quickly guess that the thread is being profiled by reading the non-atomic mProfiledThreadData, rather than reading the slightly slower atomic mIsBeingProfiled.
Depends on D121962
Assignee | ||
Comment 14•3 years ago
|
||
It's more appropriate because the running times are only relevant to a profiling session.
And PreviousThreadRunningTimes can now be accessed through the ThreadRegistration (thanks to the ProfiledThreadData pointer added in the previous patch).
Since the threads' RunningTimes don't live in PlatformData
, and because they are now implicitly cleared between profiling sessions (because ProfiledThreadData
s get destroyed/recreated when stopping/starting the profiler), there is no need for an explicit ClearRunningTimes()
anymore.
Depends on D121963
Assignee | ||
Comment 15•3 years ago
|
||
Depends on D121964
Assignee | ||
Comment 16•3 years ago
|
||
Depends on D121965
Assignee | ||
Comment 17•3 years ago
|
||
Depends on D121966
Assignee | ||
Comment 18•3 years ago
|
||
Remove some now-unused functions.
Depends on D121967
Assignee | ||
Comment 19•3 years ago
|
||
Depends on D121968
Assignee | ||
Comment 20•3 years ago
|
||
Depends on D121969
Assignee | ||
Comment 21•3 years ago
|
||
Depends on D121970
Assignee | ||
Comment 22•3 years ago
|
||
Depends on D121971
Assignee | ||
Comment 23•3 years ago
|
||
Depends on D121972
Assignee | ||
Comment 24•3 years ago
|
||
Remove now-unused RegisteredThread::GetJSContext().
Depends on D121973
Assignee | ||
Comment 25•3 years ago
|
||
And now in locked_unregister_thread, use the OnThreadPtr directly instead of the TLSRegisteredThread.
Depends on D121974
Assignee | ||
Comment 26•3 years ago
|
||
Depends on D121975
Assignee | ||
Comment 27•3 years ago
|
||
Depends on D121976
Assignee | ||
Comment 28•3 years ago
|
||
Depends on D121977
Updated•3 years ago
|
Assignee | ||
Comment 29•3 years ago
|
||
Since ThreadRegistration already handles nested registrations, the legacy profiler_{,un}register_thread() functions don't need to care about that anymore (and the RegisteredThread TLS will be removed in a later patch anyway).
The informational markers have been moved to ProfilerThreadRegistration.cpp.
Depends on D121844
Comment 30•3 years ago
|
||
Comment 31•3 years ago
|
||
Backed out 28 changesets (bug 1722261) for causing linux asan failures.
https://hg.mozilla.org/integration/autoland/rev/7a2320e8b395cfd610e8619e6461ed310b3377e7
Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=f6008261478f4c13316a54f863063b5aec81dbb6&selectedTaskRun=bTeyXJzWR5WmfwDi2hetBg.0
Failure log:
https://treeherder.mozilla.org/logviewer?job_id=348884196&repo=autoland&lineNumber=6532
https://treeherder.mozilla.org/logviewer?job_id=348884833&repo=autoland&lineNumber=3025
Assignee | ||
Comment 32•3 years ago
|
||
I see at least 2 issues:
ThreadRegistration::SizeOfIncludingThis
callsaMallocSizeOf(this)
, butthis
may actually be on the stack. I'll switch tosizeof
there.TaskController::RunPoolThread
callsPROFILER_REGISTER_THREAD
but notPROFILER_UNREGISTER_THREAD
, so theThreadRegistration
is never deleted and is caught by valgrind.
Comment 33•3 years ago
|
||
Comment 34•3 years ago
|
||
FWIW, this also caused build failures on some tier-3 platforms, with:
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-s390x-linux-gnu -std=gnu++17 --target=s390x-linux-gnu -o Unified_cpp_accessible_aom0.o -c -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/accessible/aom -I/builds/worker/workspace/obj-build/accessible/aom -I/builds/worker/checkouts/gecko/accessible/base -I/builds/worker/checkouts/gecko/accessible/generic -I/builds/worker/checkouts/gecko/accessible/atk -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wshadow-uncaptured-local -Wsign-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wcomma -Wenum-compare-conditional -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=free-nonheap-object -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-error=deprecated-copy -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-psabi -Wno-unknown-warning-option -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -O2 -fno-omit-frame-pointer -funwind-tables -fexperimental-new-pass-manager -MD -MP -MF .deps/Unified_cpp_accessible_aom0.o.pp Unified_cpp_accessible_aom0.cpp
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - In file included from Unified_cpp_accessible_aom0.cpp:2:
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - In file included from /builds/worker/checkouts/gecko/accessible/aom/AccessibleNode.cpp:19:
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/Document.h:35:
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:15:
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/Monitor.h:10:
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/CondVar.h:16:
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - In file included from /builds/worker/workspace/obj-build/dist/include/GeckoProfiler.h:26:
[task 2021-08-20T05:57:00.408Z] 05:57:00 ERROR - /builds/worker/workspace/obj-build/dist/include/mozilla/ProfilerLabels.h:184:12: error: unknown type name 'ProfilingStackOwner'; did you mean 'ProfilingStackOwnerTLS'?
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - static ProfilingStackOwner* Get() { return nullptr; }
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - ^~~~~~~~~~~~~~~~~~~
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - ProfilingStackOwnerTLS
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - /builds/worker/workspace/obj-build/dist/include/mozilla/ProfilerLabels.h:182:9: note: 'ProfilingStackOwnerTLS' declared here
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - class ProfilingStackOwnerTLS {
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - ^
[task 2021-08-20T05:57:00.408Z] 05:57:00 INFO - 1 error generated.
Assignee | ||
Comment 35•3 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #34)
FWIW, this also caused build failures on some tier-3 platforms
Thanks for the information, I can reproduce it by building without MOZ_GECKO_PROFILER, so I'll fix it before re-landing...
Assignee | ||
Comment 36•3 years ago
|
||
With the new ThreadRegistration classes, it becomes more important to properly balance registrations with unregistrations. Though mismatches are safely handled when they happen, in particular missing unregistrations can lead to leaks that make tests fail.
This patch changes PROFILER_REGISTER_THREAD
in TaskController.cpp to AUTO_PROFILER_REGISTER_THREAD
, to ensure that TaskController threads get unregistered when they're finished.
In AsyncLogger.h, a pair of PROFILER_REGISTER_THREAD
and PROFILER_UNREGISTER_THREAD
is converted to a single AUTO_PROFILER_REGISTER_THREAD
. This is equivalent, but the latter is safer, in case the code in-between later changes to include early breaks or returns.
Assignee | ||
Comment 37•3 years ago
|
||
When a ThreadRegistration
object is created through ThreadRegistration::RegisterThread
(instead of directly on the stack), this is recorded in mIsOnHeap
.
This helps better handle incorrectly-nested registration/unregistration pairs, or excess unregistrations.
Also, markers are now generated to highlight extra (un)registrations, with added backtrace for those surprising cases of excess unregistrations that should probably be reworked.
And ThreadRegistration::GetTLS()
was simplified, so it auto-initializes when first used, instead of relying on the main thread registration.
Depends on D123228
Comment 38•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/603838353c40
https://hg.mozilla.org/mozilla-central/rev/8e9876db0156
https://hg.mozilla.org/mozilla-central/rev/8aaef1795a1a
https://hg.mozilla.org/mozilla-central/rev/891941a9413b
https://hg.mozilla.org/mozilla-central/rev/5b9cb9b3aa12
https://hg.mozilla.org/mozilla-central/rev/d134728de015
https://hg.mozilla.org/mozilla-central/rev/1419406809bc
https://hg.mozilla.org/mozilla-central/rev/ab3cc3e0b246
https://hg.mozilla.org/mozilla-central/rev/b94590f8b265
https://hg.mozilla.org/mozilla-central/rev/3ce44d2fba54
https://hg.mozilla.org/mozilla-central/rev/b4799b1d33ee
https://hg.mozilla.org/mozilla-central/rev/ebd00f9619ae
https://hg.mozilla.org/mozilla-central/rev/db8085cb6a51
https://hg.mozilla.org/mozilla-central/rev/83191539a32c
https://hg.mozilla.org/mozilla-central/rev/9c9aa6d94238
https://hg.mozilla.org/mozilla-central/rev/dab662da1289
https://hg.mozilla.org/mozilla-central/rev/56aa354f0b0f
https://hg.mozilla.org/mozilla-central/rev/85988c4e2b68
https://hg.mozilla.org/mozilla-central/rev/7c8e01d5888f
https://hg.mozilla.org/mozilla-central/rev/1738e35e369a
https://hg.mozilla.org/mozilla-central/rev/c0a4c8ba6bda
https://hg.mozilla.org/mozilla-central/rev/b39eec63ba8c
https://hg.mozilla.org/mozilla-central/rev/a35acc69175c
https://hg.mozilla.org/mozilla-central/rev/0d03e73b0809
https://hg.mozilla.org/mozilla-central/rev/afc60b1285f5
https://hg.mozilla.org/mozilla-central/rev/b9c5b8047c2b
https://hg.mozilla.org/mozilla-central/rev/5b5fc1298bc0
https://hg.mozilla.org/mozilla-central/rev/0f64ac333373
https://hg.mozilla.org/mozilla-central/rev/48dde63f0b08
https://hg.mozilla.org/mozilla-central/rev/12796ed8484e
Description
•