Bug 234441 - Introduce a fast path for replacing an attribute event listener
Summary: Introduce a fast path for replacing an attribute event listener
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-17 14:14 PST by Alexey Shvayka
Modified: 2022-02-15 13:20 PST (History)
6 users (show)

See Also:

Attachments
Patch (23.04 KB, patch)
2021-12-17 14:19 PST, Alexey Shvayka 		no flags Details | Formatted Diff | Diff
Microbenchmark (916 bytes, text/html)
2021-12-17 14:20 PST, Alexey Shvayka 		no flags Details
Patch for running Speedometer2/Inferno-TodoMVC locally (23.94 KB, patch)
2021-12-17 14:27 PST, Alexey Shvayka 		no flags Details | Formatted Diff | Diff
Patch for landing (23.10 KB, patch)
2021-12-19 07:07 PST, Alexey Shvayka 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2021-12-17 14:14:00 PST 
Introduce a fast path for replacing an attribute event listener
Comment 1 Alexey Shvayka 2021-12-17 14:19:39 PST 
Created attachment 447474 [details]
Patch
Comment 2 Alexey Shvayka 2021-12-17 14:20:28 PST 
Created attachment 447475 [details]
Microbenchmark

r287168: 618ms
patch: 237ms (2.6x faster)
(50 runs)
Comment 3 Alexey Shvayka 2021-12-17 14:27:45 PST 
Created attachment 447476 [details]
Patch for running Speedometer2/Inferno-TodoMVC locally

r287168
===
score: 370.16
median: 372.38

patch
===
score: 385.03 (4% faster)
median: 387.87 (4.16% faster)

Median of 8 runs of `Tools/Scripts/run-perf-tests --repeat=4 --test-runner-count=30 PerformanceTests/Speedometer --no-build --release --no-show-results` with "inferno-todomvc-tests.patch" applied.
Comment 4 Chris Dumez 2021-12-17 15:54:27 PST 
Comment on attachment 447474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447474&action=review

r=me

> Source/WebCore/bindings/js/JSEventListener.cpp:86
> +    m_jsFunction = JSC::Weak<JSC::JSObject>(jsFunction);

m_jsFunction = JSC::Weak { jsFunction };

Should work and is more concise.

> Source/WebCore/bindings/js/JSEventListener.h:88
> +template<class JSMaybeErrorEventListener>

nit: I think we usually use "typename" instead of "class". Same comment below in this file.

> Source/WebCore/dom/EventTarget.cpp:166
> +    DOMWrapperWorld& isolatedWorld = worldForDOMObject(jsEventTarget);

auto& ?
Comment 5 Yusuke Suzuki 2021-12-19 02:27:58 PST 
This is awesome!
Comment 6 Alexey Shvayka 2021-12-19 07:07:06 PST 
Created attachment 447548 [details]
Patch for landing

Account for review comments and set m_wrapper to prevent ASSERT failures.
Comment 7 Alexey Shvayka 2021-12-19 07:07:59 PST 
(In reply to Chris Dumez from comment #4)
> Comment on attachment 447474 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447474&action=review
> 
> r=me

Thank you for review, Chris! All comments were addressed.

I was worried this change may get rejected because it adds a bit of JSC / bindings stuff to the EventTarget, yet on the second thought EventTarget already has a notion of attribute event listeners, this patch just makes it more explicit.
Comment 8 EWS 2021-12-20 18:16:43 PST 
Committed r287293 (245445@main): <https://commits.webkit.org/245445@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447548 [details].
Comment 9 Radar WebKit Bug Importer 2021-12-20 18:17:17 PST 
<rdar://problem/86748822>
Comment 10 Alexey Shvayka 2022-01-26 11:32:19 PST 
(In reply to Alexey Shvayka from comment #2)
> Created attachment 447475 [details]
> Microbenchmark
> 
> r287168: 618ms
> patch: 237ms (2.6x faster)
> (50 runs)

Sadly, this change regressed after the review and before it was landed, which should be fixed in https://bugs.webkit.org/show_bug.cgi?id=235658 (r?).