CheckForApzAwareEventHandlers is expensive

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mattwoodrow, Assigned: smaug)

Tracking

(Blocks 3 bugs)

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments)

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?
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
I don't see CheckForTouchEventHandler anywhere in the tree though.
I'm pretty sure Matt meant CheckForApzAwareEventHandlers.
Summary: CheckForTouchEventHandler is expensive → CheckForApzAwareEventHandlers is expensive
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?
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.
Assignee: nobody → bugs
(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.
Posted patch v1Splinter Review
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.
Attachment #8662152 - Flags: review?(masayuki)
(we do have static assertions ensuring the flag count stays right even on 32 bit builds, so just waiting for try results.)
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
Attachment #8662152 - Flags: review?(masayuki) → review+
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.
Posted patch v2Splinter Review
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.
That was quick, thanks smaug!
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).
# 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.
> event dispatcher would be able to dispatch unnecessary events.

I meant "not to dispatch", sorry for the spam.
(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.)
https://hg.mozilla.org/mozilla-central/rev/2074a0012421
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.