Closed
Bug 1054110
Opened 10 years ago
Closed 10 years ago
Threads should not disappear from the profile immediately after they are destroyed
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
Attachments
(1 file, 5 obsolete files)
18.29 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Summary: Threads should disappear from the profile immediately after they are destroyed → Threads should not disappear from the profile immediately after they are destroyed
Assignee | ||
Comment 1•10 years ago
|
||
Some lifetime changes to address this problems. We're going to hold more memory while profiling and some threads go down.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8500571 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 2•10 years ago
|
||
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
Assignee | ||
Comment 3•10 years ago
|
||
Forgot to clean pending ThreadInfo on profiling stop. With v1 they stuck around until shutdown.
Attachment #8500571 -
Attachment is obsolete: true
Attachment #8500571 -
Flags: review?(ehsan.akhgari)
Attachment #8500607 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 4•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f74204443402
Comment 5•10 years ago
|
||
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.
Attachment #8500607 -
Flags: review?(ehsan.akhgari) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8500607 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9db4d054a5f1
Attachment #8501362 -
Attachment is obsolete: true
Attachment #8501368 -
Flags: review+
Assignee | ||
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=74601d898729
Attachment #8501368 -
Attachment is obsolete: true
Attachment #8502690 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6438b7ac9b24
Attachment #8502690 -
Attachment is obsolete: true
Attachment #8502703 -
Flags: review+
Assignee | ||
Comment 11•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7b39ffc873b9
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ffba42510d
https://hg.mozilla.org/mozilla-central/rev/24ffba42510d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•