Closed
Bug 1091533
Opened 10 years ago
Closed 10 years ago
Task Tracer: reset thread ids after content processes are forked from Nuwa
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: cyu, Assigned: cyu)
References
Details
Attachments
(3 files, 3 obsolete files)
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cyu
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8514824 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8519759 -
Attachment description: Part 1: Don → Part 1: Don't wrap tgkill() for SPS.
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8515922 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8519759 -
Flags: review?(bgirard)
Assignee | ||
Updated•10 years ago
|
Attachment #8519761 -
Flags: review?(tlee)
Attachment #8519761 -
Flags: review?(khuey)
Assignee | ||
Updated•10 years ago
|
Attachment #8519763 -
Flags: review?(bgirard)
Assignee | ||
Comment 6•10 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•10 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•10 years ago
|
Attachment #8519759 -
Flags: review?(bgirard) → review+
Updated•10 years ago
|
Attachment #8519763 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 8•10 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•10 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•10 years ago
|
||
Updated per comment #9 and comment #10.
Attachment #8519761 -
Attachment is obsolete: true
Attachment #8523814 -
Flags: review+
Assignee | ||
Comment 12•10 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•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•