541474 - make events coalescing smarter

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

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

r=me conditional on our tests passing (please include our disabled coalescence test).

Overall it makes sense that we would only check ancestral relationship for show, hide, reorder situations. Other worrisome cases don't come to mind.

s/descedant/descendant/

>diff --git a/accessible/src/base/nsEventShell.cpp b/accessible/src/base/nsEventShell.cpp
>--- a/accessible/src/base/nsEventShell.cpp
>+++ b/accessible/src/base/nsEventShell.cpp
>@@ -248,51 +248,87 @@ nsAccEventQueue::CoalesceEvents()
>             CoalesceReorderEventsFromSameSource(thisEvent, tailEvent);
>             continue;
>           }
> 
>           // Dupe
>           thisEvent->mEventRule = nsAccEvent::eDoNotEmit;
>           continue;
>         }
>-        if (nsCoreUtils::IsAncestorOf(tailEvent->mNode, thisEvent->mNode)) {
>-          // thisDOMNode is a descendant of tailDOMNode
>+
>+        // More older show event target (thisNode) can't be contained by recent
>+        // show event target (tailNode), i.e be a descedant of tailNode.
>+        // XXX: target of older show event caused by DOM node appending can be
>+        // contained by target of recent show event caused by style change.
>+        // XXX: target of older show event caused by style change can be
>+        // contained by target of recent show event caused by style change.

It seems like the XXX comments contradict the comment preceding them. What is the impact of this?

>+        PRBool thisCanBeDescedantOfTail =
>+          tailEvent->mEventType != nsIAccessibleEvent::EVENT_SHOW ||
>+          tailEvent->mIsAsync;
>+
>+        if (thisCanBeDescedantOfTail &&
>+            nsCoreUtils::IsAncestorOf(tailEvent->mNode, thisEvent->mNode)) {
>+          // thisNode is a descendant of tailNode.

OK so we'll save some cycles by not calculating IsAncestorOf when the "if" bails early, which happens with show events, or when the tailEvent is async. 

>+        // More older hide event target (thisNode) can't contain recent hide
>+        // event target (tailNode), i.e. be ancestor of tailNode.
>+        if (tailEvent->mEventType != nsIAccessibleEvent::EVENT_HIDE &&
>+            nsCoreUtils::IsAncestorOf(thisEvent->mNode, tailEvent->mNode)) {

This looks like a perf improvement as well. (?)

Did you run this against bz's test case?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)

> >+        // More older show event target (thisNode) can't be contained by recent
> >+        // show event target (tailNode), i.e be a descedant of tailNode.
> >+        // XXX: target of older show event caused by DOM node appending can be
> >+        // contained by target of recent show event caused by style change.
> >+        // XXX: target of older show event caused by style change can be
> >+        // contained by target of recent show event caused by style change.
> 
> It seems like the XXX comments contradict the comment preceding them. What is
> the impact of this?

Yes, XXX means bug we should address. The statement above should be valid, but these XXX break it.

> 
> >+        PRBool thisCanBeDescedantOfTail =
> >+          tailEvent->mEventType != nsIAccessibleEvent::EVENT_SHOW ||
> >+          tailEvent->mIsAsync;
> >+
> >+        if (thisCanBeDescedantOfTail &&
> >+            nsCoreUtils::IsAncestorOf(tailEvent->mNode, thisEvent->mNode)) {
> >+          // thisNode is a descendant of tailNode.
> 
> OK so we'll save some cycles by not calculating IsAncestorOf when the "if"
> bails early, which happens with show events, or when the tailEvent is async. 

quite the contrary: bail early if not show events or if tailEvent is sync.

> 
> >+        // More older hide event target (thisNode) can't contain recent hide
> >+        // event target (tailNode), i.e. be ancestor of tailNode.
> >+        if (tailEvent->mEventType != nsIAccessibleEvent::EVENT_HIDE &&
> >+            nsCoreUtils::IsAncestorOf(thisEvent->mNode, tailEvent->mNode)) {
> 
> This looks like a perf improvement as well. (?)

yes, this is all this bug about.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> (From update of attachment 423036 [details] [diff] [review])
> r=me conditional on our tests passing (please include our disabled coalescence
> test).

yes they are including coalescence test.

Comment 4

[alexander :surkov (:asurkov)]

try server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-866c4e68fd2d

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #1)

> Did you run this against bz's test case?

Marco could you please compare the nightly build and try server build?

Comment 6

[Marco Zehe (:MarcoZ)]

With a regular nightly, I get a value of roughly 116000 MS.
With the try server build: I get about 103000 MS, 13 seconds less time elapses on average.

Comment 7

[Ginn Chen]

surkov, the patch doesn't apply to the HEAD, can you submit a new one? Thanks.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> surkov, the patch doesn't apply to the HEAD, can you submit a new one? Thanks.

Ginn, it's going to be on top of patches from bug 515141, bug 541352 and bug 540892. So either you could apply these patches or make review for bug 515141 so that I can land all of them. Of course I can update this patch to the trunk but it's merging pain (I need to update this patch and all patches from bugs above). What way do you prefer?

Comment 9

[Ginn Chen]

I see. You can leave it alone.

Sorry, I was busy in last 2 days. I'll review your patches tomorrow.

Comment 10

[alexander :surkov (:asurkov)]

Thank you!

Comment 11

[Ginn Chen]

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

What's the bug number for changing mDOMNode to mNode?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> (From update of attachment 423036 [details] [diff] [review])
> What's the bug number for changing mDOMNode to mNode?

bug 540892

Comment 13

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/0c9bf70733ad