Closed Bug 1659901 Opened 1 month ago Closed 1 month ago

Rework TLS usage in profiler

Categories

(Core :: Gecko Profiler, task, P2)

task

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox80 + fixed
firefox81 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

Details

Attachments

(1 file)

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.

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

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8cb615e26bdc
Rework profiler's TLS accesses - r=mstange
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Regressions: 1660458

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

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

Flags: needinfo?(gsquelart)

(here and in bug 1657174)

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

Flags: needinfo?(gsquelart)

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
Attachment #9170932 - Flags: approval-mozilla-release?
Attachment #9170932 - Flags: approval-mozilla-beta?

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

This is already on Beta.

Attachment #9170932 - Flags: approval-mozilla-beta?

(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 on attachment 9170932 [details]
Bug 1659901 - Rework profiler's TLS accesses - r?mstange

I'll redo the uplift request.

Attachment #9170932 - Flags: approval-mozilla-release?

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:
Attachment #9170932 - Flags: approval-mozilla-release?

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

Approved for GV80 for Fenix 80.1.3.

Attachment #9170932 - Flags: approval-mozilla-release? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.