195148 2019-02-27 23:29:26 -0800 Web Inspector: CPU Usage: Worker thread that dies might stay at a high value forever 2019-02-28 16:49:18 -0800 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 joepeck joepeck hi inspector-bugzilla-changes joepeck mattbaker webkit-bug-importer oldest_to_newest 1510827 0 joepeck 2019-02-27 23:29:26 -0800 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 1510828 1 363196 joepeck 2019-02-27 23:30:27 -0800 Created attachment 363196 [PATCH] Proposed Fix 1510865 2 363196 mattbaker 2019-02-28 00:39:59 -0800 Comment on attachment 363196 [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! 1510950 3 363196 hi 2019-02-28 09:37:42 -0800 Comment on attachment 363196 [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? 1511195 4 joepeck 2019-02-28 16:42:06 -0800 (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. 1511203 5 joepeck 2019-02-28 16:48:58 -0800 https://trac.webkit.org/r242243 1511204 6 webkit-bug-importer 2019-02-28 16:49:18 -0800 <rdar://problem/48494364> 363196 2019-02-27 23:30:27 -0800 2019-02-28 16:42:10 -0800 [PATCH] Proposed Fix workers-1.patch text/plain 2754 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IGUwNDQ5N2RhNjhjLi5kZjBhM2JkNDVlYSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTktMDItMjcgIEpvc2Vw aCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KKworICAgICAgICBXZWIgSW5zcGVjdG9y OiBDUFUgVXNhZ2U6IFdvcmtlciB0aHJlYWQgdGhhdCBkaWVzIG1pZ2h0IHN0YXkgYXQgYSBoaWdo IHZhbHVlIGZvcmV2ZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTE5NTE0OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgICogVXNlckludGVyZmFjZS9WaWV3cy9DUFVUaW1lbGluZVZpZXcuanM6CisgICAgICAg IChDUFVUaW1lbGluZVZpZXcucHJvdG90eXBlLmxheW91dCk6CisgICAgICAgIEhhbmRsZSB3b3Jr ZXJzIGRpZWluZyBvciBhdCBsZWFzdCB6ZXJvaW5nIG91dCBiZXR3ZWVuIHJlY29yZHMuCisKIDIw MTktMDItMjcgIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KIAogICAgICAg ICBXZWIgSW5zcGVjdG9yOiBDUFUgVXNhZ2UgVGltZWxpbmUgLSBNYWtlIFRocmVhZHMgc2VjdGlv biBleHBhbmRhYmxlIC8gY29sbGFwc2FibGUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0 b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL0NQVVRpbWVsaW5lVmlldy5qcyBiL1NvdXJjZS9XZWJJ bnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL0NQVVRpbWVsaW5lVmlldy5qcwppbmRleCAy YzA5ZTg3ZjkzOS4uZDU1MjQ4MWZjMjQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJJbnNwZWN0b3JV SS9Vc2VySW50ZXJmYWNlL1ZpZXdzL0NQVVRpbWVsaW5lVmlldy5qcworKysgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9DUFVUaW1lbGluZVZpZXcuanMKQEAgLTMw MSw2ICszMDEsNyBAQCBXSS5DUFVUaW1lbGluZVZpZXcgPSBjbGFzcyBDUFVUaW1lbGluZVZpZXcg ZXh0ZW5kcyBXSS5UaW1lbGluZVZpZXcKIAogICAgICAgICBsZXQgZGF0YVBvaW50cyA9IFtdOwog ICAgICAgICBsZXQgd29ya2Vyc0RhdGFNYXAgPSBuZXcgTWFwOworICAgICAgICBsZXQgd29ya2Vy c1NlZW5DdXJyZW50UmVjb3JkID0gbmV3IFNldDsKIAogICAgICAgICBsZXQgbWF4ID0gLUluZmlu aXR5OwogICAgICAgICBsZXQgbWFpblRocmVhZE1heCA9IC1JbmZpbml0eTsKQEAgLTM2MSw4ICsz NjIsMTIgQEAgV0kuQ1BVVGltZWxpbmVWaWV3ID0gY2xhc3MgQ1BVVGltZWxpbmVWaWV3IGV4dGVu ZHMgV0kuVGltZWxpbmVWaWV3CiAgICAgICAgICAgICB3ZWJraXRUaHJlYWRBdmVyYWdlICs9IHdl YmtpdFRocmVhZFVzYWdlOwogICAgICAgICAgICAgdW5rbm93blRocmVhZEF2ZXJhZ2UgKz0gdW5r bm93blRocmVhZFVzYWdlOwogCisgICAgICAgICAgICBsZXQgd29ya2Vyc1NlZW5MYXN0UmVjb3Jk ID0gd29ya2Vyc1NlZW5DdXJyZW50UmVjb3JkOworICAgICAgICAgICAgd29ya2Vyc1NlZW5DdXJy ZW50UmVjb3JkID0gbmV3IFNldDsKKwogICAgICAgICAgICAgaWYgKHJlY29yZC53b3JrZXJzRGF0 YSAmJiByZWNvcmQud29ya2Vyc0RhdGEubGVuZ3RoKSB7CiAgICAgICAgICAgICAgICAgZm9yIChs ZXQge3RhcmdldElkLCB1c2FnZX0gb2YgcmVjb3JkLndvcmtlcnNEYXRhKSB7CisgICAgICAgICAg ICAgICAgICAgIHdvcmtlcnNTZWVuQ3VycmVudFJlY29yZC5hZGQodGFyZ2V0SWQpOwogICAgICAg ICAgICAgICAgICAgICBsZXQgd29ya2VyRGF0YSA9IHdvcmtlcnNEYXRhTWFwLmdldCh0YXJnZXRJ ZCk7CiAgICAgICAgICAgICAgICAgICAgIGlmICghd29ya2VyRGF0YSkgewogICAgICAgICAgICAg ICAgICAgICAgICAgd29ya2VyRGF0YSA9IHsKQEAgLTQwMiw2ICs0MDcsMTYgQEAgV0kuQ1BVVGlt ZWxpbmVWaWV3ID0gY2xhc3MgQ1BVVGltZWxpbmVWaWV3IGV4dGVuZHMgV0kuVGltZWxpbmVWaWV3 CiAgICAgICAgICAgICAgICAgICAgIHdvcmtlckRhdGEuYXZlcmFnZSArPSB1c2FnZTsKICAgICAg ICAgICAgICAgICB9CiAgICAgICAgICAgICB9CisKKyAgICAgICAgICAgIC8vIENsb3NlIGFueSB3 b3JrZXIgdGhhdCBkaWVkIGJ5IGRyb3BwaW5nIHRvIHplcm8uCisgICAgICAgICAgICBpZiAod29y a2Vyc1NlZW5MYXN0UmVjb3JkLnNpemUpIHsKKyAgICAgICAgICAgICAgICBsZXQgZGVhZFdvcmtl cnMgPSB3b3JrZXJzU2Vlbkxhc3RSZWNvcmQuZGlmZmVyZW5jZSh3b3JrZXJzU2VlbkN1cnJlbnRS ZWNvcmQpOworICAgICAgICAgICAgICAgIGZvciAobGV0IHdvcmtlcklkIG9mIGRlYWRXb3JrZXJz KSB7CisgICAgICAgICAgICAgICAgICAgIGxldCB3b3JrZXJEYXRhID0gd29ya2Vyc0RhdGFNYXAu Z2V0KHdvcmtlcklkKTsKKyAgICAgICAgICAgICAgICAgICAgaWYgKHdvcmtlckRhdGEuZGF0YVBv aW50cy5sYXN0VmFsdWUudXNhZ2UgIT09IDApCisgICAgICAgICAgICAgICAgICAgICAgICB3b3Jr ZXJEYXRhLmRhdGFQb2ludHMucHVzaCh7dGltZSwgdXNhZ2U6IDB9KTsKKyAgICAgICAgICAgICAg ICB9CisgICAgICAgICAgICB9CiAgICAgICAgIH0KIAogICAgICAgICBhdmVyYWdlIC89IHZpc2li bGVSZWNvcmRzLmxlbmd0aDsK