Closed Bug 1433583 Opened 2 years ago Closed 2 years ago

Discard information about old dead threads that no longer have any samples in the buffer

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Example profile without this patch: https://perfht.ml/2BwvIvq
This profile has many grayed-out rows with threads that don't have any samples.
Comment on attachment 8945924 [details]
Bug 1433583 - Discard information about old dead threads that no longer have any samples in the buffer.

https://reviewboard.mozilla.org/r/216010/#review222116

::: tools/profiler/core/ThreadInfo.cpp:28
(Diff revision 1)
>                         int aThreadId,
>                         bool aIsMainThread,
>                         void* aStackTop)
>    : mName(strdup(aName))
>    , mRegisterTime(TimeStamp::Now())
> +  , mBufferPositionWhenUnregistered(0)

Shouldn't this be UINT64_MAX as that's the "invalid" buffer position marker?

::: tools/profiler/core/platform.cpp:294
(Diff revision 1)
> +  {
> +    // Discard any dead threads that were unregistered before aBufferRangeStart.
> +    ThreadVector& deadThreads = sInstance->mDeadThreads;
> +    for (size_t i = 0; i < deadThreads.size(); i++) {
> +      if (deadThreads.at(i)->BufferPositionWhenUnregistered() < aBufferRangeStart) {
> +        delete deadThreads.at(i);

This manual memory management is a bit unfortunate - it'd be nicer if we could have a vector of UniquePtr<T>s instead. 

This is fine though & that change should probably be done separately if at all.
Attachment #8945924 - Flags: review?(nika) → review+
Comment on attachment 8945924 [details]
Bug 1433583 - Discard information about old dead threads that no longer have any samples in the buffer.

https://reviewboard.mozilla.org/r/216010/#review222116

> Shouldn't this be UINT64_MAX as that's the "invalid" buffer position marker?

The value of this field wouldn't be used unless the thread was unregistered (at which point it would have a good value), but this wasn't enforced by the code. I've changed this field to a Maybe.

> This manual memory management is a bit unfortunate - it'd be nicer if we could have a vector of UniquePtr<T>s instead. 
> 
> This is fine though & that change should probably be done separately if at all.

I agree. I've filed bug 1434440 about it.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/dff05b1676b1
Discard information about old dead threads that no longer have any samples in the buffer. r=mystor
https://hg.mozilla.org/mozilla-central/rev/dff05b1676b1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.