230289 - Size reduction in nsFontMetricsXft.cpp

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[jim_nance]

I found some code in nsFontMetricsXft.cpp that I think is wrong, or at least
unnecessary.  I am reworking some of the classes in this file and this code is
getting in the way.

In the DrawString methods of nsFontMetricsXft we have code that looks like this:

2036         specBuffer[specBufferLen].x = x;                    
2037         specBuffer[specBufferLen].y = y;                   
2038         specBuffer[specBufferLen].font = mXftFont;
2039         FT_UInt glyph = CharToGlyphIndex(*pstr);
2040         specBuffer[specBufferLen].glyph = glyph;
2041                                                                
2042         /* check to see if this glyph is non-empty */ 
2043         if (!data->foundGlyph) {                           
2044             XGlyphInfo info;                        
2045             XftGlyphExtents(GDK_DISPLAY(), mXftFont, &glyph, 1, &info);
2046             if (info.width && info.height)                  
2047               data->foundGlyph = PR_TRUE;                      
2048         }

and:
                              
665     // go forth and blit!
666     if (data.foundGlyph)
667         XftDrawGlyphFontSpec(data.draw, &data.color, data.specBuffer,
668                              data.specBufferLen);

data.foundGlyph is set if ANY character in the string has a glyph.  Thus if we
get a string which has some charcters with glyphs and some without we will draw
them all.  The only time we will skip the call to XftDrawGlyphFontSpec() is if
NONE of the characters have glyphs.

This seems like an odd case to optimize for.  If drawing glyphless characters is
expensive we should check to see if each character has a glyph and not put
glyphless characters into the specBuffer.  If drawing glyphless characters is
cheap (which I think is the case), we should not check to see if the character
has a glyph, which would allow us to skip the call to XftGlyphExtents().

Comments?

Comment 1

[Jungshik Shin]

That code was not put in there for the optimization. It was put in there to
prevent the crash. Check it out with lxr and also see bug 175108.

Comment 2

[jim_nance]

Thanks, I just read the bug, and understand why its there a bit better.

Given that we have to check to see if the character has a good glyph at the same
place in the code where we put it into the specBuffer, would it be acceptable to
just not put it into the buffer if it was glyphless, at least until we find the
first one with a glyph?

Comment 3

[jim_nance]

Attachment: Patch to remove dynamic memory allocation of Font Spec Buffer

I would be interested in knowing what you think of this patch, which is what I
was working on when I found the code that prompted me to file this bug.

What it does is remove the code that dynamically allocates the draw spec buffer
and replaces it with a class that maintains a fixed sized buffer and can flush
it multiple times if necessary.  We avoid the possibility of failure due to out
of memory conditions this way, and the code is simpler as well.

Comment 4

[Christopher Blizzard (:blizzard)]

Can you verify that this is both a code-size and performance win?

Comment 5

[jim_nance]

I do not have any performance data.  I can confirm that the code is smaller:

   text	   data	    bss	    dec	    hex	filename
  29794	   1084	  36064	  66942	  1057e	nsFontMetricsXft.o (orig)
  29482	   1084	     64	  30630	   77a6	nsFontMetricsXft.o (new)

Comment 6

[rbs]

Comment on attachment 138548 [details] [diff] [review]
Patch to remove dynamic memory allocation of Font Spec Buffer

I like the patch...

+class nsXftDrawBuffer;

what about |nsAutoDrawSpecBuffer|.

It is customary to use the prefix |auto| for such classes. This way, the radars
of the reader immediately flash that some magic is going on (especially in the
dtor), making reading/understanding the code easier and faster.

While you are at it, you can rename |FillDrawStringSpec| to |DrawStringSpec|
because it does much more that just fill'ing.

-static XftGlyphFontSpec gFontSpecBuffer[FONT_SPEC_BUFFER_SIZE];

Either retain |FONT_SPEC_BUFFER_SIZE| or remove its #define.

Comment 7

[jim_nance]

Attachment: changes updated with requests from rbs

Here is the patch with the changes you asked for included.

Comment 8

[jim_nance]

Attachment: found a couple unused fields I had forgotten to remove

Comment 9

[rbs]

Comment on attachment 138636 [details] [diff] [review]
found a couple unused fields I had forgotten to remove

sr=rbs

+    ~nsAutoDrawSpecBuffer() {flush();}
+    if(mSpecPos) {
+	 for (PRUint32 i = 0; i<mSpecPos; i++) {

inconsistent spacing

=================
     nsresult rv;
     rv = EnumerateGlyphs(aString, aLength,
			  &nsFontMetricsXft::DrawStringCallback, &data);
-    if (NS_FAILED(rv)) {
-	 FreeSpecBuffer(data.specBuffer);
-	 return rv;
-    }
-
-    // go forth and blit!
-    if (data.foundGlyph)
-	 XftDrawGlyphFontSpec(data.draw, &data.color, data.specBuffer,
-			      data.specBufferLen);
-
-    FreeSpecBuffer(data.specBuffer);

Just use |return EnumerateGlyphs| (2 places).

================
     // actually fill up the specbuffer converting the input string

Should be |actually process the specbuffer...| or something like that.

Comment 10

[Jungshik Shin]

> you can rename |FillDrawStringSpec| to |DrawStringSpec|
> because it does much more that just fill'ing.
> |actually process the specbuffer...| or something like th

 'Fill' has to be there. 'DrawStringSpec' is the object of 'Fill'. Whatever more
it may do, it does over the input string to fill up the spec buffer. If a change
is wanted, it should be 'FillSpecBuffer' (or 'ProcessStringAndFillSpecBuffe). 
Processing the spec. buffer is done by XftDrawGlyphFontSpec after |spec buffer|
is all filled up. 

Comment 11

[jim_nance]

Attachment: Incorporated requests for jshin and rbs

Comment 12

[rbs]

I don't think it is necessary to change back to 'Fill' (attachment 138636 [details] [diff] [review] is
still the one I prefer). The semantic has changed, and 'Fill' is distracting
from what is now going on. Consider for example that we usually don't say
'Copy'XXX if the copying is a secondary thing going on. (I was the one who
suggested that Fill-name - if my memory serve me right). It used to be that the
Fill'ing will be done, and then followed by XftDrawGlyphFontSpec. Now however,
there are merged in a subtle way, and since drawing is the whole point of these
genuflexions, DrawStringSpec (viz. DrawStringCallback) seems to me a short and
instructive name, reflecting just just what is needed.

Comment 13

[jim_nance]

Whatever you two want to call it is fine with me.  I just need you both to agree
on something :-)

Comment 14

[rbs]

jim, care to re-submit your patch (attachment 138696 [details] [diff] [review]) with the name change back
to DrawStringSpec. Remember to also use the Edit action to enqueue your r/sr
requests.

Comment 15

[jim_nance]

Attachment: latest patch using rbs naming

Patch with DrawStringSpec as the method name.

Comment 16

[rbs]

Comment on attachment 138988 [details] [diff] [review]
latest patch using rbs naming

sr=rbs

Comment 17

[Christopher Blizzard (:blizzard)]

+void
+nsAutoDrawSpecBuffer::draw(nscoord x, nscoord y, XftFont *font, FT_UInt glyph)

Methods have upper case names, not lower case names.  This isn't JavaScript.

Comment 18

[jim_nance]

Attachment: Patch with Blizzard's naming changes

Thanks Chris.  I have updated the patch to change the flush and draw method so
that they start with capital letters.

Comment 19

[Jungshik Shin]

Comment on attachment 139158 [details] [diff] [review]
Patch with Blizzard's naming changes

r=jshin
sorry for the delay. I wanted to make sure that this change is not in conflict
with what I'm doing in bug 215219. It seems like it doesn't.

Comment 20

[jim_nance]

This was checked in a long time ago.  I never closed the bug.