Bug 234441 – Introduce a fast path for replacing an attribute event listener

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Alexey Shvayka]

Introduce a fast path for replacing an attribute event listener

[Alexey Shvayka]

Created attachment 447474 [details]
Patch

[Alexey Shvayka]

Created attachment 447475 [details]
Microbenchmark

r287168: 618ms
patch: 237ms (2.6x faster)
(50 runs)

[Alexey Shvayka]

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.

[Chris Dumez]

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& ?

[Yusuke Suzuki]

This is awesome!

[Alexey Shvayka]

Created attachment 447548 [details]
Patch for landing

Account for review comments and set m_wrapper to prevent ASSERT failures.

[Alexey Shvayka]

(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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86748822>

[Alexey Shvayka]

(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?).