Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gecko_ia2 vbuf backend: Support IAccessibleHypertext2 to improve performance for Firefox multi-process. #7719

Merged
merged 4 commits into from Dec 8, 2017

Conversation

jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Nov 2, 2017

Link to issue number:

None.

Summary of the issue:

With Firefox multi-process, rendering a virtual buffer for browse mode now makes cross-process calls, since the content document accessibility tree is now in another process. This has a substantial impact on performance. The more cross-process calls we can reduce, the better. This PR introduces a change which fetches all embedded children at once, rather than fetching each in a separate call.

Description of how this pull request fixes the issue:

IAccessibleHypertext2::hyperlinks allows all embedded objects to be retrieved at once, rather than retrieving them one at a time.

This improves performance for cross-process renders, which is the case for Firefox multi-process.
IAccessibleHypertext is still supported and will be used if the newer interface is not present.

In addition:

  1. Don't bother calling hyperlinkIndex, since in Gecko (and Chrome), hyperlinks correspond with embedded object characters.
  2. Use a constant for the embedded object character.

Testing performed:

confirmed that the World War I Wikipedia article renders as expected in:

  1. A build of Firefox which supports IAHypertext2. (This is now in Firefox nightly.)
  2. A build that does not.
  3. Chrome Canary, which does not support IAHypertext2.

Using IAHypertext2 seems to shave up to a second off the render time for the World War I article, though it's difficult to provide concrete numbers because the time seems to be a little variable.

Known issues with pull request:

None know.

Change log entry:

In Bug Fixes:

- Slight performance improvement when rendering large amounts of content in Mozilla Firefox 58 and later. (#7719)

…ormance for Firefox multi-process.

IAccessibleHypertext2::hyperlinks allows all embedded objects to be retrieved at once, rather than retrieving them one at a time.
This improves performance for cross-process renders, which is the case for Firefox multi-process.
IAccessibleHypertext is still supported and will be used if the newer interface is not present.

In addition:

1. Don't bother calling hyperlinkIndex, since in Gecko (and Chrome), hyperlinks correspond with embedded object characters.
2. Use a constant for the embedded object character.
@jcsteh jcsteh requested a review from michaelDCurran Nov 2, 2017
Copy link
Member

@michaelDCurran michaelDCurran left a comment

Could you please move the hyperlink getter classes into common/ia2utils.cpp/h, as we'll want to use them in ia2liveRegions.cpp as well eventually. Other than that this all looks fine to me.

@jcsteh jcsteh requested a review from michaelDCurran Nov 7, 2017
};

/**
* Create an appropriate HyperlinkGetterto retrieve hyperlinks
Copy link
Collaborator

@derekriemer derekriemer Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HyperlinkGetterto > HyperlinkGetter to

jcsteh added 2 commits Nov 11, 2017
The gecko vbuf (and probably any future callers) only call HyperlinkGetter::next() when they encounter an embedded object character.
That is, if there are no embedded object characters, there will be no calls.
Thus:

1. For IAccessibleHypertext, we don't need to call nHyperlinks. hyperlink will fail or return null if we pass an invalid index anyway.
2. For IAccessibleHypertext2, we can lazily fetch the hyperlinks on the first call to get(), thus saving a potential cross-process call when there are none.
@MarcoZehe
Copy link
Contributor

@MarcoZehe MarcoZehe commented Nov 17, 2017

I ran with both try builds for a day or two now, have been using the second one since it became available, and haven't found anything bad, no missing text or links or such.

@PratikP1
Copy link

@PratikP1 PratikP1 commented Nov 17, 2017

michaelDCurran added a commit that referenced this issue Nov 23, 2017
@michaelDCurran michaelDCurran merged commit 8ddc7e7 into nvaccess:master Dec 8, 2017
@nvaccessAuto nvaccessAuto added this to the 2018.1 milestone Dec 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants