Name the SamplerThread on linux

RESOLVED FIXED in mozilla26

Status

()

Core
Gecko Profiler
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

Trunk
mozilla26
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g18? affected)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 801768 [details] [diff] [review]
patch

Naming the threads is useful for debugging. I also want to extend profile.sh's feature to detect if the profiler is running to reflect if it's really running and not check for the startup environment. In another bug I'll patch profile.sh to send a signal to the process.
Attachment #801768 - Flags: review?(jld)
(Assignee)

Updated

4 years ago
Attachment #801768 - Attachment is patch: true
Attachment #801768 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Updated

4 years ago
Blocks: 914654
(Assignee)

Updated

4 years ago
Blocks: 914658
Comment on attachment 801768 [details] [diff] [review]
patch

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

Looks good to me.
Attachment #801768 - Flags: review?(jld) → review+
(Assignee)

Comment 2

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/508a916a482a
Backed out for suspicion of breaking Linux debug mochitest-bc.
https://hg.mozilla.org/integration/mozilla-inbound/rev/efb5dda07b74

https://tbpl.mozilla.org/php/getParsedLog.php?id=27649124&tree=Mozilla-Inbound
Note the libpthread error immediately prior.
Assignee: nobody → ryanvm
Assignee: ryanvm → bgirard
This apparently also completely broke Android 2.2 tests.
https://tbpl.mozilla.org/php/getParsedLog.php?id=27657393&tree=Mozilla-Inbound
(Assignee)

Comment 5

4 years ago
Created attachment 802759 [details] [diff] [review]
patch v2

Copying the bits from platform_thread_posix.cc
Attachment #801768 - Attachment is obsolete: true
Attachment #802759 - Flags: review?(jld)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #3)
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27649124&tree=Mozilla-Inbound
> Note the libpthread error immediately prior.

09:48:45     INFO -  /home/cltbld/talos-slave/test/build/tests/bin/xpcshell: /lib64/libpthread.so.0: version `GLIBC_2.12' not found (required by /home/cltbld/talos-slave/test/build/application/firefox/libxul.so)

It looks like libxul was built against a newer glibc and run with an older one?  That seems less than ideal — I'd think people would expect a build-only try run to catch problems like using a too-new interface.
Comment on attachment 802759 [details] [diff] [review]
patch v2

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

::: tools/profiler/platform-linux.cc
@@ +351,5 @@
>      signal_sender_launched_ = true;
>    }
> +  // Taken from platform_thread_posix.cc
> +#if defined(OS_LINUX)
> +  prctl(PR_SET_NAME, reinterpret_cast<uintptr_t>(name), 0, 0, 0);

Won't this set the name for the current thread instead of the signal sender thread?
(Assignee)

Comment 8

4 years ago
(In reply to Jed Davis [:jld] from comment #7)
> Comment on attachment 802759 [details] [diff] [review]
> patch v2
> 
> Review of attachment 802759 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/profiler/platform-linux.cc
> @@ +351,5 @@
> >      signal_sender_launched_ = true;
> >    }
> > +  // Taken from platform_thread_posix.cc
> > +#if defined(OS_LINUX)
> > +  prctl(PR_SET_NAME, reinterpret_cast<uintptr_t>(name), 0, 0, 0);
> 
> Won't this set the name for the current thread instead of the signal sender
> thread?

Ahh opps. I wrote that patch at home and wanted to test it on my work linux desktop.
(Assignee)

Comment 9

4 years ago
Created attachment 803149 [details] [diff] [review]
patch v3

Fixed & tested
Attachment #802759 - Attachment is obsolete: true
Attachment #802759 - Flags: review?(jld)
Attachment #803149 - Flags: review+
(Assignee)

Comment 10

4 years ago
Comment on attachment 803149 [details] [diff] [review]
patch v3

Opps, this patch hasn't been r+ yet.
Attachment #803149 - Flags: review+ → review?(jld)
Comment on attachment 803149 [details] [diff] [review]
patch v3

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

::: tools/profiler/platform-linux.cc
@@ +351,5 @@
>      signal_sender_launched_ = true;
>    }
> +  // Taken from platform_thread_posix.cc
> +#if defined(OS_LINUX)
> +  prctl(PR_SET_NAME, "SamplerThread", 0, 0, 0);

This sets the current thread's name.  Shouldn't it be at the top of SignalSender instead?
Flags: needinfo?(bgirard)
(Assignee)

Comment 12

4 years ago
Yes that's wrong. for the purpose I am using it for it doesn't really matter but I'll get it fixed.
Flags: needinfo?(bgirard)
(Assignee)

Comment 13

4 years ago
Created attachment 803799 [details] [diff] [review]
patch
Attachment #803149 - Attachment is obsolete: true
Attachment #803149 - Flags: review?(jld)
Attachment #803799 - Flags: review?(jld)
Attachment #803799 - Flags: review?(jld) → review+
(Assignee)

Comment 14

4 years ago
I think this is it. Thanks Jed.
https://hg.mozilla.org/integration/b2g-inbound/rev/8863624225d5
https://hg.mozilla.org/mozilla-central/rev/8863624225d5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Created attachment 809409 [details] [diff] [review]
Name the SamplerThread on linux (b2g18)

We need to port this patch to b2g18 since this change:
https://github.com/mozilla-b2g/B2G/commit/206d3ec3d72cd2f9e5421d4f415668aab59ee0e1#L0R120

...requires the sampler thread to be named for profiler capturing to work.
Attachment #809409 - Flags: review?(bgirard)
blocking-b2g: --- → leo?
status-b2g18: --- → affected
tracking-b2g18: --- → ?
Comment on attachment 809409 [details] [diff] [review]
Name the SamplerThread on linux (b2g18)

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 914658
User impact if declined: unable to capture profiling data using profile.sh patched from the above bug
Testing completed: local build and profile.sh work as expected
Risk to taking this patch (and alternatives if risky): minimal--just gives the Linux version of the thread a name, something the Windows and MacOS threads already have
String or UUID changes made by this patch: none
Attachment #809409 - Flags: approval-mozilla-b2g18?
(Assignee)

Comment 18

4 years ago
Comment on attachment 809409 [details] [diff] [review]
Name the SamplerThread on linux (b2g18)

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

IMO this doesn't need a review but here's one anyways.
Attachment #809409 - Flags: review?(bgirard) → review+
Similar question as what's shown - https://bugzilla.mozilla.org/show_bug.cgi?id=799638#c48, why can't a local application of this patch on b2g18 be sufficient here? Why do we need this patch on b2g18 directly?
Flags: needinfo?(mhabicher)
As in other bugs, de-noming; will make one big uber-patch that devs can apply as needed.
blocking-b2g: leo? → ---
Flags: needinfo?(mhabicher)

Updated

4 years ago
Attachment #809409 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
You need to log in before you can comment on or make changes to this bug.