[Webkit-unassigned] [Bug 46374] [Chromium] FontLinux performance improvement and cleanup

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46374





--- Comment #7 from Xiaomei Ji <xji at chromium.org>  2010-09-23 18:34:47 PST ---
Thanks for the review. Please see my comments inline.

(In reply to comment #5)
> (From update of attachment 68557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68557&action=review
> 
> Is this optimization worthwhile? Is it good to keep around larger arrays than you need?

Using http://www.google.ae/preferences?hl=ar as an example, the deleteGlyphArrays()/createGlyphArrays() is hit 1096 times during loading the page.

But I do not have much idea on how large one text run could be.

> 
> > WebCore/platform/graphics/chromium/FontLinux.cpp:488
> > +        m_glyphsArraySize = size; // Save the GlyphArrays size.
> 
> It seems like m_glyphsArrayCapacity (or max capacity) may be a better term here to reflect the fact that it stores the maximum size of the arrays.

Done.

> 
> > WebCore/platform/graphics/chromium/FontLinux.cpp:492
> > +    void resetGlyphArrays(int size)
> 
> Why pass in size? Just use m_item.num_glyphs.

Done.

> 
> > WebCore/platform/graphics/chromium/FontLinux.cpp:506
> > +        // Reset the array limit becuase HB_ShapeItem() overrides the
> 
> s/becuase/because/

Done.

> 
> > WebCore/platform/graphics/chromium/FontLinux.cpp:507
> > +        // m_item.num_glyphs.
> 
> This comment is very unclear. As I understand it, you're trying to say that the capacity of the arrays may be larger than the current value of m_item.num_glyphs  due to a previous call to HB_ShapeItem which used less space than was available.

You are right. Changed comments.

> 
> > WebCore/platform/graphics/chromium/FontLinux.cpp:509
> > +        resetGlyphArrays(m_glyphsArraySize);
> 
> This shouldn't be done unless the current glyphs aren't deleted (or else you are zero'ing out stuff that doesn't need to be cleared).  btw, is it a bug in the old code that the for loop could have exited without clearing the glyphs if the array was big enough to hold them?


As I understand, this function only be called once for one run. So, there is no data sharing problem within one run, and the data should not be shared between different runs. Adam, Evan, please chime in on whether the assumption is correct.

The zero'ing out stuff is done right at the beginning of the function, not within the "for" loop inside the function, it should be correct based on the above assumption.

And yes you are right that the old code exit the for loop without clearing the glyphs if the array was big enough to hold them. It is exactly the problem the fix is solving. But we should not clean the buffer after exit for loop since the content of the buffer might be used for subsequent operations. Instead, the fix cleans the buffer before re-use it (a bit waste since it does not need to clean those not dirty ones, but memset should be fast enough, I think).


> 
> One solution to this issue is to make a separate api for allocate that doesn't reset the arrays. Only call the allocate array inside of this loop and then call the reset function after the loop exits.
> 
> > WebCore/platform/graphics/chromium/FontLinux.cpp:611
> > +    unsigned m_glyphsArraySize; // Current size of all the Harfbuzz arrays.
> 
> I would say capacity instead of size.

Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.