Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1919183002:
Store null font lookup result into font cache (Closed)
DescriptionStore null font lookup result into font cache
Font name lookup issues an expensive synchronous IPC on layout
calculation. The result of the IPC is usually cached in the
renderer, but a null result was not cached. That causes
repeated font name query for the same font name.
This CL adds the null result into the cache, so that the query
happens only once.
This reduces the layout time of http://b.hatena.ne.jp from
309ms to 178ms, and reduces its time to first paint from 690ms
to 477ms on Linux.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1919183002
Patch Set 1 #
Messages
Total messages: 21 (7 generated)
Description was changed from ========== Store null font lookup result into font cache BUG=skia: ========== to ========== Store null font lookup result into font cache BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Store null font lookup result into font cache BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Store null font lookup result into font cache BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
tzik@chromium.org changed reviewers: + kouhei@chromium.org
Description was changed from ========== Store null font lookup result into font cache BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Store null font lookup result into font cache Font name lookup issues an expensive synchronous IPC on layout calculation. The result of the IPC is usually cached in the renderer, but a null result was not cached. That causes repeated font name query for the same font name. This CL adds the null result into the cache, so that the query happens only once. This reduces the layout time of http://b.hatena.ne.jp from 309ms to 178ms, and reduces its time to first paint from 690ms to 477ms on Linux. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919183002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919183002/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-04-26 19:41 UTC
Kouhei-san: Could you PTAL to this before I send this to skia reviewer?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
kouhei@chromium.org changed reviewers: + bungeman@google.com, eae@chromium.org, reed@google.com
+eae,bungeman,reed o_O. The number is super impressive.
Very exciting!
If this look promising, we want to measure the real-world impact of the CL via finch. Would it be possible to make the change see RtConf and enable/disable it from browser side?
On 2016/04/26 23:51:37, kouhei wrote: > +eae,bungeman,reed > > o_O. The number is super impressive. The numbers are super depressing. The cache here was created as a crutch for gfx and we need to move toward no longer needing it, not 'improving' it. If this CL is really making a big difference in blink, that means that blink is repeatedly making the same request over and over. I would have thought that this would already be handled by blink's FontCache. Is there a reason why it isn't?
+1 Can we assist in improving the client-side cache to catch these?
On 2016/04/27 14:29:02, bungeman-skia wrote: > On 2016/04/26 23:51:37, kouhei wrote: > > +eae,bungeman,reed > > > > o_O. The number is super impressive. > > The numbers are super depressing. The cache here was created as a crutch for gfx > and we need to move toward no longer needing it, not 'improving' it. If this CL > is really making a big difference in blink, that means that blink is repeatedly > making the same request over and over. I would have thought that this would > already be handled by blink's FontCache. Is there a reason why it isn't? The difference between SkFontHostRequestCache and blink::FontCache is granularity: blink::FontCache uses the font size as a part of cache key while SkFontHostRequestCache doesn't. Blink issues the request for each font size, and non-null results are served from SkFontHostRequestCache if available, but null results are not.
On 2016/04/28 18:56:13, tzik wrote: > On 2016/04/27 14:29:02, bungeman-skia wrote: > > On 2016/04/26 23:51:37, kouhei wrote: > > > +eae,bungeman,reed > > > > > > o_O. The number is super impressive. > > > > The numbers are super depressing. The cache here was created as a crutch for > gfx > > and we need to move toward no longer needing it, not 'improving' it. If this > CL > > is really making a big difference in blink, that means that blink is > repeatedly > > making the same request over and over. I would have thought that this would > > already be handled by blink's FontCache. Is there a reason why it isn't? > > The difference between SkFontHostRequestCache and blink::FontCache is > granularity: blink::FontCache uses the font size as a part of cache key while > SkFontHostRequestCache doesn't. > Blink issues the request for each font size, and non-null results are served > from SkFontHostRequestCache if available, but null results are not. So Blink (on *nix) makes requests without a size, but then caches with a size? On Mac it appears Blink is using the size in the query, does it have the same issue?
Also, apparently this extra font request cache is a drag on general performance in Blink anyhow, see https://bugs.chromium.org/p/chromium/issues/detail?id=587889#c12 . If we can fix gxf not to need it then it can just be removed.
On 2016/04/28 21:18:28, bungeman-skia wrote: > On 2016/04/28 18:56:13, tzik wrote: > > On 2016/04/27 14:29:02, bungeman-skia wrote: > > > On 2016/04/26 23:51:37, kouhei wrote: > > > > +eae,bungeman,reed > > > > > > > > o_O. The number is super impressive. > > > > > > The numbers are super depressing. The cache here was created as a crutch for > > gfx > > > and we need to move toward no longer needing it, not 'improving' it. If this > > CL > > > is really making a big difference in blink, that means that blink is > > repeatedly > > > making the same request over and over. I would have thought that this would > > > already be handled by blink's FontCache. Is there a reason why it isn't? > > > > The difference between SkFontHostRequestCache and blink::FontCache is > > granularity: blink::FontCache uses the font size as a part of cache key while > > SkFontHostRequestCache doesn't. > > Blink issues the request for each font size, and non-null results are served > > from SkFontHostRequestCache if available, but null results are not. > > So Blink (on *nix) makes requests without a size, but then caches with a size? Yes on Linux and Windows: FontCache::createFontPlatformData [1,2] takes |fontSize| and |fontDescription| (which has the font size [3]), but the size is not used to create SkTypeface [4]. Then it's stored as a blink::FontPlatformData with blink::FontCacheKey, which contain the font size [5,6]. [1] http://crrev.com/4ee9d76953f16811/third_party/WebKit/Source/platform/fonts/sk... [2] http://crrev.com/4ee9d76953f16811/third_party/WebKit/Source/platform/fonts/wi... [3] http://crrev.com/4ee9d76953f16811/third_party/WebKit/Source/platform/fonts/Fo... [4] http://crrev.com/4ee9d76953f16811/third_party/WebKit/Source/platform/fonts/sk... [5] http://crrev.com/4ee9d76953f16811/third_party/WebKit/Source/platform/fonts/Fo... [6] http://crrev.com/4ee9d76953f16811/third_party/WebKit/Source/platform/fonts/Fo... > On Mac it appears Blink is using the size in the query, does it have the same > issue? Probably, though I didn't measured it.
On 2016/04/29 02:53:29, tzik wrote: > On 2016/04/28 21:18:28, bungeman-skia wrote: > > On 2016/04/28 18:56:13, tzik wrote: > > > On 2016/04/27 14:29:02, bungeman-skia wrote: > > > > On 2016/04/26 23:51:37, kouhei wrote: > > > > > +eae,bungeman,reed > > > > > > > > > > o_O. The number is super impressive. > > > > > > > > The numbers are super depressing. The cache here was created as a crutch > for > > > gfx > > > > and we need to move toward no longer needing it, not 'improving' it. If > this > > > CL > > > > is really making a big difference in blink, that means that blink is > > > repeatedly > > > > making the same request over and over. I would have thought that this > would > > > > already be handled by blink's FontCache. Is there a reason why it isn't? > > > > > > The difference between SkFontHostRequestCache and blink::FontCache is > > > granularity: blink::FontCache uses the font size as a part of cache key > while > > > SkFontHostRequestCache doesn't. > > > Blink issues the request for each font size, and non-null results are served > > > from SkFontHostRequestCache if available, but null results are not. > > > > So Blink (on *nix) makes requests without a size, but then caches with a size? > > Yes on Linux and Windows: > FontCache::createFontPlatformData [1,2] takes |fontSize| and |fontDescription| > (which has the font size [3]), but the size is not used to create SkTypeface > [4]. Then it's stored as a blink::FontPlatformData with blink::FontCacheKey, > which contain the font size [5,6]. Good find, we should probably fix that... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
