1820988 - Optimize canvas2d text drawing for purely-LTR strings

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jonathan Kew [:jfkthame]]

I think we can (marginally) improve Canvas text performance for common cases by avoiding some of the Unicode bidi processing for strings that are known to be purely LTR.

We already have a simplified code path used for single-character strings (introduced because pdf.js likes to paint each character individually, so this is a hot path), but I think we can extend that to also cover longer strings where no potentially-bidi content is present.

In local testing on macOS, this gives me about a 2.5% improvement on the old IETestDrive Preschool demo mentioned in bug 932613.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1820988 - Templatize the 8- and 16-bit versions of gfxFontGroup::MakeTextRun for convenience. r=#gfx-reviewers

No functional change, just a drive-by cleanup noticed when looking over the text-rendering APIs.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1820988 - The width parameter to BidiProcessor::DrawText is not used by any implementation, so remove it. r=#layout-reviewers

I guess this used to be needed somewhere, but it's long dead now.
Removing the parameter makes it clear that the caller doesn't need
to compute and pass it.

Depends on D171984

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1820988 - Remove unused bidiEngine parameter to CalculateBidiClass. r=#layout-reviewers

This function doesn't need a Bidi engine, it just does property lookups
via the intl API. So drop the redundant parameter.

Depends on D171985

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1820988 - Handle separator characters in canvas2d TextReplaceWhitespaceCharacters, to avoid a second preprocessing of the string in ProcessText. r=#gfx-reviewers

This avoids an extra string copy/scan in ProcessText, by including the block/segment
separators it cares about in the preprocessing that's already being done to normalize
whitespace characters by the caller.

Depends on D171986

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1820988 - Use 'using' to eliminate a bunch of explicit namespace-prefix usage in nsBidiPresUtils. r=#layout-reviewers

Just to reduce visual clutter in the code, for readability.

Depends on D171987

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1820988 - Rename ProcessOneChar to ProcessSimpleRun, and allow it to also handle purely-LTR strings. r=#layout-reviewers

This is the significant change here: extend the single-character fast-path that skips
bidi processing to also handle strings that are confirmed to have no bidi content.
In such cases, we don't need to pass the text to a Bidi engine for analysis at all.

Depends on D171988

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fdd0782eb8d
Templatize the 8- and 16-bit versions of gfxFontGroup::MakeTextRun for convenience. r=gfx-reviewers,aosmond
https://hg.mozilla.org/integration/autoland/rev/bcc2e31546e8
The width parameter to BidiProcessor::DrawText is not used by any implementation, so remove it. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b618fa47868b
Remove unused bidiEngine parameter to CalculateBidiClass. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a9888772a4e1
Handle separator characters in canvas2d TextReplaceWhitespaceCharacters, to avoid a second preprocessing of the string in ProcessText. r=gfx-reviewers,aosmond
https://hg.mozilla.org/integration/autoland/rev/1926e59c9e75
Use 'using' to eliminate a bunch of explicit namespace-prefix usage in nsBidiPresUtils. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5fd91fb4cb3b
Rename ProcessOneChar to ProcessSimpleRun, and allow it to also handle purely-LTR strings. r=emilio

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1820988 followup, use nsDependentSubstring in the assertion, because the text may not be null-terminated.

Comment 9

[Norisz Fay [:noriszfay]]

Backed out changeset 6fdd0782eb8d for causing build bustage

Backout link

Push with failures

Failure log

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f486b8078ac
followup, use nsDependentSubstring in the assertion, because the text may not be null-terminated.

Comment 11

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/bcc2e31546e8
https://hg.mozilla.org/mozilla-central/rev/b618fa47868b
https://hg.mozilla.org/mozilla-central/rev/a9888772a4e1
https://hg.mozilla.org/mozilla-central/rev/1926e59c9e75
https://hg.mozilla.org/mozilla-central/rev/5fd91fb4cb3b
https://hg.mozilla.org/mozilla-central/rev/7f486b8078ac

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/223bc3a0b9b5
Templatize the 8- and 16-bit versions of gfxFontGroup::MakeTextRun for convenience. r=gfx-reviewers,aosmond

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/223bc3a0b9b5