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

Issue 1683883002: Add request cache to SkFontHost_fontconfig. (Closed)

Created:
4 years, 10 months ago by bungeman-skia
Modified:
4 years, 10 months ago
Reviewers:
mtklein, kochi, reed1
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Add request cache to SkFontHost_fontconfig. The current code deduplicates SkTypeface instances as all font lookups should for better use of the glyph cache. This adds a request cache as well, so that repeated recent requests will return the cached result instead of doing a full lookup. BUG=chromium:424082, chromium:444894 Committed: https://skia.googlesource.com/skia/+/70bb80802198b7fedee58f79d0d68d5f8aba8b62

Patch Set 1 #

Total comments: 16

Patch Set 2 : Address comments. #

Patch Set 3 : Use SkResourceCache instead. #

Total comments: 2

Patch Set 4 : Fix deallocation. #

Patch Set 5 : Clean up, assert the right things. #

Patch Set 6 : 'this' is already sufficiently aligned, just check length. #

Patch Set 7 : No longer need 'memory' include. #

Total comments: 2

Patch Set 8 : Change to 'size', clarify comments. #

Total comments: 2

Patch Set 9 : Make method 'const'. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+135 lines, -52 lines) Patch
M src/core/SkResourceCache.h View 1 2 3 4 5 6 7 8 1 chunk +12 lines, -8 lines 0 comments Download
M src/core/SkResourceCache.cpp View 1 2 3 4 5 6 7 2 chunks +3 lines, -3 lines 0 comments Download
M src/ports/SkFontHost_fontconfig.cpp View 1 2 3 4 5 6 7 2 chunks +120 lines, -41 lines 0 comments Download

Messages

Total messages: 56 (28 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683883002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683883002/1
4 years, 10 months ago (2016-02-09 22:42:28 UTC) #4
bungeman-skia
This is an alternative to https://crrev.com/838743002 .
4 years, 10 months ago (2016-02-09 22:47:06 UTC) #6
kochi
lgtm The performance with this fix is quite nice. Thanks for working on this.
4 years, 10 months ago (2016-02-10 05:44:22 UTC) #8
mtklein
https://codereview.chromium.org/1683883002/diff/1/src/ports/SkFontHost_fontconfig.cpp File src/ports/SkFontHost_fontconfig.cpp (left): https://codereview.chromium.org/1683883002/diff/1/src/ports/SkFontHost_fontconfig.cpp#oldcode95 src/ports/SkFontHost_fontconfig.cpp:95: // Check if requested NameStyle is in the NameStyle ...
4 years, 10 months ago (2016-02-10 16:14:10 UTC) #9
bungeman-skia
After having written all that... lets try something completely different. https://codereview.chromium.org/1683883002/diff/1/src/ports/SkFontHost_fontconfig.cpp File src/ports/SkFontHost_fontconfig.cpp (left): https://codereview.chromium.org/1683883002/diff/1/src/ports/SkFontHost_fontconfig.cpp#oldcode95 ...
4 years, 10 months ago (2016-02-11 17:46:02 UTC) #11
bungeman-skia
https://codereview.chromium.org/1683883002/diff/40001/src/ports/SkFontHost_fontconfig.cpp File src/ports/SkFontHost_fontconfig.cpp (right): https://codereview.chromium.org/1683883002/diff/40001/src/ports/SkFontHost_fontconfig.cpp#newcode96 src/ports/SkFontHost_fontconfig.cpp:96: char* storage = new char [sizeof(Request) + SkAlign4(nameLen)]; The ...
4 years, 10 months ago (2016-02-11 18:23:36 UTC) #12
bungeman-skia
https://codereview.chromium.org/1683883002/diff/40001/src/ports/SkFontHost_fontconfig.cpp File src/ports/SkFontHost_fontconfig.cpp (right): https://codereview.chromium.org/1683883002/diff/40001/src/ports/SkFontHost_fontconfig.cpp#newcode96 src/ports/SkFontHost_fontconfig.cpp:96: char* storage = new char [sizeof(Request) + SkAlign4(nameLen)]; On ...
4 years, 10 months ago (2016-02-11 19:28:24 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683883002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683883002/60001
4 years, 10 months ago (2016-02-11 20:52:38 UTC) #16
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-11 21:23:17 UTC) #18
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683883002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683883002/80001
4 years, 10 months ago (2016-02-12 22:54:22 UTC) #21
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x86_64-Debug-Trybot/builds/6167)
4 years, 10 months ago (2016-02-12 22:58:24 UTC) #23
bungeman-skia
On 2016/02/12 22:58:24, commit-bot: I haz the power wrote: > Dry run: Try jobs failed ...
4 years, 10 months ago (2016-02-13 02:04:45 UTC) #24
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683883002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683883002/100001
4 years, 10 months ago (2016-02-16 16:29:36 UTC) #26
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-16 16:52:12 UTC) #29
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683883002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683883002/120001
4 years, 10 months ago (2016-02-16 19:22:37 UTC) #33
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-16 19:59:36 UTC) #35
bungeman-skia
This should now be ready for review. This fixes an immediate need of Chromium, and ...
4 years, 10 months ago (2016-02-16 20:11:47 UTC) #36
kochi
confirmed this fixes the issue. I'd defer real code review to skia experts.
4 years, 10 months ago (2016-02-17 04:42:58 UTC) #37
reed1
https://codereview.chromium.org/1683883002/diff/120001/src/core/SkResourceCache.h File src/core/SkResourceCache.h (right): https://codereview.chromium.org/1683883002/diff/120001/src/core/SkResourceCache.h#newcode39 src/core/SkResourceCache.h:39: size_t length() { size_t size() const { ...
4 years, 10 months ago (2016-02-17 14:12:52 UTC) #38
reed1
with discardable memory, it is harder to cache small things. How wil this get purged?
4 years, 10 months ago (2016-02-17 14:14:41 UTC) #39
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683883002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683883002/140001
4 years, 10 months ago (2016-02-17 16:43:47 UTC) #43
bungeman-skia
> with discardable memory, it is harder to cache small things. How wil this > ...
4 years, 10 months ago (2016-02-17 16:55:36 UTC) #44
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-17 17:10:43 UTC) #46
reed1
lgtm w/ const-nit thanks for the explanation about a separate instance. https://codereview.chromium.org/1683883002/diff/140001/src/core/SkResourceCache.h File src/core/SkResourceCache.h (right): ...
4 years, 10 months ago (2016-02-17 17:13:39 UTC) #47
bungeman-skia
https://codereview.chromium.org/1683883002/diff/140001/src/core/SkResourceCache.h File src/core/SkResourceCache.h (right): https://codereview.chromium.org/1683883002/diff/140001/src/core/SkResourceCache.h#newcode41 src/core/SkResourceCache.h:41: size_t size() { On 2016/02/17 17:13:39, reed1 wrote: > ...
4 years, 10 months ago (2016-02-17 17:19:41 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683883002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683883002/160001
4 years, 10 months ago (2016-02-17 17:45:23 UTC) #53
commit-bot: I haz the power
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/70bb80802198b7fedee58f79d0d68d5f8aba8b62
4 years, 10 months ago (2016-02-17 18:13:53 UTC) #55
kochi
4 years, 10 months ago (2016-02-18 01:44:24 UTC) #56
Message was sent while issue was closed.
You're my hero!

Powered by Google App Engine
This is Rietveld 408576698