1261425 - coalesce mutation events by a tree structure

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

keeping the events in an array and coalescing them by traversing it for each single insertion/removal, takes a half time we spend for the tree update. replacing the array to a tree-like structure, removes the bottle neck.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Review of attachment 8737292 [details] [diff] [review]:
-----------------------------------------------------------------

went over the patch, hopefully I didn't miss anything obvious.

::: accessible/base/AccEvent.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "AccEvent.h"
>  
> +#include "EventQueue.h"

Curious why this is necessary?

::: accessible/base/EventQueue.h
@@ +70,5 @@
>     */
>    nsTArray<RefPtr<AccEvent> > mEvents;
>  };
>  
> +

nit: whitespace

::: accessible/base/EventTree.cpp
@@ +89,5 @@
> +}
> +
> +TreeMutation::~TreeMutation()
> +{
> +  MOZ_ASSERT(mIsDone, "Done() must be called explicitly");

mIsDone seems to only be set to true with DEBUG, would this not assert when DEBUG is not set ?

@@ +295,5 @@
> +        break;
> +      }
> +    }
> +
> +    // So the given node is contained by a current node

nit: remove So

@@ +343,5 @@
> +      newNode->mFirst = Move(nodeOwnerRef);
> +      nodeOwnerRef = Move(newNode);
> +      nodeOwnerRef->mNext = Move(node->mNext);
> +
> +      //node->mContainer->Document()->mNotificationController->RootEventTree().Log();

If you are leaving it, might want to add a comment or something (though I do realize it's for logging).

@@ +368,5 @@
> +
> +          prevNode->mNext = Move(node->mNext);
> +          node = prevNode;
> +
> +          //prevNode->mContainer->Document()->mNotificationController->RootEventTree().Log();

same here

@@ +460,5 @@
> +void
> +EventTree::Mutated(AccMutationEvent* aEv)
> +{
> +  // If shown or hidden node is a root of previously mutated subtree, then
> +  // discard those subtree mutations as we aren't longer interested in them.

aren't -> are no

@@ +505,5 @@
> +          // sibling of this target.
> +          aEv->mAccessible->AppendTextTo(prevTextEvent->mModifiedText);
> +        }
> +        else if (aEv->mAccessible->IndexInParent() ==
> +                 prevEvent->mAccessible->IndexInParent() -1) {

nit: space between - and 1

::: accessible/base/NotificationController.h
@@ +119,5 @@
> +   * the given accessible, if the accessible is a part of name computation of
> +   * the container.
> +   */
> +  void QueueNameChange(Accessible* aChangeTarget)
> +  { 

nit: whitespace

::: accessible/tests/mochitest/events/test_coalescence.html
@@ +332,5 @@
> +      this.child2 = getNode(aChild2Id);
> +      this.parent = getNode(aParentId);
> +
> +      this.eventSeq = [
> +        new invokerChecker(EVENT_HIDE, getAccessible(aParentId)),

It would be great to also check for unexpected events here associated with children?

::: accessible/tests/mochitest/events/test_namechange.html
@@ +48,5 @@
> +        new invokerChecker(EVENT_SHOW, aID)
> +      ];
> +      this.invoke = function setAttr_recreate_invoke()
> +      {
> +        todo(false, "No accessible recreation has to be, just name change event");

No accessible recreation has to be -> No accessible recreation should happen

::: accessible/tests/mochitest/events/test_selection.xul
@@ +38,4 @@
>  
>        this.invoke = function advanceTab_invoke()
>        {
> +        todo(false, "No accessible recreation has to be, just selection event");

No accessible recreation has to be -> No accessible recreation should happen

@@ +155,5 @@
>       * Do tests.
>       */
>      var gQueue = null;
>  
> +    enableLogging("events");

You're planning on keeping this uncommented?

Comment 2

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

(In reply to alexander :surkov from comment #0)

> keeping the events in an array and coalescing them by traversing it for each
> single insertion/removal, takes a half time we spend for the tree update.
> replacing the array to a tree-like structure, removes the bottle neck.

Certainly possible. Did you profile? (I'm curious if Talos will show a change)

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #1)
> > +#include "EventQueue.h"
> 
> Curious why this is necessary?

an artifact

> > +  MOZ_ASSERT(mIsDone, "Done() must be called explicitly");
> 
> mIsDone seems to only be set to true with DEBUG, would this not assert when
> DEBUG is not set ?

https://dxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#395

> @@ +343,5 @@
> > +      //node->mContainer->Document()->mNotificationController->RootEventTree().Log();
> 
> If you are leaving it, might want to add a comment or something (though I do
> realize it's for logging).

I'll remote them, the mochitest provides a coverage for this code

> ::: accessible/tests/mochitest/events/test_coalescence.html
> @@ +332,5 @@
> > +      this.child2 = getNode(aChild2Id);
> > +      this.parent = getNode(aParentId);
> > +
> > +      this.eventSeq = [
> > +        new invokerChecker(EVENT_HIDE, getAccessible(aParentId)),
> 
> It would be great to also check for unexpected events here associated with
> children?

child hide events are still fired, this patch doesn't fix this issue.

> @@ +155,5 @@
> > +    enableLogging("events");
> 
> You're planning on keeping this uncommented?

will comment out

Comment 4

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #2)
> (In reply to alexander :surkov from comment #0)
> 
> > keeping the events in an array and coalescing them by traversing it for each
> > single insertion/removal, takes a half time we spend for the tree update.
> > replacing the array to a tree-like structure, removes the bottle neck.
> 
> Certainly possible. Did you profile?

those numbers are taken from local profiling on local tests

> (I'm curious if Talos will show a
> change)

We could see an improvement, however I didn't run it. But we really should extend our Talos test suite to provide good coverage for different scenarios of a web page mutations.

Comment 5

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0d5a27da0f8c83192a3a7965874ac111dfb458
Bug 1261425 - coalesce mutation events by a tree structure, r=yzen

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/2b0d5a27da0f