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

Issue 1144023002: Move font loading in gm tests and benches out of constructors (Closed)

Created:
5 years, 7 months ago by Kimmo Kinnunen
Modified:
5 years, 7 months ago
Reviewers:
mtklein
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Move font loading in gm tests and benches out of constructors Constructing the gm tests and benches causes many calls to font loads. This is visible as profiling samples in fontconfig and freetype on Linux for all profiling runs of nanobench. This complicates analysis of test-cases that are suspected of being slow due to font-related issues. Move the font loading to GM::onOnceBeforeDraw and Benchmark::onPreDraw. This way the code is not executed if the testcase does not match the nanobench --match filter. This way the samples in font-related code are more easy to identify as legitimate occurances caused by the testcase. This should not cause differences in timings, because: * Benchmark::preDraw / onPreDraw is defined to be run outside the timer * GM::runAsBench is not enabled for any of the modified testcases. Also nanobench untimed warmup round should run the onOnceBeforeDraw. (and there are other GM::runAsBench gms already doing loading in onOnceBeforeDraw). Changes the behavior: In TextBench: Before, the test would report two different gms with the same name if the color emoji font was not loaded successfully. After, the test always reports all tests as individual names. Generally: The errors from loading fonts now print inbetween each testcase, as opposed to printing during construction phase. Sample output: ( 143/145 MB 1872) 14.7ms 8888 gm quadclosepathResource /fonts/Funkster.ttf not a valid font. ( 160/160 MB 1831) 575µs 8888 gm surfacenewResource /fonts/Funkster.ttf not a valid font. ( 163/165 MB 1816) 12.5ms 8888 gm linepathResource /fonts/Funkster.ttf not a valid font. ( 263/411 MB 1493) 118ms 8888 gm typefacestyles_kerningResource /fonts/Funkster.ttf not a valid font. ( 374/411 MB 1231) 7.16ms 565 gm getpostextpathResource /fonts/Funkster.ttf not a valid font. ( 323/411 MB 1179) 4.92ms 565 gm stringartResource /fonts/Funkster.ttf not a valid font. ( 347/493 MB 917) 191ms 565 gm patch_gridResource /fonts/Funkster.ttf not a valid font. ( 375/493 MB 857) 23.9ms gpu gm clipdrawdrawCannot render path (0) ( 393/493 MB 706) 2.91ms unit test ParsePath------ png error IEND: CRC error ( 394/493 MB 584) 166ms gpu gm hairmodesResource /fonts/Funkster.ttf not a valid font. Resource /fonts/Funkster.ttf not a valid font. Resource /fonts/Funkster.ttf not a valid font. ... Committed: https://skia.googlesource.com/skia/+/b4a797f3aa8c10387f01cf51a65dd1a8aa5eec9d

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -61 lines) Patch
M bench/TextBench.cpp View 3 chunks +18 lines, -16 lines 1 comment Download
M bench/TextBlobBench.cpp View 1 2 2 chunks +6 lines, -2 lines 0 comments Download
M gm/colortype.cpp View 1 2 2 chunks +10 lines, -6 lines 0 comments Download
M gm/colortypexfermode.cpp View 3 chunks +13 lines, -12 lines 0 comments Download
M gm/textblob.cpp View 2 chunks +10 lines, -6 lines 0 comments Download
M gm/typeface.cpp View 1 2 3 chunks +24 lines, -13 lines 1 comment Download
M gm/verttext2.cpp View 1 2 2 chunks +9 lines, -6 lines 0 comments Download

Messages

Total messages: 6 (2 generated)
Kimmo Kinnunen
5 years, 7 months ago (2015-05-21 06:44:37 UTC) #2
mtklein
lgtm https://codereview.chromium.org/1144023002/diff/40001/bench/TextBench.cpp File bench/TextBench.cpp (right): https://codereview.chromium.org/1144023002/diff/40001/bench/TextBench.cpp#newcode94 bench/TextBench.cpp:94: fName.printf("text_%g", SkScalarToFloat(fPaint.getTextSize())); Kind of weird that this builds ...
5 years, 7 months ago (2015-05-21 13:09:26 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1144023002/40001
5 years, 7 months ago (2015-05-21 13:09:33 UTC) #5
commit-bot: I haz the power
5 years, 7 months ago (2015-05-21 13:15:33 UTC) #6
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/b4a797f3aa8c10387f01cf51a65dd1a8aa5eec9d

Powered by Google App Engine
This is Rietveld 408576698