Last Comment Bug 311274 - Transfer cairo glyph optimizations from trunk to branch
: Transfer cairo glyph optimizations from trunk to branch
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 1.8 Branch
: All All
: -- normal (vote)
: ---
Assigned To: tor
: Hixie (not reading bugmail)
:
Mentors:
: 312191 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-05 15:26 PDT by tor
Modified: 2007-02-15 16:51 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
merge cairo text optimizations from trunk (10.15 KB, patch)
2005-10-05 15:27 PDT, tor
asa: approval1.8rc1-
Details | Diff | Splinter Review
more minimal patch, comments from brendan addressed (6.46 KB, patch)
2005-10-12 11:47 PDT, tor
scootermorris: review+
vladimir: superreview+
asa: approval1.8rc1+
Details | Diff | Splinter Review

Description tor 2005-10-05 15:26:48 PDT
As part of <textPath> implemention I did some performance and specification
conformance improvements to the svg cairo backend glue.  Profiling a slow svg
with a moderate amount of text, the areas that I improved are showing at the top
of the hits.  Merging in just those changes from the trunk gives about a 3x
improvement in this case (empire).

The first major change is hit detection based. the character boxes, rather than
the stroked boundaries.  The other is calculation region coverage based on the
text extents, rather than always getting the extent of the stroked boundaries
(which is still used if someone strokes text).

This code has been baking on the trunk since August 26th.
Comment 1 tor 2005-10-05 15:27:56 PDT
Created attachment 198635 [details] [diff] [review]
merge cairo text optimizations from trunk
Comment 2 tor 2005-10-07 13:01:57 PDT
Comment on attachment 198635 [details] [diff] [review]
merge cairo text optimizations from trunk

Very large performance boost for SVG when dealing with large amounts of text
and/or dynamic text (3x speed increase seen in real svg use with moderate
amount of dynamic text mixed with other graphical elements).  This code has
been baking in the trunk for six weeks, should be safe.
Comment 3 Asa Dotzler [:asa] 2005-10-10 14:46:54 PDT
Comment on attachment 198635 [details] [diff] [review]
merge cairo text optimizations from trunk

Might have taken something like this in Beta2, but we're approaching RC1 which
could be our final release so we don't really have room to take such large
changes.
Comment 4 Brendan Eich [:brendan] 2005-10-12 11:18:32 PDT
Comment on attachment 198635 [details] [diff] [review]
merge cairo text optimizations from trunk

A minimal patch would help get this into 1.8.  The cast cleanups were optional,
right?

>-  cairo_text_path(ctx, NS_ConvertUCS2toUTF8(text).get());
>+  if (hasCoveredStroke) {
>+    cairo_text_path(ctx, NS_ConvertUCS2toUTF8(text).get());
>+  } else {
>+    cairo_text_extents_t extent;
>+    cairo_text_extents(ctx,
>+                       NS_ConvertUCS2toUTF8(text).get(),
>+                       &extent);
>+    cairo_rectangle(ctx, x + extent.x_bearing, y + extent.y_bearing,
>+                    extent.width, extent.height);
>+  }
> 
>   double xmin, ymin, xmax, ymax;
> 
>   if (hasCoveredStroke) {

Shaver points out that you have two hasCoveredStroke tests that (unless the
else clause could cause that flag to be set somehow) should be unified here.

>-  float sX, sY, eX, eY, eWidth, eHeight;
>-  mSource->GetX(&sX);
>-  mSource->GetY(&sY);
>-  box->GetX(&eX);
>-  box->GetY(&eY);
>-  box->GetWidth(&eWidth);
>-  box->GetHeight(&eHeight);
>-  cairo_rectangle(ctx, sX + eX, sY + eY, eWidth, eHeight);
>+  metrics->SelectFont(ctx);
>+
>+  nsAutoString text;
>+  mSource->GetCharacterData(text);
>+
>+  float xx, yy;
>+  mSource->GetX(&xx);
>+  mSource->GetY(&yy);
>+
>+  cairo_matrix_t matrix;
>+
>+  for (PRUint32 i=0; i<text.Length(); i++) {

Comment what's going on here?

/be
Comment 5 tor 2005-10-12 11:39:36 PDT
(In reply to comment #4)
> (From update of attachment 198635 [details] [diff] [review] [edit])
> A minimal patch would help get this into 1.8.  The cast cleanups were optional,
> right?

Yes, I was trying to keep the new branch code as close as possible to the code
that had been baking on the trunk.  I'll attach a more minimal patch.

> >-  cairo_text_path(ctx, NS_ConvertUCS2toUTF8(text).get());
> >+  if (hasCoveredStroke) {
> >+    cairo_text_path(ctx, NS_ConvertUCS2toUTF8(text).get());
> >+  } else {
> >+    cairo_text_extents_t extent;
> >+    cairo_text_extents(ctx,
> >+                       NS_ConvertUCS2toUTF8(text).get(),
> >+                       &extent);
> >+    cairo_rectangle(ctx, x + extent.x_bearing, y + extent.y_bearing,
> >+                    extent.width, extent.height);
> >+  }
> > 
> >   double xmin, ymin, xmax, ymax;
> > 
> >   if (hasCoveredStroke) {
> 
> Shaver points out that you have two hasCoveredStroke tests that (unless the
> else clause could cause that flag to be set somehow) should be unified here.

This is the result of starting from the trunk code and removing the textPath
code.  New patch will combine the blocks.

> >-  float sX, sY, eX, eY, eWidth, eHeight;
> >-  mSource->GetX(&sX);
> >-  mSource->GetY(&sY);
> >-  box->GetX(&eX);
> >-  box->GetY(&eY);
> >-  box->GetWidth(&eWidth);
> >-  box->GetHeight(&eHeight);
> >-  cairo_rectangle(ctx, sX + eX, sY + eY, eWidth, eHeight);
> >+  metrics->SelectFont(ctx);
> >+
> >+  nsAutoString text;
> >+  mSource->GetCharacterData(text);
> >+
> >+  float xx, yy;
> >+  mSource->GetX(&xx);
> >+  mSource->GetY(&yy);
> >+
> >+  cairo_matrix_t matrix;
> >+
> >+  for (PRUint32 i=0; i<text.Length(); i++) {
> 
> Comment what's going on here?

Comment added.

Comment 6 tor 2005-10-12 11:47:51 PDT
Created attachment 199324 [details] [diff] [review]
more minimal patch, comments from brendan addressed
Comment 7 tor 2005-10-12 14:11:09 PDT
*** Bug 312191 has been marked as a duplicate of this bug. ***
Comment 8 Vladimir Vukicevic [:vlad] [:vladv] 2005-10-12 15:44:31 PDT
Comment on attachment 199324 [details] [diff] [review]
more minimal patch, comments from brendan addressed

r=me; this looks pretty safe and lightweight.  Only SVG can really be affected
by this.
Comment 9 tor 2005-10-13 11:31:43 PDT
Checked in on branch.

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