Closed Bug 1722261 Opened 4 months ago Closed 3 months ago

Replace RegisteredThread with ThreadRegistration

Categories

(Core :: Gecko Profiler, task, P2)

task

Tracking

()

RESOLVED FIXED
93 Branch
Tracking Status
firefox93 --- fixed

People

(Reporter: gerald, Assigned: gerald)

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

Blocks: 1694492

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

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

ThreadRegistration already knows if it's registered, so profiler_is_active_and_thread_is_registered can use that now.

Depends on D121844

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

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

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

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

Also moved profiler_thread_is_sleeping to ProfilerThreadState.h.

Depends on D121960

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

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 ProfiledThreadDatas get destroyed/recreated when stopping/starting the profiler), there is no need for an explicit ClearRunningTimes() anymore.

Depends on D121963

Remove some now-unused functions.

Depends on D121967

And now in locked_unregister_thread, use the OnThreadPtr directly instead of the TLSRegisteredThread.

Depends on D121974

Attachment #9235053 - Attachment is obsolete: true

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

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7e53f12dc99
New header ProfilerThreadState.h will contain thread-related profiler APIs - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/955f1c1c9f1a
Insert new ThreadRegistration (un)registering functions in the middle of profiler_{,un}register_thread - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/3b06f1822f48
Remove handling of nested RegisteredThreads - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/de6aa81e9ccf
Move profiler_is_active_and_thread_is_registered to ProfilerThreadState.h and use ThreadRegistration - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/21f448ea0744
Use ProfilingStack inside ThreadRegistrationData - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/0ad4e47ff813
Remove data from RegisteredThread, only keep pass-through methods to the ThreadRegistrationData - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/aa80d9331b8c
Move profiler_thread_is_being_profiled to ProfilerThreadState.h and use ThreadRegistration - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/842b895b7ac1
Store {,Racy}RegisteredThread pointer in ThreadRegistrationData - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/2f6b4f80db1f
Remove ThreadInfo.h, use ProfilerRegistrationInfo instead - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/eb61f791b99b
ProfilerThreadPlatformData platform-specific implementation - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/5e0cf65719d3
Use ThreadRegistration classes during sampling - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/5798abd2f42a
Use IsSleeping and SetSleeping/Awake in ThreadRegistration - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/015fe6840a85
GetRunningEventDelay through ThreadRegistration - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/d1cf4d7f6796
Store ProfiledThreadData pointer in ThreadRegistrationData - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/0c05174fc2c1
Move mPreviousThreadRunningTimes from old PlatformData to ProfiledThreadData - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/cca5ffa19387
Remove the old PlatformData, which is now unused - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/7574972ea660
During sampling, go through the ThreadRegistry to find ThreadRegistrations with ProfiledThreadData - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/cf05e4baf78f
In locked_profiler_stop, find live threads from ThreadRegistry and stop using RegisteredThread - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/e5cc8b60c7f4
Rework profiler_set/clear_js_context to use ThreadRegistration - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/d88b53a4c4ac
Remove final uses of {,Racy}RegisteredThread's ReinitializeOnResume, and all ...JSSampling's - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/641b915a5877
In locked_profiler_start, find registered threads from ThreadRegistry and stop using RegisteredThread - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/01f52fd9d41b
Convert remaining uses of RegisteredThread's GetEventTarget and ResetMainThread - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/54f922f8d85c
Convert remaining use of RegisteredThread's ProfilingStack that was in locked_register_thread - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/0f79b507f7fa
Convert locked_profiler_stream_json_for_this_process and ActivePS::ProfiledThreads to stop using RegisteredThread - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/3ca4e85a2f0c
Stop using RegisteredThread::Info() in profiler_unregister_thread - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/c382b83927b1
SizeOf{Ex,In}cludingThis for ProfilerRegist* classes, used in CorePS - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/e0acdc278398
Remove RegisteredThread from LiveProfiledThreadData - r=canaltinova
https://hg.mozilla.org/integration/autoland/rev/f6008261478f
RegisteredThread is now effectively unused, remove all remaining traces - r=canaltinova

I see at least 2 issues:

  • ThreadRegistration::SizeOfIncludingThis calls aMallocSizeOf(this), but this may actually be on the stack. I'll switch to sizeof there.
  • TaskController::RunPoolThread calls PROFILER_REGISTER_THREAD but not PROFILER_UNREGISTER_THREAD, so the ThreadRegistration is never deleted and is caught by valgrind.
Flags: needinfo?(gsquelart)

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.

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

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.

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

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

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch
Regressions: 1727754
Regressions: 1727762
Blocks: 1728056
Blocks: 1728058
Blocks: 1709502
Blocks: 1598133
Regressions: 1729434
You need to log in before you can comment on or make changes to this bug.