1205466 - CheckForApzAwareEventHandlers is expensive

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

On my reddit.com/r/space testcase, we spend about 5% of our display list building time within this function.

Almost all of the time is within nsContentUtils::GetExistingListenerManagerForNode, though a small amount is in EventListenerManager::HasListenersFor().

Does anyone know if there's a way we can compute this once and then get notified if the result is going to change?

Comment 1

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Hmm, we still have SetHasTouchEventListeners() in nsPIDOMWindow, and that state is even set, but apparently not ever used.
http://mxr.mozilla.org/mozilla-central/source/dom/events/EventListenerManager.cpp#351
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h#453

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

I don't see CheckForTouchEventHandler anywhere in the tree though.

Comment 3

[Botond Ballo [:botond]]

I'm pretty sure Matt meant CheckForApzAwareEventHandlers.

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Ah, and it is about the events which nsLayoutUtils::HasApzAwareListeners checks.
So yes, we could add flags for each of those events, or perhaps just mMayHaveApzDependedEvents
to nsPIDOMWindow and update it similarly to other flags we have there and which we update from EventListenerManager and in nsNodeUtils.
Or just keep the flag in EventListenerManager. That might be the best option.

When is CheckForApzAwareEventHandlers called? For which all nodes?

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Though, if it is getting the ELM which is slow, we need the flag in nsINode, so that we don't even need to access ELM. I think we have still some spare bits in nsINode.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Olli Pettay [:smaug] from comment #1)
> Hmm, we still have SetHasTouchEventListeners() in nsPIDOMWindow, and that
> state is even set, but apparently not ever used.
> http://mxr.mozilla.org/mozilla-central/source/dom/events/
> EventListenerManager.cpp#351
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h#453

It is used by Fennec, at least until we switch it over to APZ. I tried removing it before and stuff broke.

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef7ec2670064
I don't want to change the existing behavior, so the list of events stays the same (although mousewheel is silly).

Decided to keep the flag as a hint, which isn't updated when a listener is removed, since that tends to happen rarely, and updating the flag in that case would make removing event listeners in general slower.

Seems to cut most of the apz related GetExistingListenerManager calls when loading reddit.com/r/space


Hopefully I counted the flags correctly, so that there is still that last spare bit even on 32bit builds.

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(we do have static assertions ensuring the flag count stays right even on 32 bit builds, so just waiting for try results.)

Comment 9

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

Comment on attachment 8662152 [details] [diff] [review]
v1

> #define NS_INODE_IID \
>-{ 0xe8fdd227, 0x27da, 0x46ee, \
>-  { 0xbe, 0xf3, 0x1a, 0xef, 0x5a, 0x8f, 0xc5, 0xb4 } }
>+{ 0x70ba4547, 0x7699, 0x44fc, \
>+  { 0xb3, 0x20, 0x52, 0xdb, 0xe3, 0xd1, 0xf9, 0x0a } }

Don't you need to modify IID of nsINode derived interfaces too? E.g., nsIContent and nsIAtribute? DXR had the feature to check which classes are derived from an class (even if the super class is one of ancestors). However, I don't see the feature with current DXR...


>@@ -400,16 +400,23 @@ EventListenerManager::AddEventListenerIn
>              aTypeAtom == nsGkAtoms::oncompositionstart ||
>              aTypeAtom == nsGkAtoms::oncompositionupdate ||
>              aTypeAtom == nsGkAtoms::oninput) {
>     if (!aFlags.mInSystemGroup) {
>       mMayHaveInputOrCompositionEventListener = true;
>     }
>   }
> 
>+  if (IsApzAwareEvent(aTypeAtom)) {
>+    nsCOMPtr<nsINode> n = do_QueryInterface(mTarget);
>+    if (n) {
>+      n->SetMayHaveApzAwareListeners();
>+    }
>+  }

Only one character variable name isn't good, please use "node" or something.

>+bool
>+EventListenerManager::HasApzAwareListeners()
>+{
>+  uint32_t count = mListeners.Length();
>+  for (uint32_t i = 0; i < count; ++i) {
>+    Listener* listener = &mListeners.ElementAt(i);
>+    if (IsApzAwareEvent(listener->mTypeAtom)) {
>+      return true;
>+    }
>+  }
>+  return false;
>+}

Even if this is slow, we can add similar bool member to ELM too :-)

>+bool
>+EventListenerManager::IsApzAwareEvent(nsIAtom* aEvent)
>+{
>+  return aEvent == nsGkAtoms::ontouchstart ||
>+         aEvent == nsGkAtoms::ontouchmove ||
>+         aEvent == nsGkAtoms::onwheel ||
>+         aEvent == nsGkAtoms::onDOMMouseScroll ||
>+         aEvent == nsHtml5Atoms::onmousewheel ||

Oh, why do we have onmousewheel? As far as I know we haven't supported it and wont' support forever. But this must not be your fault even if I correct. So, keeping checking onmousewheel is okay.

>+         aEvent == nsGkAtoms::onMozMousePixelScroll;
>+}

> // IID for the dom::EventTarget interface
> #define NS_EVENTTARGET_IID \
>-{ 0x605158a9, 0xe229, 0x45b1, \
>- { 0xbc, 0x12, 0x02, 0x9f, 0xa3, 0xa9, 0x3f, 0xcb } }
>+{ 0xde651c36, 0x0053, 0x4c67, \
>+  { 0xb1, 0x3d, 0x67, 0xb9, 0x40, 0xfc, 0x82, 0xe4 } }

Same as nsINode. E.g., Shouldn't you to change nsPIWindowRoot's IID?


With fixing the variable name and the IID issue, r=masayuki

Comment 10

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Usually we don't actually update all the IIDs in inheriting classes, but we perhaps should.
For .idl's uuids we have nice tools and everything.

Comment 11

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: v2

And yes, we do have some spare bits in ELM too, so we could have another flag there too, but this patch should help already in the common case.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2074a0012421

Comment 13

[Matt Woodrow (:mattwoodrow)]

That was quick, thanks smaug!

Comment 14

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Feel free to test. I never managed to see anywhere near 5% of display list construction time, but it should be faster now, and can be optimized still some more if needed (at the cost of making removeEventListener slower).

Comment 15

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

# off-topic, though

(In reply to Olli Pettay [:smaug] from comment #14)
> and can be optimized still some more if needed (at the cost of making removeEventListener slower).

I'm not sure the importance of the performance of removeEventListener because I don't think that removeEventListener() is called not so many time per web app. If we could ignore the cost of removeEventListener, event dispatcher would be able to dispatch unnecessary events. That *might* improve the battery life.

Comment 16

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

> event dispatcher would be able to dispatch unnecessary events.

I meant "not to dispatch", sorry for the spam.

Comment 17

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(that is indeed off-topic, but the issue is that we'd need to still know about default handling, which in general happens in someEventTarget::PostHandleEvent() and doesn't depend on event listeners to exist.)

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2074a0012421