On behalf of Claire who investigated the problem in Android. ChangeSet #61795 (http://trac.webkit.org/changeset/61795/trunk/WebCore/platform/graphics/chromium/FontLinux.cpp) did FontLinux.cpp cleanup to fix the complex script overlap problem. The problem is fixed probably by delete and new glyph arrays so the arrays are clean when use. Now, the overlap issue is gone but Android team reported there is a core dump in FontAndroid.cpp which is similar to FontLinux.cpp in Chrome. While investing the new core dump problem, Claire found there is unnecessary free/new memory in the code. Here is the high level code structure: Font:: *ComplexText ... while (walker.nextScriptRun()) { .... } ... bool nextScriptRun() { ... shapeGlyphs(); ... } void shapeGlyphs() { ... for (;;) { if (HB_ShapeItem(&m_item)) ==> m_item.num_glyphs is changed after this call. break; deleteGlyphArrays(); createGlyphArrays(m_item.num_glyphs); ==> delete/createGlyphArrays are hit if m_item.num_glyphs > previous m_item.num_glyphs ... } HB_Bool HB_ShapeItem(HB_ShaperItem *shaper_item) { HB_Bool result = false; if (shaper_item->num_glyphs < shaper_item->item.length) { shaper_item->num_glyphs = shaper_item->item.length; return false; } ... } So, Claire proposed to resume the setting of m_item.num_glyphs = m_maxGlyphs before HB_ShapeItem() call to minimize the number of delete/new and always cleanup all glyph arrays before re-using them.
Created attachment 68550 [details] patch
Attachment 68550 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/graphics/chromium/FontLinux.cpp:488: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 68557 [details] patch
By my reading this patch ends up zeroing already zeroed memory and saves no allocations. Maybe I'm missing something, but I think you need to be clearer.
Comment on attachment 68557 [details] patch 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.
(In reply to comment #4) > By my reading this patch ends up zeroing already zeroed memory and saves no allocations. Maybe I'm missing something, but I think you need to be clearer. 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* the no zero-ing array is called either, so the next script run's shaping is based on a dirty array. This is one issue the patch addressed. 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. And above delete/new will introduce extra overhead. This is the 2nd issue the patch addressed.
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.
(In reply to comment #6) > Following is the change of m_item.num_glyphs after the call of HB_ShapeItem(). > > 54 --> 5 > 5 --> 1 > 1 --> 8 > ..... <snip> Given the above I understand what the patch is trying to do and LGTM with David's comments address. However, the ChangeLog description needs to be made a lot clearer, ideally by using the explanation from #6.
Created attachment 68789 [details] patch patch address David's comments.
Comment on attachment 68789 [details] patch 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.
Committed r68618: <http://trac.webkit.org/changeset/68618>
Due to my mistake on git commit, I've committed a set of unsquashed commits for the bug. http://trac.webkit.org/changeset/68618 is the revert of the batch commits. And I will recommit the fix.
Committed r68660: <http://trac.webkit.org/changeset/68660>
This changed the behavior of fast/text/international/thai-baht-space.html. It used to produce this output: http://trac.webkit.org/export/61795/trunk/LayoutTests/platform/chromium-linux/fast/text/international/thai-baht-space-expected.png Now it produces this: http://trac.webkit.org/export/68847/trunk/LayoutTests/platform/chromium-linux/fast/text/international/thai-baht-space-expected.png Is this an expected change or a regression? I know when I've tried to do cleanups in FontLinux.cpp before I've accidentally broken this test.
The new one looks more correct to me (compare it against the expected images from other platforms)
(In reply to comment #14) > This changed the behavior of fast/text/international/thai-baht-space.html. > > It used to produce this output: http://trac.webkit.org/export/61795/trunk/LayoutTests/platform/chromium-linux/fast/text/international/thai-baht-space-expected.png > > Now it produces this: http://trac.webkit.org/export/68847/trunk/LayoutTests/platform/chromium-linux/fast/text/international/thai-baht-space-expected.png > > Is this an expected change or a regression? I know when I've tried to do cleanups in FontLinux.cpp before I've accidentally broken this test. Verified with Jungshik and the new one is correct. The difference is space before or after 5 around 5000. It should be before. I had problem with rebaseline and thanks hclam for rebaselining.
Awesome! Thanks for looking.