RESOLVED FIXED 195148
Web Inspector: CPU Usage: Worker thread that dies might stay at a high value forever
https://bugs.webkit.org/show_bug.cgi?id=195148
Summary Web Inspector: CPU Usage: Worker thread that dies might stay at a high value ...
Joseph Pecoraro
Reported 2019-02-27 23:29:26 PST
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
Attachments
[PATCH] Proposed Fix (2.69 KB, patch)
2019-02-27 23:30 PST, Joseph Pecoraro
mattbaker: review+
Joseph Pecoraro
Comment 1 2019-02-27 23:30:27 PST
Created attachment 363196 [details] [PATCH] Proposed Fix
Matt Baker
Comment 2 2019-02-28 00:39:59 PST
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 3 2019-02-28 09:37:42 PST
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
Comment 4 2019-02-28 16:42:06 PST
(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
Comment 5 2019-02-28 16:48:58 PST
Radar WebKit Bug Importer
Comment 6 2019-02-28 16:49:18 PST
Note You need to log in before you can comment on or make changes to this bug.