918833 - Improved profiler thread register

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

Currently you have to uncomment the thread register on b2g. I quickly disabled it at the time because registering a thread allocates a pseudostack which requires memory and we weren't using this feature at all. Now I want to find a solution to use this feature without a custom build.

Let's enable it for profiling builds or enable it for builds that run with MOZ_PROFILER_STARTUP.

Also this fixes the bug where registering the same thread would mess things up.

Comment 1

[Benoit Girard (:BenWa)]

Comment on attachment 807767 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=fccd929a4b47

Comment 2

[(no longer active)]

Comment on attachment 807767 [details] [diff] [review]
patch

Review of attachment 807767 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/platform-linux.cc
@@ +401,5 @@
> +    if (info->ThreadId() == id) {
> +      // Thread already registered. This means the first unregister will be
> +      // too early.
> +      ASSERT(false);
> +      return;

return false

::: tools/profiler/platform-macos.cc
@@ +364,5 @@
> +    if (info->ThreadId() == id) {
> +      // Thread already registered. This means the first unregister will be
> +      // too early.
> +      ASSERT(false);
> +      return;

return false

::: tools/profiler/platform-win32.cc
@@ +284,5 @@
> +    if (info->ThreadId() == id) {
> +      // Thread already registered. This means the first unregister will be
> +      // too early.
> +      ASSERT(false);
> +      return;

return false

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=b96fce78ec36

Comment 4

[Benoit Girard (:BenWa)]

Attachment: rebased

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd28a8561332

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd28a8561332

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug918833-fix-thread-registration-uplift26.diff

I've crossported the patch to Gecko 26, which is also affected.  (It doesn't apply cleanly as-is.)  We'll probably want the profiler working as effortlessly as possible as B2G 1.2 nears release.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Developers will have to apply this patch (or equivalent) manually if they want to profile non-main threads on aurora.
Testing completed (on m-c, etc.): Successfully profiled the B2G Compositor thread, with this patch on aurora.
Risk to taking this patch (and alternatives if risky): None if profiling isn't enabled, as I understand it.
String or IDL/UUID changes made by this patch: None.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d32335e66e7