1054110 - Threads should not disappear from the profile immediately after they are destroyed

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

On Talos profiles we don't save the compositor thread data in profiles. What happens is we unregister the compositor thread on shutdown which releases the profiler resources associated with the thread. This means that once a thread is unregistered we lose all the recent samples against that thread.

Ideally we should keep the circular buffer for at least a full generation before we delete the data.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Defer TheadProfile delete when unregistered under profiling

Some lifetime changes to address this problems. We're going to hold more memory while profiling and some threads go down.

Comment 2

[Benoit Girard (:BenWa)]

Before: (Since we don't run with Talos Profiling)
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a99fb792d513

After:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0906098bfe05

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Defer TheadProfile delete when unregistered under profiling v2

Forgot to clean pending ThreadInfo on profiling stop. With v1 they stuck around until shutdown.

Comment 4

[Benoit Girard (:BenWa)]

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

Comment 5

[(no longer active)]

Comment on attachment 8500607 [details] [diff] [review]
Defer TheadProfile delete when unregistered under profiling v2

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

This patch does solve the problem at hand, but FTR I hate it.  :-)  I think we could probably make ThreadInfo refcountable, and make sRegisteredThreads hold a strong ref to those objects.  Then we could just drop them off of sRegisteredThreads and drop the strong ref, and delete the stack when the object is about to die.  Basically, if profiler_is_active(), we would put the ThreadInfo's in a different container which we'd clear out when the profiler is shut down.  That way we wouldn't have two ways of dealing with deleting the stack.  Please file a follow-up if you think that makes sense, but r=me regardless.

::: tools/profiler/TableTicker.cpp
@@ +284,5 @@
>            if (!sRegisteredThreads->at(i)->Profile())
>              continue;
>  
> +          // Note that we intentionally include ThreadProfile which
> +          // have been mark for pending delete.

Nit: marked.

::: tools/profiler/TableTicker.h
@@ +123,5 @@
> +        // We've stopped profiling. We no longer need to retain
> +        // information for an old thread.
> +        if (info->IsPendingDelete()) {
> +          delete info;
> +          sRegisteredThreads->erase(sRegisteredThreads->begin() + i);

Ditto.

::: tools/profiler/platform-linux.cc
@@ +505,5 @@
> +      } else {
> +        delete info;
> +        sRegisteredThreads->erase(sRegisteredThreads->begin() + i);
> +        break;
> +      }

So many copies of this code :(

::: tools/profiler/platform-win32.cc
@@ +360,5 @@
> +        info->SetPendingDelete();
> +        break;
> +      } else {
> +        delete info;
> +        sRegisteredThreads->erase(sRegisteredThreads->begin() + i);

You're not making anything worse here, but you're not allowed to mutate a vector as you're iterating over it in the general case.  Please file a follow-up and fix this.

::: tools/profiler/platform.h
@@ +409,5 @@
>    PlatformData* GetPlatformData() const { return mPlatformData; }
>    void* StackTop() const { return mStackTop; }
>  
> +  void SetPendingDelete();
> +  bool IsPendingDelete() { return mPendingDelete; }

Nit: const.

Comment 6

[Benoit Girard (:BenWa)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> Comment on attachment 8500607 [details] [diff] [review]
> Defer TheadProfile delete when unregistered under profiling v2
> 
> Review of attachment 8500607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch does solve the problem at hand, but FTR I hate it.  :-)  I think
> we could probably make ThreadInfo refcountable, and make sRegisteredThreads
> hold a strong ref to those objects.  Then we could just drop them off of
> sRegisteredThreads and drop the strong ref, and delete the stack when the
> object is about to die.  Basically, if profiler_is_active(), we would put
> the ThreadInfo's in a different container which we'd clear out when the
> profiler is shut down.  That way we wouldn't have two ways of dealing with
> deleting the stack.  Please file a follow-up if you think that makes sense,
> but r=me regardless.

Refcount-ing would make this code safe but refcounting causes you to lose track of the lifetime of objects. We should discuss this in person. Jeff suggests using a unique_ptr with a get() for the TLS.

> ::: tools/profiler/platform-linux.cc
> @@ +505,5 @@
> > +      } else {
> > +        delete info;
> > +        sRegisteredThreads->erase(sRegisteredThreads->begin() + i);
> > +        break;
> > +      }
> 
> So many copies of this code :(

Yes, I've been wanting to clean up this code.

> 
> ::: tools/profiler/platform-win32.cc
> @@ +360,5 @@
> > +        info->SetPendingDelete();
> > +        break;
> > +      } else {
> > +        delete info;
> > +        sRegisteredThreads->erase(sRegisteredThreads->begin() + i);
> 
> You're not making anything worse here, but you're not allowed to mutate a
> vector as you're iterating over it in the general case.  Please file a
> follow-up and fix this.

I don't have an iterator live across an erase boundary. This is safe. I also break out of the loop after the first erase.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: v2 Fix gtest lifetime as well

Comment 8

[Benoit Girard (:BenWa)]

Attachment: v3

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

Comment 9

[Benoit Girard (:BenWa)]

Attachment: v4

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

Comment 10

[Benoit Girard (:BenWa)]

Attachment: v5

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

Comment 11

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7b39ffc873b9

Comment 12

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24ffba42510d

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/24ffba42510d