Closed Bug 1431256 Opened 2 years ago Closed 2 years ago

[e10s a11y] Improve performance of fetching children

Categories

(Core :: Disability Access APIs, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- verified
firefox61 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(5 files)

IEnumVARIANT is used by AccessibleChildren (and sometimes directly by clients) to fetch multiple children. Aside from the call to IEnumVARIANT::Next, calls to IEnumVARIANT::Reset and/or IEnumVARIANT::Clone are also necessary. And as discussed in bug 1419992, UI Automation is even worse because it calls IEnumVARIANT::Next to fetch children one at a time. This means there can be two or more cross-process calls to fetch children instead of just one. When tree walking, this can be a significant number of wasted calls. We can eliminate many of these cross-proc calls using the handler.
Try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ef06881c075c341b2b55fb4320b35233fbed87d

Unfortunately, from what I can see, this doesn't result in a notable performance improvement for either UI Automation, JAWS or NVDA. (NVDA uses IEnumVARIANT to walk tables.) This really confuses me. The impact would certainly be smaller for NVDA and JAWS, since they already fetch all children in one go; this just eliminates the call to reset the enumerator to 0. However, for UIA, this reduces one call per child down to one call for all children. I do perhaps see a tiny difference with UIA, but it's just not consistent enough for me to be sure.

This table test case should trigger use of IEnumVARIANT in NVDA, JAWS and UI Automation. You can adjust the count variable to change the number of rows. Since each row has cells as children, this requires a request for children on each row.

<div id="content"></div>
<script>
var count = 1000;
var content = document.getElementById("content");
var out = "<table>";
for (var r = 1; r <= count; ++r) {
	out += "<tr><td>r" + r + "c1</td><td>r" + r + "c2</td></tr>";
}
content.innerHTML = out;
</script>

For UIA, you can use the following code in the NVDA Python console to time a tree walk (see explanations in bug 1419992):

import time, UIAHandler; t = time.time(); root = UIAHandler.handler.clientObject.ElementFromHandle(nav.windowHandle); cond = UIAHandler.handler.clientObject.CreatePropertyCondition(UIAHandler.UIA_NamePropertyId, "Conversations"); root.FindFirst(UIAHandler.TreeScope_Descendants, cond); time.time() - t

Note that the number of children might need to be higher to notice a difference, since 1000 wasted calls probably isn't much in the grand scheme of things.
From my work on relations (bug 1431264), we learnt that reducing calls is not the only consideration in terms of improving cross-process COM performance. Marshaling objects is significantly more expensive than marshaling other kinds of data. So, pointless calls to IEnumVARIANT::Reset are insignificant when compared to the performance hit incurred when marshaling lots of objects, even if those objects are marshaled in a single call. Therefore, it's not useful to land this change to IEnumVARIANT alone.

However, this led me to another idea. When considering a large document, a huge number of the children we return are text leaf nodes. Marshaling full objects is expensive, but for text leaf nodes, the client is only interested in the text and a few other pieces of information; it doesn't need the full object. Therefore, rather than returning the full object for text leaf accessibles, we can just return the text and other necessary information and "fake" the accessible in the handler. This results in a significant performance improvement for JAWS; World War I renders in about a third of the time compared with current Nightly.
Summary: [e10s a11y] Reduce cross-process calls required to fetch children → [e10s a11y] Improve performance of fetching children
Comment on attachment 8961179 [details]
Bug 1431256 part 1: Accessible HandlerProvider: Implement a method to optimally retrieve all children in a single call.

https://reviewboard.mozilla.org/r/229946/#review235668
Attachment #8961179 - Flags: review?(mzehe) → review+
Comment on attachment 8961180 [details]
Bug 1431256 part 2: AccessibleHandler: Implementation of IAccessible2 for text leaf accessibles using data provided in AccChildData.

https://reviewboard.mozilla.org/r/229948/#review235670
Attachment #8961180 - Flags: review?(mzehe) → review+
Comment on attachment 8961181 [details]
Bug 1431256 part 3: AccessibleHandler: When a client requests children, fetch them optimally using a single cross-process call.

https://reviewboard.mozilla.org/r/229950/#review235672
Attachment #8961181 - Flags: review?(mzehe) → review+
Comment on attachment 8961182 [details]
Bug 1431256 part 4: Remove IEnumVARIANT from the AccessibleHandler payload.

https://reviewboard.mozilla.org/r/229952/#review235674
Attachment #8961182 - Flags: review?(mzehe) → review+
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc4b465ae5c1
part 1: Accessible HandlerProvider: Implement a method to optimally retrieve all children in a single call. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/580a1123de81
part 2: AccessibleHandler: Implementation of IAccessible2 for text leaf accessibles using data provided in AccChildData. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/7e3ec61fa952
part 3: AccessibleHandler: When a client requests children, fetch them optimally using a single cross-process call. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/8f68422b3307
part 4: Remove IEnumVARIANT from the AccessibleHandler payload. r=MarcoZ
Comment on attachment 8961566 [details]
Bug 1431256 correction: AccessibleHandler: Really return E_NOINTERFACE for IEnumVARIANT if there are no children.

https://reviewboard.mozilla.org/r/230424/#review235920
Attachment #8961566 - Flags: review?(mzehe) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d50507c182ab7760c17c5e7bb9956f46a3dc188c -d 18a7ed496352: rebasing 453624:d50507c182ab "Bug 1431256 part 1: Accessible HandlerProvider: Implement a method to optimally retrieve all children in a single call. r=MarcoZ"
merging accessible/ipc/win/HandlerProvider.cpp
merging accessible/ipc/win/handler/HandlerData.idl
note: rebase of 453624:d50507c182ab created no changes to commit
rebasing 453625:0cdc01158f7f "Bug 1431256 part 2: AccessibleHandler: Implementation of IAccessible2 for text leaf accessibles using data provided in AccChildData. r=MarcoZ"
merging accessible/ipc/win/handler/moz.build
note: rebase of 453625:0cdc01158f7f created no changes to commit
rebasing 453626:a6c0a56c2bb6 "Bug 1431256 part 3: AccessibleHandler: When a client requests children, fetch them optimally using a single cross-process call. r=MarcoZ"
merging accessible/ipc/win/handler/AccessibleHandler.cpp
warning: conflicts while merging accessible/ipc/win/handler/AccessibleHandler.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa06fb8316c2
correction: AccessibleHandler: Really return E_NOINTERFACE for IEnumVARIANT if there are no children. r=MarcoZ
Verified fixed in Firefox 61.0a1 (20180323122900).
Comment on attachment 8961179 [details]
Bug 1431256 part 1: Accessible HandlerProvider: Implement a method to optimally retrieve all children in a single call.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Users of the JAWS screen reader will experience slow page load times (~2.5x slower).
[Is this code covered by automated tests?]: No; no mechanism to test platform a11y.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Other patches from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: While these are certainly not small patches, they only affect accessibility. Both Marco and I from Mozilla accessibility have tested this quite extensively in both normal usage and with specific test cases with both the JAWS and NVDA screen readers. Freedom Scientific (the JAWS vendor) have also tested this and reported no issues.
Attachment #8961179 - Flags: approval-mozilla-beta?
Comment on attachment 8961180 [details]
Bug 1431256 part 2: AccessibleHandler: Implementation of IAccessible2 for text leaf accessibles using data provided in AccChildData.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Users of the JAWS screen reader will experience slow page load times (~2.5x slower).
[Is this code covered by automated tests?]: No; no mechanism to test platform a11y.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Other patches from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: While these are certainly not small patches, they only affect accessibility. Both Marco and I from Mozilla accessibility have tested this quite extensively in both normal usage and with specific test cases with both the JAWS and NVDA screen readers. Freedom Scientific (the JAWS vendor) have also tested this and reported no issues.
Attachment #8961180 - Flags: approval-mozilla-beta?
Comment on attachment 8961181 [details]
Bug 1431256 part 3: AccessibleHandler: When a client requests children, fetch them optimally using a single cross-process call.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Users of the JAWS screen reader will experience slow page load times (~2.5x slower).
[Is this code covered by automated tests?]: No; no mechanism to test platform a11y.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Other patches from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: While these are certainly not small patches, they only affect accessibility. Both Marco and I from Mozilla accessibility have tested this quite extensively in both normal usage and with specific test cases with both the JAWS and NVDA screen readers. Freedom Scientific (the JAWS vendor) have also tested this and reported no issues.
Attachment #8961181 - Flags: approval-mozilla-beta?
Comment on attachment 8961182 [details]
Bug 1431256 part 4: Remove IEnumVARIANT from the AccessibleHandler payload.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Users of the JAWS screen reader will experience slow page load times (~2.5x slower).
[Is this code covered by automated tests?]: No; no mechanism to test platform a11y.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Other patches from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: While these are certainly not small patches, they only affect accessibility. Both Marco and I from Mozilla accessibility have tested this quite extensively in both normal usage and with specific test cases with both the JAWS and NVDA screen readers. Freedom Scientific (the JAWS vendor) have also tested this and reported no issues.
Attachment #8961182 - Flags: approval-mozilla-beta?
Comment on attachment 8961566 [details]
Bug 1431256 correction: AccessibleHandler: Really return E_NOINTERFACE for IEnumVARIANT if there are no children.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Users of the JAWS screen reader will experience slow page load times (~2.5x slower).
[Is this code covered by automated tests?]: No; no mechanism to test platform a11y.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Other patches from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: While these are certainly not small patches, they only affect accessibility. Both Marco and I from Mozilla accessibility have tested this quite extensively in both normal usage and with specific test cases with both the JAWS and NVDA screen readers. Freedom Scientific (the JAWS vendor) have also tested this and reported no issues.
Attachment #8961566 - Flags: approval-mozilla-beta?
I'm not sold on uplifting such a large set of changes, this isn't a new regression in 60 AIUI, so I feel like this can ride the trains.
(In reply to Julien Cristau [:jcristau] from comment #24)
> I'm not sold on uplifting such a large set of changes, this isn't a new
> regression in 60 AIUI, so I feel like this can ride the trains.

I totally understand the reluctance. Here's some further context/justification:
1. Firefox 57 massively regressed users of the JAWS screen reader, to the point where it was completely unsuitable for real world usage and we had to recommend that all JAWS users switch to 52 ESR. We definitely lost screen reader users as a result.
2. We've been working hard with the JAWS vendor to get Firefox + JAWS back to an acceptable state for users and have been targeting Firefox 60.
3. The longer users have to wait for these fixes, the greater the chance we lose users who will not return.
4. Because Firefox 60 is an ESR, enterprise users using the JAWS screen reader will be stuck with a very suboptimal browsing experience for quite some time.
5. Although these patches are large, a substantial amount of the code is (necessary) boilerplate.

I hope these concerns might impact the decision, but ultimately, I understand and respect the need to be cautious concerning approvals.
(In reply to Julien Cristau [:jcristau] from comment #24)
> I'm not sold on uplifting such a large set of changes, this isn't a new
> regression in 60 AIUI, so I feel like this can ride the trains.

In addition to what Jamie said above, it must be mentioned that JAWS is the primary screen reader consuming our APIs. It is the market leader world-wide, and its usage exceeds that of NVDA in regaular, pre-quantum statistics by an amount. So regaining these users as soon as possible, and getting them on an updated train that does not get its last security update in July of this year, will play a key role in regaining, and retaining, those users. This plays directly into one of the top level key initiatives of user retention.

These patches are well-tested and also have sign-off from the JAWS vendor.
Comment on attachment 8961179 [details]
Bug 1431256 part 1: Accessible HandlerProvider: Implement a method to optimally retrieve all children in a single call.

OK, let's give this a try.  I don't suppose we get lots of pre-release testing for this kind of work anyway, so if we + JAWS are happy with it I guess it makes sense.  Thanks for the explanations.  This should be in 60.0b8.
Attachment #8961179 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8961180 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8961181 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8961182 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8961566 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Thanks for your consideration and understanding. Rest assured we want the best experience for our accessibility users, so if we'll be watching this very closely to make sure there aren't any problems.
Verified that this landed properly in Firefox 60.0b8.
Flags: qe-verify-
Depends on: 1467257
You need to log in before you can comment on or make changes to this bug.