1659901 - Rework TLS usage in profiler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, task, P2)

Description

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

There are two TLS static objects used in the Gecko Profiler: TLSRegisteredThread::sRegisteredThread and -AutoProfilerLabel::sProfilingStackOwnerTLS.

While investigating bug 1657174, I noticed that there are a few potential issues/inefficiencies with their usage:

• init() is called multiple times (once per thread registration), it's not incorrect but we could avoid it.
• Some get()/set() accesses don't check that init() was called.
• At least one get() that could return null is not null-checked.

To clarify the situation, I want to better encapsulate these TLSs, to ensure that init() is always called the first time and the result is cached, and that all accesses correctly deal with a failed init(). And of course add the missing null check(s).

This will most probably not solve bug 1657174, but it should at least remove this part of the equation from the problem there, and in any case it will help with maintenance.

Comment 1

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

Attachment: Bug 1659901 - Rework profiler's TLS accesses - r?mstange

To ensure correct usage of TLSs in the profiler, they are now better encapsulated so that:

• init() is called once and its result is cached. (TLSREGISTEREDThread::Init() doesn't need proof of the PSLock, because it's using thread-safe function-static initializers.)
• get() and set() always init() as needed, or in some particular cases strongly assert that init() was successful.

Also, a null-check was missing in profiler_init_threadmanager().

Depends on D87588

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cb615e26bdc
Rework profiler's TLS accesses - r=mstange

Comment 3

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8cb615e26bdc

Comment 4

[Julien Cristau [:jcristau]]

Tracking for 80 in case we want to respin GV for a fenix update.

Comment 5

[Julien Cristau [:jcristau]]

Would you mind requesting uplift to mozilla-release so we can get the crash fix in a fenix update?

Comment 6

[Julien Cristau [:jcristau]]

(here and in bug 1657174)

Comment 7

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

(In reply to Julien Cristau [:jcristau] from comment #6)

(here and in bug 1657174)

I believe bug 1657174 is not required, as it's only changing some assertions (i.e., no real code change), and it's touching different parts.

Comment 8

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

Comment on attachment 9170932 [details]
Bug 1659901 - Rework profiler's TLS accesses - r?mstange

Beta/Release Uplift Approval Request

• User impact if declined: Crashes
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I believe it's less risky than the previous code, because operations are now encapsulated in a class so they cannot be done incorrectly (e.g., previously it was possible to access TLS variables without proper initialization).

Note: This patch introduced an installer size regression on Mac, see bug 1660458. I think preventing crashes is better than the size issue; but I will work on reducing the size this week, if you can and would prefer to wait for that...

• String changes made/needed: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9170932 [details]
Bug 1659901 - Rework profiler's TLS accesses - r?mstange

This is already on Beta.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

(In reply to Gerald Squelart [:gerald] (he/him) from comment #7)

(In reply to Julien Cristau [:jcristau] from comment #6)

(here and in bug 1657174)

I believe bug 1657174 is not required, as it's only changing some assertions (i.e., no real code change), and it's touching different parts.

There's conflicts with this patch if bug 1657174 isn't included.

Comment 11

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

Comment on attachment 9170932 [details]
Bug 1659901 - Rework profiler's TLS accesses - r?mstange

I'll redo the uplift request.

Comment 12

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

Comment on attachment 9170932 [details]
Bug 1659901 - Rework profiler's TLS accesses - r?mstange

Beta/Release Uplift Approval Request

• User impact if declined: Crashes
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1657174
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I believe it's less risky than the previous code, because operations are now encapsulated in a class so they cannot be done incorrectly (e.g., previously it was possible to access TLS variables without proper initialization).

Note: This patch introduced an installer size regression on Mac, see bug 1660458. I think preventing crashes is better than the size issue; but I will work on reducing the size this/next week, if you can and would prefer to wait for that...

• String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9170932 [details]
Bug 1659901 - Rework profiler's TLS accesses - r?mstange

Approved for GV80 for Fenix 80.1.3.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/2fb97a7f50e8