1273511 - Optimize EventListenerManager::AddEventListenerByType

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

It does too many hashtable lookups and temporary string creations etc.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Also atomizations, free() calls... Lots of stuff in there.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

yup, it all reduces to one hashtable lookup.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05b3bdb6e24afbc24c8f208f9ceb21f9e42dc785

This reduces the temporary string creation and hashtable lookups.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8753390 [details] [diff] [review]
patch

I don't like adding bool argument because it's not unclear from caller side. But I have no better idea.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

yeah, the method call is a bit ugly, but luckily this is used rarely. And since the method takes weird params anyhow, one needs to look at the documentation each time one uses it.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I need to look at the SVG/SMIL events. I may have made a mistake there.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d47a2084eda

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 + tests for SMIL/SVG

We pass the test current, and with this patch, but the previous patch didn't.
And yes, SVG/SMIL events are crazy.

(looks like try is closed atm)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99bb89c02cfcdccefd6e124c0a27d23228f58783

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8754545 [details] [diff] [review]
v2 + tests for SMIL/SVG

>+  if (sStringEventTable->Get(aName, &mapping)) {
>+    if (mapping.mMaybeSpecialSVGorSMILEvent) {
>+      // Try the atom version so that we should get the right message for
>+      // SVG/SMIL.
>+      atom = NS_Atomize(NS_LITERAL_STRING("on") + aName);
>+      msg = GetEventMessage(atom);

The events are not so many, so, it might be faster to use switch statement instead of hashtable. However, I guess that the hashtable's cost is not so better than switch statement and this is stronger for maintain.

I think that we should make the SVG/SMIL events constructed as usual in another bug, though.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2974fa22af9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2974fa22af9