gecko_ia2 vbuf backend: Support IAccessibleHypertext2 to improve performance for Firefox multi-process.#7719
Merged
Merged
Conversation
…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.
michaelDCurran
requested changes
Nov 3, 2017
michaelDCurran
left a comment
Member
There was a problem hiding this 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.
derekriemer
reviewed
Nov 8, 2017
| }; | ||
|
|
||
| /** | ||
| * Create an appropriate HyperlinkGetterto retrieve hyperlinks |
Collaborator
There was a problem hiding this comment.
HyperlinkGetterto > HyperlinkGetter to
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.
Contributor
|
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. |
|
Confirmed with FF Nightly and the try builds.
|
michaelDCurran
approved these changes
Nov 22, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
Testing performed:
confirmed that the World War I Wikipedia article renders as expected in:
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: