Bug 196956 - Web Inspector: use weak collections for holding event listeners
Summary: Web Inspector: use weak collections for holding event listeners
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 222431 222940
  Show dependency treegraph
 
Reported: 2019-04-15 21:13 PDT by Devin Rousso
Modified: 2021-03-11 14:05 PST (History)
19 users (show)

See Also:


Attachments
[Patch] WIP (21.09 KB, patch)
2019-04-15 23:34 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (27.58 KB, patch)
2019-04-16 02:08 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.32 MB, application/zip)
2019-04-16 03:19 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (3.03 MB, application/zip)
2019-04-16 04:08 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (2.89 MB, application/zip)
2019-04-16 14:20 PDT, EWS Watchlist
no flags Details
[Patch] WIP (120.23 KB, patch)
2019-06-18 22:11 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (121.57 KB, patch)
2019-06-18 22:59 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-highsierra (1.54 MB, application/zip)
2019-06-19 00:24 PDT, EWS Watchlist
no flags Details
[Patch] WIP (121.65 KB, patch)
2019-06-19 10:41 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (132.44 KB, patch)
2019-06-19 21:38 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (186.93 KB, patch)
2019-08-18 18:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.24 MB, application/zip)
2019-08-18 19:37 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.02 MB, application/zip)
2019-08-18 20:20 PDT, EWS Watchlist
no flags Details
Patch (264.56 KB, patch)
2020-10-10 17:51 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (267.53 KB, patch)
2020-10-10 21:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (272.45 KB, patch)
2020-11-03 19:35 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (272.51 KB, patch)
2020-11-03 23:40 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] followup (1.73 KB, patch)
2020-11-05 12:28 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-04-15 21:13:57 PDT
The current `WI.Object` can create reference cycles when it saves `thisObject` via `addEventListener`.  We should use a WeakMap/WeakSet to hold these objects (and maybe even the listener function) so that they can be GC'd.
Comment 1 Devin Rousso 2019-04-15 23:34:35 PDT
Created attachment 367502 [details]
[Patch] WIP
Comment 2 Devin Rousso 2019-04-16 02:08:26 PDT
Created attachment 367517 [details]
[Patch] WIP
Comment 3 EWS Watchlist 2019-04-16 03:19:51 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-04-16 03:19:53 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-04-16 04:08:20 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-04-16 04:08:22 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-04-16 14:20:42 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-04-16 14:20:44 PDT Comment hidden (obsolete)
Comment 9 Devin Rousso 2019-06-18 22:11:35 PDT
Created attachment 372431 [details]
[Patch] WIP
Comment 10 EWS Watchlist 2019-06-18 22:14:42 PDT Comment hidden (obsolete)
Comment 11 Devin Rousso 2019-06-18 22:18:02 PDT
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.
Comment 12 Devin Rousso 2019-06-18 22:59:55 PDT
Created attachment 372436 [details]
[Patch] WIP
Comment 13 EWS Watchlist 2019-06-19 00:24:35 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2019-06-19 00:24:37 PDT Comment hidden (obsolete)
Comment 15 Devin Rousso 2019-06-19 10:41:42 PDT
Created attachment 372477 [details]
[Patch] WIP

Add overflow check.
Comment 16 EWS Watchlist 2019-06-19 12:38:26 PDT Comment hidden (obsolete)
Comment 17 Devin Rousso 2019-06-19 21:38:52 PDT
Created attachment 372530 [details]
Patch
Comment 18 Devin Rousso 2019-06-19 21:42:14 PDT
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.
Comment 19 Joseph Pecoraro 2019-06-26 11:27:29 PDT
There seems to be work on a WeakRef type in Ecmascript. That might be better than this.
Comment 20 Devin Rousso 2019-06-26 11:38:28 PDT
(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 21 Joseph Pecoraro 2019-08-02 20:16:54 PDT
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.
Comment 22 Joseph Pecoraro 2019-08-15 12:21:59 PDT
Seems we can implement our own InspectorIterableWeakMap / InspectorIterableWeakSet using WeakRef?
Comment 23 Devin Rousso 2019-08-15 13:12:16 PDT
(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 24 Nikita Vasilyev 2019-08-15 13:30:01 PDT
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.
Comment 25 Nikita Vasilyev 2019-08-15 13:34:06 PDT
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.
Comment 26 Devin Rousso 2019-08-18 18:32:59 PDT
Created attachment 376656 [details]
Patch
Comment 27 EWS Watchlist 2019-08-18 19:37:11 PDT Comment hidden (obsolete)
Comment 28 EWS Watchlist 2019-08-18 19:37:13 PDT Comment hidden (obsolete)
Comment 29 EWS Watchlist 2019-08-18 20:20:44 PDT Comment hidden (obsolete)
Comment 30 EWS Watchlist 2019-08-18 20:20:46 PDT Comment hidden (obsolete)
Comment 31 BJ Burg 2020-01-06 09:25:29 PST
Comment on attachment 376656 [details]
Patch

Clearing r?, patch is quite stale at this point.
Comment 32 Devin Rousso 2020-10-10 17:51:09 PDT
Created attachment 411024 [details]
Patch

`WeakRef` and `FinalizationRegistry` were enabled in r268284
Comment 33 Devin Rousso 2020-10-10 21:32:22 PDT
Created attachment 411030 [details]
Patch
Comment 34 BJ Burg 2020-11-03 16:16:14 PST
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 35 Devin Rousso 2020-11-03 17:33:38 PST
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
Comment 36 Devin Rousso 2020-11-03 19:35:43 PST
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.
Comment 37 Devin Rousso 2020-11-03 23:40:03 PST
Created attachment 413142 [details]
Patch
Comment 38 EWS 2020-11-04 03:30:10 PST
Committed r269359: <https://trac.webkit.org/changeset/269359>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413142 [details].
Comment 39 Radar WebKit Bug Importer 2020-11-04 03:31:24 PST
<rdar://problem/71031192>
Comment 40 Truitt Savell 2020-11-05 12:18:11 PST
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.
Comment 41 Devin Rousso 2020-11-05 12:28:29 PST
Created attachment 413341 [details]
[Patch] followup
Comment 42 Joseph Pecoraro 2020-11-05 12:29:23 PST
Comment on attachment 413341 [details]
[Patch] followup

rs=me
Comment 43 Keith Miller 2020-11-05 12:30:39 PST
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 44 Ryan Haddad 2020-11-06 12:29:42 PST
Comment on attachment 413341 [details]
[Patch] followup

Clearing flags on attachment: 413341

Committed r269532: <https://trac.webkit.org/changeset/269532>
Comment 45 Ryan Haddad 2020-11-06 12:29:46 PST
All reviewed patches have been landed.  Closing bug.