Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2411643002:
Make HarfBuzzFace release SimpleFontData. (Closed)
|
Created:
4 years, 2 months ago by jb Modified:
4 years, 2 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake HarfBuzzFace release SimpleFontData.
HarfBuzzFace did a retained look up of SimpleFontData from the
FontDataCache but never released the SimpleFontData. This caused the
SimpleFontData to remain in the cache, indefinitely holding on to
SkFontFaces and all associated data. This fix makes HarfBuzzFace
release the SimpleFontData when deleted.
BUG=617568
Committed: https://crrev.com/af675a084051af4b0fd90a66ddbc1373e3ddae8d
Cr-Commit-Position: refs/heads/master@{#424993}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed m_simpleFontData from HarfBuzzFace. #
Total comments: 3
Messages
Total messages: 19 (6 generated)
Description was changed from ========== Make HarfBuzzFace release SimpleFontData. HarfBuzzFace did a retained look up of SimpleFontData from the FontDataCache but never released the SimpleFontData. This caused the SimpleFontData to remain in the cache, indefinitely holding on to SkFontFaces and all associated data. This fix makes HarfBuzzFace release the SimpleFontData when deleted. BUG= ========== to ========== Make HarfBuzzFace release SimpleFontData. HarfBuzzFace did a retained look up of SimpleFontData from the FontDataCache but never released the SimpleFontData. This caused the SimpleFontData to remain in the cache, indefinitely holding on to SkFontFaces and all associated data. This fix makes HarfBuzzFace release the SimpleFontData when deleted. BUG= ==========
jb@opera.com changed reviewers: + drott@chromium.org, eae@chromium.org
PTAL
That's a great find! I was looking for more unbalanced cache lookups to releaseFontData situations, but I didn't find this one. See https://bugs.chromium.org/p/chromium/issues/detail?id=617568 and blocking bugs. You found a really important one, let's look at how we can address this. The reason we need to reconfigure m_harfBuzzFontData->m_simpleFontData each time we get a call to getScaledFont is that this SimpleFontData object captures/encodes the font size. Whereas for the m_platformData object of HarfBuzzFace we sort of disregard the font size. There are two HarfBuzzFace HarfBuzz callback functions which indirectly make use of this size: harfBuzzGetGlyphVerticalOrigin, harfBuzzGetGlyphVerticalAdvance and they would fail if an old SimpleFontData object for a different size would be reused. The constructor of HarfBuzzFace only creates a new internal object (HbFontCacheEntry / HarfBuzzFontData) when a new unique Skia typeface ID comes in. Otherwise an existing HbFontCacheEntry is reused. We can call this HbFontCacheEntry "unscaled", as it only contains information about the typeface, but not about its size. The HarfBuzzFontData object inside it is the user pointer object for the HarfBuzz callback functions for glyph metrics, etc. So a HarfBuzzFace object has access to the unscaled cache entry and has its own FontPlatformData member, but in a way, the size information of that FontPlatformData is only used once we get a call to getScaledFont. Note again, however, that the cache key for the HbFontCacheEntry is not the full FontPlatformData object but only the SkTypeface's uniqueId. The releaseFontData call in the HarfBuzzFace destructor is definitely good. But then, instead of keeping the m_simpleFontData in HarfBuzzFace itself for comparison and assignment, I would suggest to put something like a updateSimpleFontData(FontPlatformData*) method in HarfBuzzFontData, and call that from getScaledFont. There, replace the old m_simpleFontData if the looked up new one is different, release the old one. Otherwise, release the new one. What do you think? Or it might be enough to just compare the size of m_fontPlatformData to the size of HarfBuzzFontData->m_simpleFontData->platformData()->size() and update if different. We should also be able to measure memory improvements from this - did you do any own measurements? I am drott on #blink IRC if you'd like to discuss more online. I also plan to work on removing the FontPlatformData/SimpleFontData duality - which is a mess. This is blocked on removing the simple font code path, but we should get there, soon. https://codereview.chromium.org/2411643002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right): https://codereview.chromium.org/2411643002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:154: FontCache::fontCache()->releaseFontData(m_simpleFontData); Yes, this definitely seems needed. https://codereview.chromium.org/2411643002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.h (right): https://codereview.chromium.org/2411643002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.h:73: mutable class SimpleFontData* m_simpleFontData; Because of the way the caching works in this class, I'd prefer to avoid additional storage in HarfBuzzFace itself for pointers to SimpleFontData or FontPlatformData. As much as possible should be in HarfBuzzFontData since these we have a lot fewer of those. SimpleFontData objects actually keep a copy of FontPlatformData objects, so this would be redundantly storing the same FontPlatformData inside the m_simpleFontData. A longer term consideration might be, whether we should hang HarfBuzzFace off of SimpleFontData or make the main m_platformData maybe a m_simpleFontData.
Thanks for clarifying. As for measurements. I picked up this leak in a page cycler test loading important sites in a single renderer (process per tab). Without the fix it accumulates more than 100 megs of font data over night. With this fix the font data stays below 10 megs with no indication of increasing. Page cycling of course isn't a realistic use case, so the real impact is much smaller. I don't have any more fine grained data like how much is leaked from one page to another.
https://codereview.chromium.org/2411643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right): https://codereview.chromium.org/2411643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:97: if (m_simpleFontData) I think in the else case we need to release the new simpleFontData, don't we? Or, alternatively, compare m_simpleFontData->platformData().size() to platformData.size() and only lookup a new one from the cache if they differ. If all LayoutTests pass after that, this should be fine, too.
Thanks for updating the CL.
https://codereview.chromium.org/2411643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right): https://codereview.chromium.org/2411643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:97: if (m_simpleFontData) On 2016/10/13 07:28:34, drott wrote: > I think in the else case we need to release the new simpleFontData, don't we? > > Or, alternatively, compare > m_simpleFontData->platformData().size() to platformData.size() and only lookup a > new one from the cache if they differ. If all LayoutTests pass after that, this > should be fine, too. I think you misread the code. I always release the old one. If there is no change, then releasing the old one has the same effect as releasing the new one, so the counter is balanced either way.
LGTM, again, great find, thanks for the CL. https://codereview.chromium.org/2411643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp (right): https://codereview.chromium.org/2411643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp:97: if (m_simpleFontData) > I think you misread the code. I always release the old one. If there is no change, then releasing the old one has the same effect as releasing the new one, so the counter is balanced either way. True, sorry.
Can you put bug 617568 in the CL description?
Description was changed from ========== Make HarfBuzzFace release SimpleFontData. HarfBuzzFace did a retained look up of SimpleFontData from the FontDataCache but never released the SimpleFontData. This caused the SimpleFontData to remain in the cache, indefinitely holding on to SkFontFaces and all associated data. This fix makes HarfBuzzFace release the SimpleFontData when deleted. BUG= ========== to ========== Make HarfBuzzFace release SimpleFontData. HarfBuzzFace did a retained look up of SimpleFontData from the FontDataCache but never released the SimpleFontData. This caused the SimpleFontData to remain in the cache, indefinitely holding on to SkFontFaces and all associated data. This fix makes HarfBuzzFace release the SimpleFontData when deleted. BUG=617568 ==========
On 2016/10/13 08:07:15, drott wrote: > Can you put bug 617568 in the CL description? Sure, NP.
The CQ bit was checked by jb@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make HarfBuzzFace release SimpleFontData. HarfBuzzFace did a retained look up of SimpleFontData from the FontDataCache but never released the SimpleFontData. This caused the SimpleFontData to remain in the cache, indefinitely holding on to SkFontFaces and all associated data. This fix makes HarfBuzzFace release the SimpleFontData when deleted. BUG=617568 ========== to ========== Make HarfBuzzFace release SimpleFontData. HarfBuzzFace did a retained look up of SimpleFontData from the FontDataCache but never released the SimpleFontData. This caused the SimpleFontData to remain in the cache, indefinitely holding on to SkFontFaces and all associated data. This fix makes HarfBuzzFace release the SimpleFontData when deleted. BUG=617568 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make HarfBuzzFace release SimpleFontData. HarfBuzzFace did a retained look up of SimpleFontData from the FontDataCache but never released the SimpleFontData. This caused the SimpleFontData to remain in the cache, indefinitely holding on to SkFontFaces and all associated data. This fix makes HarfBuzzFace release the SimpleFontData when deleted. BUG=617568 ========== to ========== Make HarfBuzzFace release SimpleFontData. HarfBuzzFace did a retained look up of SimpleFontData from the FontDataCache but never released the SimpleFontData. This caused the SimpleFontData to remain in the cache, indefinitely holding on to SkFontFaces and all associated data. This fix makes HarfBuzzFace release the SimpleFontData when deleted. BUG=617568 Committed: https://crrev.com/af675a084051af4b0fd90a66ddbc1373e3ddae8d Cr-Commit-Position: refs/heads/master@{#424993} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/af675a084051af4b0fd90a66ddbc1373e3ddae8d Cr-Commit-Position: refs/heads/master@{#424993} |
