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

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68557|review?                     |review-
               Flag|                            |




--- Comment #5 from David Levin <levin at chromium.org>  2010-09-23 13:27:02 PST ---
(From update of attachment 68557)
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?

> 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.

> WebCore/platform/graphics/chromium/FontLinux.cpp:492
> +    void resetGlyphArrays(int size)

Why pass in size? Just use m_item.num_glyphs.

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

s/becuase/because/

> 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.

> 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?

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.

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