[e10s a11y] Reduce cross-process calls required to fetch relations

RESOLVED FIXED in Firefox 59

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jamie, Assigned: Jamie)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed, firefox60 verified)

Details

Attachments

(3 attachments)

JAWS needs quite a few relations when building its virtual buffer. Retrieving relations either means calling IAccessible2::nRelations + IAccessible2::relations(two calls) or calling IAccessible2_2::relationTargetsOfType (one call for each type required). In addition, because IAccessibleRelation is an interface, additional calls are required to determine the relation type and/or get its targets (up to 3 calls per relation object). We can reduce the number of cross-proc calls using the handler.

We don't want to fetch all targets for all relations in one shot, as the client probably won't need all of these. My thinking at present is that IA2::nRelations/IA2::relations would fetch information for all relation objects in a single cross-proc call, including the type and number of targets for each relation. Fetching the targets would remain a separate call. For JAWS, that would result in one cross-proc call to fetch all relation info, plus one additional call to fetch the targets for each type of interest.
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b2688439daf9de043c214957245dae1eac45152&selectedJob=157666425
Direct link to installer: https://queue.taskcluster.net/v1/task/OYUw2DH7RsiXBigBKmmGzg/runs/0/artifacts/public/build/install/sea/target.installer.exe

I'm not seeing much (if any) difference locally. <sigh> I don't really understand why. However, I suspect I'm also running outdated JAWS code, so working with VFO to see whether it makes a difference with their latest code.
Even with VFO's latest code, things are still horribly slow with the code in comment 1. Relations still appear to be the bottleneck.

Here's what I know:
1. My original code included the IAccessibleRelation pointer for each relation, as well as the type and number of targets, in the bulk fetched data. Even though this was all done in a single cross-proc call, it seems that marshaling/unmarshaling the IAccessibleRelation pointers is quite slow. Passing the type and nTargets for each relation but *not* the IARelation pointer was significantly faster.
2. Despite several days of investigation, debugging and banging my head against a brick wall, I have not been able to fully determine why this is so slow, nor can I speed it up much.
a) COM garbage collection/pinging, which we previously thought was disabled, is having *some* impact. See bug 1434822. However, while disabling this might speed things up a tiny bit in this case, it isn't significant enough.
b) Frequently, when I debug break in a large buffer render, I see COM looking up ids in a hash table while unmarshaling an IARelation pointer (COIDHashTable::Lookup). I guess this is the translation of remote OIDs to local objects. I'm wondering whether the sheer scale of objects we're dealing with means that this lookup becomes a bottleneck.
c) As far as I can tell, we're not doing an unnecessary cross-proc QI. I logged IARelation QIs (in content) and unmarshals (in the parent). I always saw the QIs before the unmarshals, which suggests an unmarshal didn't trigger a QI.
d) That said, I was seeing null riids in calls to combase!_CoUnmarshalInterface for IARelations (but calls for some other interfaces did have a valid riid). I still don't really understand this, but perhaps COM just figures out the riid later when it decodes the OBJREF. I'm not sure why it's different for some other interfaces. c) makes me reasonably certain this isn't anything to be concerned about, though.

Given this, I started working on another approach:
1. Since marshaling/unmarshaling IARelation is the bottleneck, we should just avoid that altogether.
2. We can just bulk fetch the type and number of targets for each relation and not the interface pointer.
3. When the client requests targets for a given relation, we can call IA2_2::relationTargetsOfType. This is technically a bit inefficient because content has to go look up the relation twice, once for IA2::relations and once for IA2_2::relationTargetsOfType. However, this will only be necessary for relations the client actually wants targets for.
4. I've only tested this briefly so far, but this appears to be significantly faster than the former approach, even despite the double relation lookup in content.

Here's a try build with this new code:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c4686ff1e1161733c357385c06bd20594ddb1a
VFO report a very significant performance boost with this code, so I'm submitting it for review. Some further testing by us wouldn't go astray, however.
Marco, that patch update just fixed the wording in the commit message for part 2; there was no code change.
Ug. Sorry Marco. I realised one of the files had Windows line endings, so had to push another update. :(
Comment on attachment 8947661 [details]
Bug 1431264 part 1: Accessible HandlerProvider: Implement a method to retrieve info for all relations in a single call.

https://reviewboard.mozilla.org/r/217384/#review223190
Attachment #8947661 - Flags: review?(mzehe) → review+
Comment on attachment 8947662 [details]
Bug 1431264 part 2: AccessibleHandler: Local implementation of IAccessibleRelation using data provided in IARelationData.

https://reviewboard.mozilla.org/r/217386/#review223192
Attachment #8947662 - Flags: review?(mzehe) → review+
Comment on attachment 8947663 [details]
Bug 1431264 part 3: AccessibleHandler: If a client wants to query all relations, fetch as much info as possible in a single cross-process call.

https://reviewboard.mozilla.org/r/217388/#review223194
Attachment #8947663 - Flags: review?(mzehe) → review+
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b11b10cebf25
part 1: Accessible HandlerProvider: Implement a method to retrieve info for all relations in a single call. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/85acda76be9c
part 2: AccessibleHandler: Local implementation of IAccessibleRelation using data provided in IARelationData. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/7fe5e2760a88
part 3: AccessibleHandler: If a client wants to query all relations, fetch as much info as possible in a single cross-process call. r=MarcoZ
After this landing on central I can no longer build:

builds\opt\dist\include\mozilla/a11y/HandlerProvider.h(63): error C2061: syntax error: identifier 'IARelationData'
builds\opt\dist\include\mozilla/a11y/HandlerProvider.h(93): error C2061: syntax error: identifier 'IARelationData'

any idea what's going wrong?
Flags: needinfo?(jteh)
Looks like HandlerProvider.h should include <oleacc.h> , and this causes build failure when unified compilation doesn't include it for whatever reason . I'll file a followup bug.
Depends on: 1435696
Because of bug 1420119, I should have committed a clobber along with this change. Sorry about that. I'm currently investigating a patch from Aaron which should avoid the need to clobber in this case, so if all goes well, this won't occur again in future.
Flags: needinfo?(jteh)
Marco, a VFO engineer and I have all verified that this significantly improves performance for JAWS with nightly builds including the fix. VFO have been testing with this for a week or so now without any issues.
Comment on attachment 8947661 [details]
Bug 1431264 part 1: Accessible HandlerProvider: Implement a method to retrieve info for all relations in a single call.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Loading documents with the JAWS screen reader is unusably slow.
[Is this code covered by automated tests?]: No, because we don't have a mechanism for testing platform accessibility.
[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?]: This patch only affects Windows accessibility. Furthermore, it is limited to functions which are currently only called by JAWS.
[String changes made/needed]: None.
Attachment #8947661 - Flags: approval-mozilla-beta?
Comment on attachment 8947662 [details]
Bug 1431264 part 2: AccessibleHandler: Local implementation of IAccessibleRelation using data provided in IARelationData.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Loading documents with the JAWS screen reader is unusably slow.
[Is this code covered by automated tests?]: No, because we don't have a mechanism for testing platform accessibility.
[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?]: This patch only affects Windows accessibility. Furthermore, it is limited to functions which are currently only called by JAWS.
[String changes made/needed]: None.
Attachment #8947662 - Flags: approval-mozilla-beta?
Comment on attachment 8947663 [details]
Bug 1431264 part 3: AccessibleHandler: If a client wants to query all relations, fetch as much info as possible in a single cross-process call.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Loading documents with the JAWS screen reader is unusably slow.
[Is this code covered by automated tests?]: No, because we don't have a mechanism for testing platform accessibility.
[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?]: This patch only affects Windows accessibility. Furthermore, it is limited to functions which are currently only called by JAWS.
[String changes made/needed]: None.
Attachment #8947663 - Flags: approval-mozilla-beta?
Comment on attachment 8947661 [details]
Bug 1431264 part 1: Accessible HandlerProvider: Implement a method to retrieve info for all relations in a single call.

e10s+a11y+jaws related fixed, Beta59+
Attachment #8947661 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8947662 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8947663 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.