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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

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.

Comment 1

[James Teh [:Jamie]]

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.

Comment 2

[James Teh [:Jamie]]

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

Comment 3

[James Teh [:Jamie]]

Attachment: Bug 1431264 part 1: Accessible HandlerProvider: Implement a method to retrieve info for all relations in a single call.

IAccessible2::relations allows you to fetch IAccessibleRelation objects for all relations.
However, you must first call IAccessible2::nRelations to get the number of relations.
In addition, getting the type and number of targets for each relation requires additional calls.
This new method allows all of this to be retrieved in a single cross-process call.

Review commit: https://reviewboard.mozilla.org/r/217384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217384/

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1431264 part 2: AccessibleHandler: Local implementation of IAccessibleRelation using data provided in IARelationData.

The HandlerProvider::RelationsInfo method provides the type, number of targets and remote pointer for each relation in an IARelationData struct.
This local implementaition of IAccessibleRelation is constructed with an IARelationData struct and serves a single relation.
It uses the data from the struct to answer queries, except for actual targets.
For targets, it makes a remote call to IA2_2::relationTargetsOfType to answer the query.
We use relationTargetsOfType instead of IARelation::targets because marshaling so many IARelation pointers is a major bottleneck.

Review commit: https://reviewboard.mozilla.org/r/217386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217386/

Comment 5

[James Teh [:Jamie]]

Attachment: 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.

If a client calls IAccessible2::nRelations, it's likely that it will next call IAccessible2::relations to query each relation.
Furthermore, it's likely the client will call relationType and nTargets on each relation.
Therefore, fetch all of this info when nRelations is called.
The number of relations is immediately returned to the client.
The rest of the info is cached and returned to the client when the appropriate methods are called.
The info is only cached for one call; i.e. after the client calls relations once, the cache is dropped.
This makes memory management simpler and lowers the risk of cache invalidation problems.

Review commit: https://reviewboard.mozilla.org/r/217388/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217388/

Comment 6

[James Teh [:Jamie]]

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.

Comment 7

[James Teh [:Jamie]]

Comment on attachment 8947662 [details]
Bug 1431264 part 2: AccessibleHandler: Local implementation of IAccessibleRelation using data provided in IARelationData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217386/diff/1-2/

Comment 8

[James Teh [:Jamie]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217388/diff/1-2/

Comment 9

[James Teh [:Jamie]]

Marco, that patch update just fixed the wording in the commit message for part 2; there was no code change.

Comment 10

[James Teh [:Jamie]]

Comment on attachment 8947662 [details]
Bug 1431264 part 2: AccessibleHandler: Local implementation of IAccessibleRelation using data provided in IARelationData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217386/diff/2-3/

Comment 11

[James Teh [:Jamie]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217388/diff/2-3/

Comment 12

[James Teh [:Jamie]]

Ug. Sorry Marco. I realised one of the files had Windows line endings, so had to push another update. :(

Comment 13

[Marco Zehe (:MarcoZ)]

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

Comment 14

[Marco Zehe (:MarcoZ)]

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

Comment 15

[Marco Zehe (:MarcoZ)]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b11b10cebf25
https://hg.mozilla.org/mozilla-central/rev/85acda76be9c
https://hg.mozilla.org/mozilla-central/rev/7fe5e2760a88

Comment 18

[:Gijs (he/him)]

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?

Comment 19

[:Gijs (he/him)]

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.

Comment 20

[James Teh [:Jamie]]

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.

Comment 21

[James Teh [:Jamie]]

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 22

[James Teh [:Jamie]]

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.

Comment 23

[James Teh [:Jamie]]

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.

Comment 24

[James Teh [:Jamie]]

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.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2ec2a10573d
https://hg.mozilla.org/releases/mozilla-beta/rev/617e767b8af1
https://hg.mozilla.org/releases/mozilla-beta/rev/6f99bde986e0