Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(218)

Issue 2199333008: Remove glyph metrics caching on non-Mac platforms (Closed)

Created:
4 years, 4 months ago by drott
Modified:
4 years, 4 months ago
Reviewers:
kojii, eae, tzik
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
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove glyph metrics caching on non-Mac platforms As shown in the benchmarking results discussed in crbug.com/631032 removing the glyph metrics caching in Blink on non Mac platforms and only using the lower level Skia caches instead should not affect overall layout and text performance negatively. In fact, some performance tests such as html5-full-render and html-parser-threaded are improving in local measurements on linux. This should give us improvements in memory consumption ranging from tens of kilobytes in a single-origin use renderer up to over a megabyte on longer lifespan renderers such as in the Android WebView and help with crbug.com/577306. On Mac platforms we are still using path based glyph metrics, which seem to be too slow. Here, the glyph metrics caching in Blink cannot yet be removed, compare https://bugs.chromium.org/p/chromium/issues/detail?id=608338#c9 BUG=589462, 631032 R=kojii,tzik,eae Committed: https://crrev.com/3af81d9791986c5d47b261dda6f2a672e65ae313 Cr-Commit-Position: refs/heads/master@{#409564}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -3 lines) Patch
M third_party/WebKit/Source/platform/fonts/SimpleFontData.h View 6 chunks +21 lines, -3 lines 0 comments Download

Messages

Total messages: 12 (6 generated)
drott
4 years, 4 months ago (2016-08-03 15:06:36 UTC) #3
eae
LGTM
4 years, 4 months ago (2016-08-03 16:12:56 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2199333008/1
4 years, 4 months ago (2016-08-03 16:13:28 UTC) #8
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 4 months ago (2016-08-03 17:53:07 UTC) #9
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/3af81d9791986c5d47b261dda6f2a672e65ae313 Cr-Commit-Position: refs/heads/master@{#409564}
4 years, 4 months ago (2016-08-03 17:57:52 UTC) #11
drott
4 years, 4 months ago (2016-08-04 08:30:27 UTC) #12
Message was sent while issue was closed.
Thanks for the quick review, Emil.

Powered by Google App Engine
This is Rietveld 408576698