721511 - [cairo-dwrite] avoid excessive buffer allocation/deallocation when drawing glyph runs

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: patch, use stack-based buffers for moderate-sized glyph runs

While digging through DWrite/D2D text rendering in the cairo backend, I noticed that we're allocating and then releasing three separate buffers (glyph indices, advances, offsets) for every glyph run we draw.

In practice, virtually all glyph runs are short, so we can normally avoid the new/delete operations by using stack-based buffers instead. A stack-based helper class wrapping DWRITE_GLYPH_RUN makes this pretty simple.

Comment 1

[Bas Schouten (:bas.schouten)]

Comment on attachment 591925 [details] [diff] [review]
patch, use stack-based buffers for moderate-sized glyph runs

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

I like this change! In the long run I think the best thing we could do is draw a lot more glyphs per call though (and not a single line as we do now), this might give us a considerable performance improvement when rendering text with D2D (right now we fill the command buffer much more quickly, causing more regular flushes and more frequent texture access).

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 591925 [details] [diff] [review]
patch, use stack-based buffers for moderate-sized glyph runs

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

I like this change! In the long run I think the best thing we could do is draw a lot more glyphs per call though (and not a single line as we do now), this might give us a considerable performance improvement when rendering text with D2D (right now we fill the command buffer much more quickly, causing more regular flushes and more frequent texture access).

Comment 3

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea682dba295

Comment 4

[Jonathan Kew [:jfkthame]]

Backed out for a crashtest failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb21301bbdd3

TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/base/crashtests/149014-1.html | Exited with code 253 during test run
PROCESS-CRASH | file:///C:/talos-slave/test/build/reftest/tests/layout/base/crashtests/149014-1.html | application crashed (minidump found)

Crash reason:  EXCEPTION_ACCESS_VIOLATION_WRITE
Crash address: 0x8

Thread 0 (crashed)
 0  ntdll.dll + 0x46b90
    eip = 0x77906b90   esp = 0x001377c4   ebp = 0x001377d8   ebx = 0x00100000
    esi = 0x00000008   edi = 0x00000004   eax = 0x00000008   ecx = 0x00138528
    edx = 0x005800f0   efl = 0x00210216
    Found by: given as instruction pointer in context
 1  mozglue.dll!arena_dalloc [jemalloc.c:2ea682dba295 : 4624 + 0xc]
    eip = 0x733b3d69   esp = 0x001377e0   ebp = 0x001377f4
    Found by: previous frame's frame pointer
 2  mozglue.dll!je_free [jemalloc.c:2ea682dba295 : 6580 + 0x5]
    eip = 0x733b4202   esp = 0x001377fc   ebp = 0x00137898   ebx = 0x00137928
    Found by: call frame info
 3  xul.dll!_cairo_dwrite_scaled_show_glyphs(void *,_cairo_operator,_cairo_pattern const *,_cairo_surface *,int,int,int,int,unsigned int,unsigned int,cairo_glyph_t *,int,_cairo_region *,int *) [cairo-dwrite-font.cpp:2ea682dba295 : 707 + 0x7]
    eip = 0x6be76cf3   esp = 0x00137804   ebp = 0x00137898
    Found by: call frame info

Comment 5

[Jonathan Kew [:jfkthame]]

Re-landed, with the redundant delete[]s in _cairo_dwrite_scaled_show_glyphs removed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f36a9b649c6b

Comment 6

[Joe Drew (not getting mail)]

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