1844464 - Optimize gfxHarfBuzzShaper::ShapeText if possible, for Editor-TipTap

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, task)

Description

[Markus Stange [:mstange]]

On Windows, 20% of the time on the Speedometer 3 subtest "Editor-TipTap" is spent inside the function gfxHarfBuzzShaper::ShapeText. Reducing the time in this function would have an appreciable impact on the score of that subtest.

Profile: https://share.firefox.dev/3pZqFD9 (double-click a function to view line-by-line timings)

https://shell--speedometer-preview.netlify.app/?suite=Editor-TipTap&iterationCount=100

I should note that we are already faster than Chrome at layout on this subtest, but this may not remain so indefinitely, and a percent is a percent.

Comment 1

[Jonathan Kew [:jfkthame]]

A couple of things catch my eye in this profile that may be worth exploring:

First, gfxDWriteFont::GetGlyphWidth currently uses an nsTHashMap to cache glyph widths for the font; we could switch this to mozilla::HashTable, which should be faster, and/or we could experiment with layering a fast MRUCache in front of the hashtable.

We could also try an MRUCache to accelerate gfxHarfBuzzShaper::GetNominalGlyph.

The other thing that stands out is gfxFontShaper::MergeFontFeatures, which again is heavy on hashtable work. We could use mozilla::HashTable for a faster implementation, but I think a better way will be to use a simple sorted array to accumulate the features. No real-world content is likely to have so many separate features specified that we really need the hashtable.

I'll experiment with some patches along these lines.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1844464 - patch 1 - Optimize gfxFontShaper::MergeFontFeatures by using a sorted array rather than nsTHashMap to accumulate the features. r=#gfx-reviewers

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1844464 - patch 2 - Add an MruCache to accelerate gfxHarfBuzzShaper::GetNominalGlyph. r=#gfx-reviewers

Depends on D184101

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1844464 - patch 3 - Add an MruCache in front of the shaper's call to gfxFont::GetGlyphWidth(). r=#gfx-reviewers

Depends on D184102

Comment 5

[Jonathan Kew [:jfkthame]]

With the patches above, the Editor-TipTap score in my local (non-PGO) build rises from 6.806 to 7.252, which seems a healthy win.

Comment 6

[Markus Stange [:mstange]]

Very nice!

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1844464 - patch 0 - Require the gfxFont lock to be held while calling the shaper. r=#gfx-reviewers

We need to do this in order to add internal caches to the shaper
(without a lot of thread-safety pain).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52e713717c12
patch 0 - Require the gfxFont lock to be held while calling the shaper. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/fc05f1740cda
patch 1 - Optimize gfxFontShaper::MergeFontFeatures by using a sorted array rather than nsTHashMap to accumulate the features. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/1fa1a2e110e5
patch 2 - Add an MruCache to accelerate gfxHarfBuzzShaper::GetNominalGlyph. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/6921e3bf21b2
patch 3 - Add an MruCache in front of the shaper's call to gfxFont::GetGlyphWidth(). r=gfx-reviewers,lsalzman

Comment 9

[Noemi Erli[:noemi_erli]]

Backed out 4 changesets (Bug 1844464) for causing failures on test_offscreencanvas_font.html CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=bu3Kg8uPQ3K-c2djAS3CoQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=android%2C7.0%2Cx86-64%2Cwebrender%2Cdebug%2Cmochitests%2Cwith%2Csoftware%2Cwebrender%2Cwithout%2Cfission%2Cenabled%2Ctest-android-em-7.0-x86_64-qr%2Fdebug-geckoview-mochitest-plain-swr-nofis%2C2&revision=6921e3bf21b299e646c863dcd98838e15fc8428b

Logs: https://treeherder.mozilla.org/logviewer?job_id=423453067&repo=autoland&lineNumber=2211
https://treeherder.mozilla.org/logviewer?job_id=423465103&repo=autoland&lineNumber=8219
Reftest: https://treeherder.mozilla.org/logviewer?job_id=423463006&repo=autoland&lineNumber=4125

Backout: https://hg.mozilla.org/integration/autoland/rev/e0a7f85b89aaa94526bb26ec54c1000e71b91981

Comment 10

[Jonathan Kew [:jfkthame]]

Revised patches to avoid the need to hold the gfxFont lock, which was leading to possible-deadlock warnings.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12d609f24c16
patch 1 - Optimize gfxFontShaper::MergeFontFeatures by using a sorted array rather than nsTHashMap to accumulate the features. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/a0c8264fc96a
patch 2 - Add an MruCache to accelerate gfxHarfBuzzShaper::GetNominalGlyph. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/4a7e5d260b93
patch 3 - Add an MruCache in front of the shaper's call to gfxFont::GetGlyphWidth(). r=gfx-reviewers,lsalzman

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/12d609f24c16
https://hg.mozilla.org/mozilla-central/rev/a0c8264fc96a
https://hg.mozilla.org/mozilla-central/rev/4a7e5d260b93