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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 24 17:39:47 PDT 2010


David Levin <levin at chromium.org> has granted 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 68789: patch
https://bugs.webkit.org/attachment.cgi?id=68789&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68789&action=review

Please consider adjusting the ChangeLog.

> WebCore/ChangeLog:46
> +	   and new glyph arrays so the arrays in use are clean initially.
> +	   But it introduced more delete/new operation which might be a problem
in
> +	   memory tight devices (such as Android device).
> +
> +	   This fix proposed to
> +	   1. resume the setting of 
> +	   m_item.num_glyphs = m_maxGlyphs before HB_ShapeItem() call to
minimize
> +	   the number of delete/new operations
> +	   2. always cleanup the glyph arrays before re-use them to make sure
> +	   the arrays in use are clean initially.
> +
> +	   Using http://www.google.ae/preferences?hl=ar as an example.
> +
> +	   Following is the change of m_item.num_glyphs after the call of
HB_ShapeItem().
> +
> +	   54 --> 5
> +	   5 --> 1
> +	   1 --> 8
> +	   .....
> +
> +	   The array is zero-ed initially (with size 54), so there is no
problem
> +	   when shaping the first script run. After shaping, the
m_item.num_glyphs
> +	   changed to 5.
>  
> +	   Then, when shaping next script run, since there is enough space
available
> +	   for HB_ShapeItem(), no deleteGlypyArrays/createGlyphArrays will be
called,
> +	   but zero-ing array is not called either, so the next script run's
> +	   shaping is based on a dirty array. This is 2nd issue mentioned above
that
> +	   the patch addresses.
> +
> +	   In the 3rd script run, although there is actually an array of size
54,
> +	   the recorded num_glyphs is 1, and it is less than the needed size,
so,
> +	   array of size 54 is deleted and an array of size 8 is created. 
> +	   The increase of num_glyphs from one run to its next is not uncommon.

> +	   It is hit 1096 times during loading the above example page. So the
> +	   delete/new will introduce extra overhead. This is the 1st issue
mentioned
> +	   above that the patch addresses.
> +

This is a bit verbose.

How about this: 

Reduce new/delete operations by storing the maximum capacity of the glyph array
and use value that
in subsequent HB_ShapeItem calls. (Note that a call to HB_ShapeItem may reduce
the value of m_item.num_glyphs
below the capacity.)

Also be consistent with zero'ing the glyph arrays before calling HB_ShapeItem.


More information about the webkit-reviews mailing list