Bug 195148 – Web Inspector: CPU Usage: Worker thread that dies might stay at a high value forever

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

CPU Usage: Worker thread that dies might stay at a high value forever

Steps to Reproduce:
1. Inspect maps.google.com
2. Reload
  => If a Worker Thread is sampled high and then dies it may stay around forever at a high value

Notes:
- Need to kill off dead workers in the range

[Joseph Pecoraro]

Created attachment 363196 [details]
[PATCH] Proposed Fix

[Matt Baker]

Comment on attachment 363196 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=363196&action=review

r=me

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:413
> +                let deadWorkers = workersSeenLastRecord.difference(workersSeenCurrentRecord);

Cool!

[Devin Rousso]

Comment on attachment 363196 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=363196&action=review

> Source/WebInspectorUI/ChangeLog:10
> +        Handle workers dieing or at least zeroing out between records.

s/dieing/dying

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:304
> +        let workersSeenCurrentRecord = new Set;

Having "record" makes me think that this has something to do with actual timeline records.  I think a name without "record" in it (or one that explains more that the records are what you're filtering off of, like `workersWithRecentRecords`) would be better.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:366
> +            let workersSeenLastRecord = workersSeenCurrentRecord;
> +            workersSeenCurrentRecord = new Set;

Rather than try to do some fancy checking of usage numbers, couldn't you check with `WI.WorkerManager` and see if the `workerId` is still active?  It seems to me that that's the only way of knowing whether a worker is "dead".

Are you trying to make it so that alive workers with no activity don't appear, or that actually dead (`terminate`d) workers don't appear?

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 363196 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363196&action=review
> 
> > Source/WebInspectorUI/ChangeLog:10
> > +        Handle workers dieing or at least zeroing out between records.
> 
> s/dieing/dying
> 
> > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:304
> > +        let workersSeenCurrentRecord = new Set;
> 
> Having "record" makes me think that this has something to do with actual
> timeline records.  I think a name without "record" in it (or one that
> explains more that the records are what you're filtering off of, like
> `workersWithRecentRecords`) would be better.

I'll go with workersSeen*In*CurrentRecord or workersSeen*In*LastRecord

> 
> > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:366
> > +            let workersSeenLastRecord = workersSeenCurrentRecord;
> > +            workersSeenCurrentRecord = new Set;
> 
> Rather than try to do some fancy checking of usage numbers, couldn't you
> check with `WI.WorkerManager` and see if the `workerId` is still active?  It
> seems to me that that's the only way of knowing whether a worker is "dead".
> 
> Are you trying to make it so that alive workers with no activity don't
> appear, or that actually dead (`terminate`d) workers don't appear?

The timeline range selection might be a period of time in the past where a worker existed, but no longer exist. So checking WorkerManager/TargetManager could be insufficient.

Dropping any worker thread to zero if not seen again is a safe way of handling this regardless of any outside data from the CPU Timeline Records themselves.

[Joseph Pecoraro]

https://trac.webkit.org/r242243

[Radar WebKit Bug Importer]

<rdar://problem/48494364>