Bug 196956 - Web Inspector: use weak collections for holding event listeners
Summary: Web Inspector: use weak collections for holding event listeners
Status: ASSIGNED
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:
Depends on:
Blocks:
 
Reported: 2019-04-15 21:13 PDT by Devin Rousso
Modified: 2019-09-03 17:18 PDT (History)
11 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, Build Bot
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, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (2.89 MB, application/zip)
2019-04-16 14:20 PDT, Build Bot
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, Build Bot
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
drousso: review?
ews: commit-queue-
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, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.02 MB, application/zip)
2019-08-18 20:20 PDT, Build Bot
no flags Details

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 Build Bot 2019-04-16 03:19:51 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2019-04-16 03:19:53 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-04-16 04:08:20 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-04-16 04:08:22 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-04-16 14:20:42 PDT Comment hidden (obsolete)
Comment 8 Build Bot 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 Build Bot 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 Build Bot 2019-06-19 00:24:35 PDT Comment hidden (obsolete)
Comment 14 Build Bot 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 Build Bot 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 Build Bot 2019-08-18 19:37:11 PDT
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
Comment 28 Build Bot 2019-08-18 19:37:13 PDT
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 29 Build Bot 2019-08-18 20:20:44 PDT
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
Comment 30 Build Bot 2019-08-18 20:20:46 PDT
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