[webkit-reviews] review denied: [Bug 46374] [Chromium] FontLinux performance improvement and cleanup : [Attachment 68557] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 46374: [Chromium] FontLinux performance improvement and cleanup
https://bugs.webkit.org/show_bug.cgi?id=46374

Attachment 68557: patch
https://bugs.webkit.org/attachment.cgi?id=68557&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.