Summary: | Web Inspector: use weak collections for holding event listeners | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||||||||||||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | bburg, eric.carlson, ews-watchlist, glenn, hi, inspector-bugzilla-changes, jer.noble, joepeck, keith_miller, mattbaker, nvasilyev, philipj, rniwa, saam, sergio, timothy, tsavell, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=17240 https://bugs.webkit.org/show_bug.cgi?id=223090 |
||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 222431, 222940 | ||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2019-04-15 21:13:57 PDT
Created attachment 367502 [details]
[Patch] WIP
Created attachment 367517 [details]
[Patch] WIP
Comment on attachment 367517 [details] [Patch] WIP Attachment 367517 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11883731 New failing tests: http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html inspector/unit-tests/event-listener-set.html inspector/unit-tests/event-listener.html Created attachment 367523 [details]
Archive of layout-test-results from ews103 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 367517 [details] [Patch] WIP Attachment 367517 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11884067 New failing tests: http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html inspector/unit-tests/event-listener-set.html inspector/unit-tests/event-listener.html Created attachment 367527 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 367517 [details] [Patch] WIP Attachment 367517 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11889201 New failing tests: http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html inspector/unit-tests/event-listener-set.html inspector/unit-tests/event-listener.html Created attachment 367568 [details]
Archive of layout-test-results from ews113 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 372431 [details]
[Patch] WIP
Attachment 372431 [details] did not pass style-queue:
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:2337: Path does not exist. [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:2338: Path does not exist. [test/expectations] [5]
Total errors found: 2 in 46 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 372431 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=372431&action=review > Source/WebInspectorUI/UserInterface/Base/Object.js:37 > + console.assert(eventType, "Object.addEventListener: invalid eventType ", eventType, "(listener: ", listener, " thisObject: ", thisObject, ")"); Part of me also want's to add an `assert` that `eventType` exists in a `WI.*.Event` object that's related to `this` (e.g. `Object.values(this.prototype.constructor.Event).includes(eventType)`), but I'm not sure how worth it that would be, as it can basically be caught in style review instead. > Source/WebInspectorUI/UserInterface/Base/Object.js:45 > + if (window.InspectorTest && !thisObject) I'd like to remove this in the future (e.g. make all tests provide a `thisObject`), but I think this is fine for now, as it's basically guaranteed to keep the listener alive. I'm also not against the idea of allowing this for non-tests too, as it would eliminate much of the diff in this patch :P > Source/WebInspectorUI/UserInterface/Base/Object.js:67 > + console.assert(!listenersForThisObject.has(listener), "Object.addEventListener: duplicate listener ", listener, "(eventType: ", eventType, " thisObject ", thisObject, ")"); I added this extra `assert` so we can find places where we're doing unnecessary work :) > Source/WebInspectorUI/UserInterface/Base/Object.js:90 > + this._listeners.forEach((thisObjectsForEventType) => { NIT: this doesn't need to be an arrow function. Created attachment 372436 [details]
[Patch] WIP
Comment on attachment 372436 [details] [Patch] WIP Attachment 372436 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12517473 Number of test failures exceeded the failure limit. Created attachment 372441 [details]
Archive of layout-test-results from ews116 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 372477 [details]
[Patch] WIP
Add overflow check.
Comment on attachment 372477 [details] [Patch] WIP Attachment 372477 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12522807 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl apiTests Created attachment 372530 [details]
Patch
Comment on attachment 372530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372530&action=review > Source/WebInspectorUI/UserInterface/Base/Object.js:-160 > - let listeners = listenersTable.toArray(); The only downside I can see to using a `WeakMap` is that it no longer preserves the order if there's more than one `thisObject`. Since `listener`s are stored in a `Set` (per `thisObject`), the order of execution (per `thisObject`) will match the order of insertion. If I had the following scenario: Object A adds Listener 1 Object B adds Listener 2 Object A adds Listener 3 Then the new order of execution would be [A1, A3, B2], which wouldn't match the "true" execution order. I think this is probably fine, however, as we likely don't have that many (if any) cases where the same object adds multiple different listeners to the same target, and for any where that's the case we should probably remove them. There seems to be work on a WeakRef type in Ecmascript. That might be better than this. (In reply to Joseph Pecoraro from comment #19) > There seems to be work on a WeakRef type in Ecmascript. That might be better than this. If we use attachment 372530 [details] as is, the I don't think so actually. All it would entail is replacing the `WeakMap` with a `Map` where the keys are `WeakRef`s. The same issues with order preservation and anonymous functions being used as the `listener` without a `thisObject` still apply, albeit in a slightly different form. With that having been said, we could instead just use `WeakRef` in place of the `thisObject` with the existing `ListMultimap`, but frankly I'd like to simplify that system anyways. Another option would be to ditch the concept of a `Map` altogether and just use an array of `{thisObject: WeakRef(...), listener}` items. Calls to `addEventListener` and `removeEventListener` are "rare" (when compared to how often the `listener` itself is fired/used) that I think we can "afford" being less efficient there in order to preserve the order of addition. Comment on attachment 372530 [details]
Patch
I don't like the idea of only Web Inspector having custom JS to iterate WeakMap/Set. I'd rather we wait for something like WeakRef. I'm going to r- but let me know if you disagree and I can take another look.
Seems we can implement our own InspectorIterableWeakMap / InspectorIterableWeakSet using WeakRef? (In reply to Joseph Pecoraro from comment #22) > Seems we can implement our own InspectorIterableWeakMap / InspectorIterableWeakSet using WeakRef? That's basically my plan. I'm thinking of moving back to an array of `{listener, thisObject}` and removing most of the `removeEventListener(null, null, this)` that wouldn't be needed anymore. Comment on attachment 372530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372530&action=review > Source/WebInspectorUI/ChangeLog:8 > + Replace the `ListMultimap` with a `WeakMap` and the `LinkedList` with a `Set` for listeners. Note that 3-4 years ago when, iterating a linked list was significantly faster than Set. Removing, adding, and triggering event listeners is a performance sensitive part of Web Inspector. It's worth measuring how long it takes to remove, add, fire >1000 events. Created attachment 376656 [details]
Patch
Comment on attachment 376656 [details] Patch Attachment 376656 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12938272 New failing tests: inspector/unit-tests/resource-collection.html inspector/script-profiler/event-type-Other.html inspector/runtime/promise-native-getter.html inspector/unit-tests/protocol-test-dispatch-event-to-frontend.html inspector/unit-tests/event-listener.html Created attachment 376661 [details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376656 [details] Patch Attachment 376656 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12938309 New failing tests: inspector/unit-tests/resource-collection.html inspector/script-profiler/event-type-Other.html inspector/runtime/promise-native-getter.html inspector/unit-tests/protocol-test-dispatch-event-to-frontend.html inspector/unit-tests/event-listener.html Created attachment 376662 [details]
Archive of layout-test-results from ews114 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376656 [details]
Patch
Clearing r?, patch is quite stale at this point.
Created attachment 411024 [details] Patch `WeakRef` and `FinalizationRegistry` were enabled in r268284 Created attachment 411030 [details]
Patch
Comment on attachment 411030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411030&action=review r=me, great work! > Source/WebInspectorUI/ChangeLog:265 > + Remove the `closed` method that only `removeEventListener` as they are no longer needed Nit: grammaro > Source/WebInspectorUI/UserInterface/Base/Object.js:60 > + let wrappedCallback = (...args) => { Nice to get rid of unneeded `arguments`! > Source/WebInspectorUI/UserInterface/Base/Object.js:124 > // Make a copy with slice so mutations during the loop doesn't affect us. Nit: unnecessary comment. > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:566 > + let promises = [this.awaitEvent(WI.DebuggerManager.Event.Paused, this)]; Nice! > Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:38 > +}, WI.settings.engineeringPauseForInternalScripts); LOL, oops > Source/WebInspectorUI/UserInterface/Models/Resource.js:1037 > + this.singleFireEventListener(WI.Resource.Event.LoadingDidFinish, resolve, this); Nice. > Source/WebInspectorUI/UserInterface/Views/AuditTreeElement.js:100 > + if (this.representedObject instanceof WI.AuditTestBase) { Cranky logic like this is why I wrote EventListenerSet in the first place. > Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:493 > + this.dispatchEventToListeners(DataGridNode.Event.Populate); O_O > Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:-108 > - WI.HeapSnapshotWorkerProxy.singleton().addEventListener("HeapSnapshot.CollectionEvent", this._heapSnapshotCollectionEvent, this); o_O > Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:-264 > - WI.HeapSnapshotWorkerProxy.singleton().removeEventListener("HeapSnapshot.CollectionEvent", this._heapSnapshotCollectionEvent, this); It seems like we lost this removeEventListener. Comment on attachment 411030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411030&action=review >> Source/WebInspectorUI/UserInterface/Views/AuditTreeElement.js:100 >> + if (this.representedObject instanceof WI.AuditTestBase) { > > Cranky logic like this is why I wrote EventListenerSet in the first place. Yeah I can see the appeal. IMO this happens so infrequently that it's not worth another data structure. If anything, I'd rather loosen the restrictions around `removeEventListener` so that you can call it even when nothing is registered which would allow us to drop all the `if`. I do see a benefit to having an assertion that the registration exists, but if others feel strongly I can change it (and this). >> Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:493 >> + this.dispatchEventToListeners(DataGridNode.Event.Populate); > > O_O ikr >> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:-264 >> - WI.HeapSnapshotWorkerProxy.singleton().removeEventListener("HeapSnapshot.CollectionEvent", this._heapSnapshotCollectionEvent, this); > > It seems like we lost this removeEventListener. This should just disappear when the `WI.HeapAllocationsTimelineView` is destructed, which should only ever happen if the user hides the heap allocations timeline. EDIT: after discussing this offline with you, I've decided to undo my removal of all the `removeEventListener` in `closed` throughout this patch in the hopes of not introducing subtle regressions and keeping behavior as close to what it was before Created attachment 413134 [details]
Patch
streamline changes by
- not eliminating `closed` and instead just replacing any `removeEventListener(null, null, this)` therein
- not adding `closed` to handle `WI.DataGrid`, etc.
Created attachment 413142 [details]
Patch
Committed r269359: <https://trac.webkit.org/changeset/269359> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413142 [details]. It looks like the changes in https://trac.webkit.org/changeset/269359/webkit broke inspector/unit-tests/heap-snapshot-collection-event.html History: https://results.webkit.org/?suite=layout-tests&test=inspector%2Funit-tests%2Fheap-snapshot-collection-event.html Diff: --- /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/inspector/unit-tests/heap-snapshot-collection-event-expected.txt +++ /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/inspector/unit-tests/heap-snapshot-collection-event-actual.txt @@ -8,9 +8,7 @@ -- Running test case: HeapSnapshotCollectionEvent PASS: Should not have an error creating a snapshot. -PASS: Received HeapSnapshot.CollectionEvent. -PASS: Collection should include at least 200 nodes (100 objects and 100 strings). -PASS: Collection should affect the first snapshot. +!! TIMEOUT: took longer than 10000ms -- Running test case: HeapSnapshot.prototype.update PASS: 0 Objects were dead before. Created attachment 413341 [details]
[Patch] followup
Comment on attachment 413341 [details]
[Patch] followup
rs=me
Comment on attachment 413341 [details]
[Patch] followup
FWIW, it seems wrong that this test is expecting every object to be collected. That's a recipe for a flaky test, you should probably only test that at least one object was collected.
Comment on attachment 413341 [details] [Patch] followup Clearing flags on attachment: 413341 Committed r269532: <https://trac.webkit.org/changeset/269532> All reviewed patches have been landed. Closing bug. |