Last Comment Bug 721511 - [cairo-dwrite] avoid excessive buffer allocation/deallocation when drawing glyph runs
: [cairo-dwrite] avoid excessive buffer allocation/deallocation when drawing gl...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-26 13:39 PST by Jonathan Kew (:jfkthame)
Modified: 2012-01-28 18:53 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, use stack-based buffers for moderate-sized glyph runs (11.02 KB, patch)
2012-01-26 13:39 PST, Jonathan Kew (:jfkthame)
bas: review+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2012-01-26 13:39:56 PST
Created attachment 591925 [details] [diff] [review]
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) 2012-01-26 17:52:17 PST
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) 2012-01-27 09:25:21 PST
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 4 Jonathan Kew (:jfkthame) 2012-01-28 01:54:07 PST
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) 2012-01-28 04:28:56 PST
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) 2012-01-28 18:53:22 PST
https://hg.mozilla.org/mozilla-central/rev/f36a9b649c6b

Note You need to log in before you can comment on or make changes to this bug.