Closed Bug 1408611 Opened 7 years ago Closed 7 years ago

Avoid initializing glyph buffer needlessly

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jrmuizel, Assigned: jfkthame)

References

Details

Attachments

(3 files)

GlyphBufferAzure contains a 2048 byte array of 'Glyph' each of which has a 'Point' in it which we end up initializing to 0. This takes about 8.1% of the time spent in gfxFont::DrawGlyphs()
Blocks: 1408500
Here's a possible approach to solving this. An alternative would be to use AutoTArray. What would you prefer Jonathan?
Assignee: nobody → jmuizelaar
Flags: needinfo?(jfkthame)
Attached file alternative approach (deleted) —
Hmmm, the union option.... I guess it should work, but it makes for some really ugly code, IMO. I'd prefer an AutoTArray approach, I think; that seems a lot cleaner. But do we know how much overhead that would have (in an opt build)? If it means AppendGlyph() would call through to AppendElement, I guess there must be some amount of overhead there (because AppendElement will need to check whether it's going to overflow the auto-buffer and so needs to reallocate). OTOH, if we allow the buffer to grow arbitrarily, we could skip calling Flush() except when we're actually finished, so perhaps it evens out. Another option I was wondering about would be something like this: we could declare the space for the glyph buffer as a simple array of uint32_t, which shouldn't get zero-initialized when allocated on the stack (right?); and then just cast it to a Glyph* pointer to actually use it. WDYT?
Flags: needinfo?(jfkthame) → needinfo?(jmuizelaar)
(In reply to Jonathan Kew (:jfkthame) from comment #2) > Created attachment 8918978 [details] > alternative approach > > Hmmm, the union option.... I guess it should work, but it makes for some > really ugly code, IMO. > > I'd prefer an AutoTArray approach, I think; that seems a lot cleaner. But do > we know how much overhead that would have (in an opt build)? If it means > AppendGlyph() would call through to AppendElement, I guess there must be > some amount of overhead there (because AppendElement will need to check > whether it's going to overflow the auto-buffer and so needs to reallocate). > OTOH, if we allow the buffer to grow arbitrarily, we could skip calling > Flush() except when we're actually finished, so perhaps it evens out. Yeah, with an AutoTArray we can just size it ahead of time and then access the elements directly instead of using AppendElement. That will avoid the AppendElement overhead. > Another option I was wondering about would be something like this: we could > declare the space for the glyph buffer as a simple array of uint32_t, which > shouldn't get zero-initialized when allocated on the stack (right?); and > then just cast it to a Glyph* pointer to actually use it. WDYT? Yep, casting to Glyph* pointer would be fine. In thinking about it now, I don't really see any reason to prefer the union solution over your suggestion (though using AlignedStorage2 is probably more correct than uint32_t)
Flags: needinfo?(jmuizelaar)
OK, here's a version that uses AlignedStorage2 to avoid the zero-initialization in a relatively clean way.
Attachment #8919050 - Flags: review?(jmuizelaar)
Assignee: jmuizelaar → jfkthame
Status: NEW → ASSIGNED
Attachment #8919050 - Flags: review?(jmuizelaar) → review+
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ff38c5a5e2 Use AlignedStorage2 to avoid zero-initializing the array of glyphs in GlyphBufferAzure. r=jrmuizel
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: