Task Tracer: reset thread ids after content processes are forked from Nuwa

RESOLVED FIXED in mozilla36

Status

()

Core
Gecko Profiler
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cyu, Assigned: cyu)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
On Linux (also on b2g), ThreadInfo::ThreadId() uses gettid(). To make SignalSender sends the signal to the correct thread, we wrap tgkill() in Nuwa.cpp and perform the tid lookup and translation (bug 961959). This causes problems in TaskTracer because it needs the real thread ids. We need to take another approach to use correct tids in sending signals.
(Assignee)

Comment 1

3 years ago
Created attachment 8514824 [details]
WIP
No longer blocks: 1091479
(Assignee)

Comment 2

3 years ago
Created attachment 8515922 [details]
WIP V2
Attachment #8514824 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8519759 [details] [diff] [review]
Part 1: Don't wrap tgkill() for SPS.
(Assignee)

Updated

3 years ago
Attachment #8519759 - Attachment description: Part 1: Don → Part 1: Don't wrap tgkill() for SPS.
(Assignee)

Comment 4

3 years ago
Created attachment 8519761 [details] [diff] [review]
Part 2: Implement NuwaAddThreadConstructor() for threads to register custom callback to be invoked in recreation.
(Assignee)

Comment 5

3 years ago
Created attachment 8519763 [details] [diff] [review]
Part 3: Reset thread ID in SPS when a process is forked from Nuwa
Attachment #8515922 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8519759 - Flags: review?(bgirard)
(Assignee)

Updated

3 years ago
Attachment #8519761 - Flags: review?(tlee)
Attachment #8519761 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Attachment #8519763 - Flags: review?(bgirard)
(Assignee)

Comment 6

3 years ago
The patches take another approach to fix bug 961959. The problem arises from that SPS doesn't function correctly with the Nuwa process, which has several threads already registered to SPS. SPS in the forked processes doesn't know that tids already changed and thus can't send SIGPROF to the recreated threads.

In bug 961959 we took a simple route by wrapping tgkill(). In the wrapper if the signal is sent to a recreated thread, we perform tid translation for the caller so the caller may use the tid in the old process to send the corresponding thread in the forked process. This works in SPS but not in TaskTracer. So we take the approach to perform tid fixup when the process is forked.

The idea is to add a method to the Nuwa API, NuwaAddThreadConstructor(). It can be called in the Nuwa process to register a callback that will be invoked during thread recreation in the forked process. SPS uses this method to correct thread ids after a process is forked.

Comment 7

3 years ago
I just have a rough view on the part 2.  I concern that we use stl in Nuwa.cpp that may struggle people porting Nuwa to different platform; ex. BSD.  We had meet lib dependency issue with LD_PRELOAD, now we rely on the behavior of current platform, but we never sure how the next platform behaves.  struct as simple as vector and linked list, we should consider to have a simple implementation in Nuwa.cpp instead of using stl.

Updated

3 years ago
Attachment #8519759 - Flags: review?(bgirard) → review+

Updated

3 years ago
Attachment #8519763 - Flags: review?(bgirard) → review+
(Assignee)

Comment 8

3 years ago
(In reply to Thinker Li [:sinker] from comment #7)
> I just have a rough view on the part 2.  I concern that we use stl in
> Nuwa.cpp that may struggle people porting Nuwa to different platform; ex.
> BSD.  We had meet lib dependency issue with LD_PRELOAD, now we rely on the
> behavior of current platform, but we never sure how the next platform
> behaves.  struct as simple as vector and linked list, we should consider to
> have a simple implementation in Nuwa.cpp instead of using stl.

If we really have a problem with stl, we have another choice: mozilla::Vector in MFBT.

Comment 9

3 years ago
Comment on attachment 8519761 [details] [diff] [review]
Part 2: Implement NuwaAddThreadConstructor() for threads to register custom callback to be invoked in recreation.

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

::: mozglue/build/Nuwa.cpp
@@ +148,5 @@
>    // The thread specific function to recreate the new thread. It's executed
>    // after the thread is recreated.
> +
> +  std::vector<nuwa_construct_t> *recrFunctions;
> +  void addThreadConstructor(nuwa_construct_t *construct) {

void addThreadConstructor(const nuwa_construct_t &construct) {

With const and reference, it gives developers a hint that the ownership of the object is not transferred.

::: mozglue/build/Nuwa.h
@@ +154,4 @@
>  /**
>   * Register a method to be invoked after a new process is spawned and threads
> + * are recreated. The method will be invoked on the main thread *after*
> + * the other threads are recreated and fully functional.

all threads??
Attachment #8519761 - Flags: review?(tlee) → review+
Comment on attachment 8519761 [details] [diff] [review]
Part 2: Implement NuwaAddThreadConstructor() for threads to register custom callback to be invoked in recreation.

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

::: mozglue/build/Nuwa.h
@@ +142,5 @@
>  /**
>   * Register a method to be invoked after a new process is spawned. The method
> + * will be invoked on the main thread *before* recreating the other threads.
> + * The registered method cannot perform any action (e.g. acquiring a mutex)
> + * that might depend on another thread that is nonexistent.

must not perform any action .. thread that has not yet been recreated.

@@ +164,5 @@
> +
> +/**
> + * Register a method to be invoked after the current thread is recreated in the
> + * spawned process. Note that this function is called while other threads are
> + * frozen. It cannot perform any action (e.g. acquiring a mutex)

It must not perform
Attachment #8519761 - Flags: review?(khuey) → review+
(Assignee)

Comment 11

3 years ago
Created attachment 8523814 [details] [diff] [review]
Part 2: Implement NuwaAddThreadConstructor() for threads to register custom callback to be invoked in recreation.

Updated per comment #9 and comment #10.
Attachment #8519761 - Attachment is obsolete: true
Attachment #8523814 - Flags: review+
(Assignee)

Comment 12

3 years ago
(In reply to Thinker Li [:sinker] from comment #9)
> @@ +154,4 @@
> >  /**
> >   * Register a method to be invoked after a new process is spawned and threads
> > + * are recreated. The method will be invoked on the main thread *after*
> > + * the other threads are recreated and fully functional.
> 
> all threads??

All threads include the main thread, which is not recreated by us. So the wording here is the other threads.
(Assignee)

Comment 13

3 years ago
Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b9cec78d9f30
(Assignee)

Comment 14

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0cf903ee97
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccac54007b1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f127e0f6293
https://hg.mozilla.org/mozilla-central/rev/bd0cf903ee97
https://hg.mozilla.org/mozilla-central/rev/ccac54007b1e
https://hg.mozilla.org/mozilla-central/rev/1f127e0f6293
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.