can we cache the results of gfxFontShaper::GetRoundOffsetsToPixels

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: Text
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: dbaron, Assigned: jfkthame)

Tracking

({perf})

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
In the profile in bug 1329601 comment 13 I noticed this stack to call free:

free
gfxFontShaper::GetRoundOffsetsToPixels(mozilla::gfx::DrawTarget*, bool*, bool*)
gfxHarfBuzzShaper::SetGlyphsFromRun(mozilla::gfx::DrawTarget*, gfxShapedText*, unsigned int, unsigned int, char16_t const*, hb_buffer_t*, bool)
gfxHarfBuzzShaper::ShapeText(mozilla::gfx::DrawTarget*, char16_t const*, unsigned int, unsigned int, mozilla::unicode::Script, bool, gfxShapedText*)


Can we cache the result of GetRoundOffsetsToPixels so we don't have to call this as often?
Flags: needinfo?(jfkthame)
(Reporter)

Updated

5 months ago
Keywords: perf
(Assignee)

Comment 1

5 months ago
Hmm.... we can't readily cache this within the shaper, or within the gfxFont instance, because the answers to GetRoundOffsetsToPixels() depend on the drawTarget that we're aiming for, and a single font instance may be used with multiple drawTargets.

We can't work around that by just hanging the cached values off of the drawTarget (e.g. in its userData), because changes to the DT's transform could invalidate them. So I think we'd have to push the implementation of GetRoundOffsetsToPixels() itself down into the drawTarget, and make DT::SetTransform (and possibly other methods?) aware of the caching so that they can invalidate the cached values. This is all starting to sound too invasive...

An alternative, though: I guess in many cases we end up calling ShapeText() numerous times during construction of a single textrun, and the values will be constant across those calls. So we could hoist the call to GetRoundOffsetsToPixels() up to, say, the beginning of gfxFontGroup::InitTextRun(), and then pass the resulting booleans through to all the ShapeText() calls. The downside of that, though, is that it means we'd call GetRoundOffsetsToPixels() (once) for every InitTextRun(), whereas currently we don't call it at all if it turns out that the textrun can be initialized entirely from the shaped-word caches and so it doesn't call ShapeText() at all.

That prompts me to suspect, though, that we actually have a correctness bug here: AFAICS, it's possible that we'll cache shapedWord records that were initialized according to one set of GetRoundOffsetsToPixels() results, but then end up using them in a context where GetRoundOffsetsToPixels() would have returned a different answer. To avoid this, we should be incorporating the rounding flags into the shaped-word cache keys.

I'll see if I can put together a testcase that confirms whether we do have such a bug; and if so, the "alternative" above will also serve as a basis for fixing it.
Flags: needinfo?(jfkthame)
(Assignee)

Updated

5 months ago
Depends on: 1353000
(Reporter)

Updated

5 months ago
Whiteboard: [qf:?]
Whiteboard: [qf:?] → [qf]
(Assignee)

Comment 2

4 months ago
Created attachment 8855013 [details] [diff] [review]
Hoist call to GetRoundOffsetsToPixels out of the inner loop of text shaping

To avoid calling GetRoundOffsetsToPixels repeatedly in the inner loop of text shaping (e.g. for each word of a long text block), this patch hoists it up so we call it at the beginning of gfxFont::SplitAndInitTextRun, and then pass the resulting values down to where we shape the individual words.
Attachment #8855013 - Flags: review?(jmuizelaar)
(Assignee)

Updated

4 months ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Updated

4 months ago
Blocks: 1353000
No longer depends on: 1353000
Comment on attachment 8855013 [details] [diff] [review]
Hoist call to GetRoundOffsetsToPixels out of the inner loop of text shaping

Review of attachment 8855013 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like a good idea to me.
Attachment #8855013 - Flags: review?(jmuizelaar) → review+
Is this ready to land?
Flags: needinfo?(jfkthame)
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 5

4 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf453f66c9c384768d7a8d92f129c1328886096
Bug 1352528 - Hoist call to GetRoundOffsetsToPixels out of the inner loop of text shaping. r=jrmuizel
(Assignee)

Updated

4 months ago
Flags: needinfo?(jfkthame)

Comment 6

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fdf453f66c9c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.