1408611 - Avoid initializing glyph buffer needlessly

Status: RESOLVED FIXED

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

Description

[Jeff Muizelaar [:jrmuizel]]

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()

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use a union to avoid initializing Points

Here's a possible approach to solving this. An alternative would be to use AutoTArray. What would you prefer Jonathan?

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: 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.

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?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(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)

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Use AlignedStorage2 to avoid zero-initializing the array of glyphs in GlyphBufferAzure

OK, here's a version that uses AlignedStorage2 to avoid the zero-initialization in a relatively clean way.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ff38c5a5e2
Use AlignedStorage2 to avoid zero-initializing the array of glyphs in GlyphBufferAzure. r=jrmuizel

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f6ff38c5a5e2