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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: WIP

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: WIP V2

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Part 1: Don't wrap tgkill() for SPS.

Comment 4

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Part 2: Implement NuwaAddThreadConstructor() for threads to register custom callback to be invoked in recreation.

Comment 5

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Part 3: Reset thread ID in SPS when a process is forked from Nuwa

Comment 6

[Cervantes Yu [:cyu] [:cervantes]]

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

[Thinker Li [:sinker]]

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.

Comment 8

[Cervantes Yu [:cyu] [:cervantes]]

(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

[Thinker Li [:sinker]]

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??

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 11

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Part 2: Implement NuwaAddThreadConstructor() for threads to register custom callback to be invoked in recreation.

Updated per comment #9 and comment #10.

Comment 12

[Cervantes Yu [:cyu] [:cervantes]]

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

Comment 13

[Cervantes Yu [:cyu] [:cervantes]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b9cec78d9f30

Comment 14

[Cervantes Yu [:cyu] [:cervantes]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd0cf903ee97
https://hg.mozilla.org/mozilla-central/rev/ccac54007b1e
https://hg.mozilla.org/mozilla-central/rev/1f127e0f6293