Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(753)

Issue 6438045: Avoid a loop in writeString and writePad by zeroing padding first. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 4 months ago by Leon
Modified:
12 years, 4 months ago
Reviewers:
reed1
CC:
skia-review_googlegroups.com
Base URL:
https://skia.googlecode.com/svn/trunk
Visibility:
Public.

Description

Avoid a loop in writeString and writePad by zeroing padding first. Also add a benchmark to time the new improved writeString. Before my change the bench took ~1.23ms and afterwards it takes ~.95ms. Add some testing to ensure that writePad works properly. TEST=Writer32Test, WriterBench Committed: https://code.google.com/p/skia/source/detail?r=4742

Patch Set 1 #

Total comments: 2

Patch Set 2 : Improved some comments, modified WriterBench to cache strLen result. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+111 lines, -13 lines) Patch
A bench/WriterBench.cpp View 1 1 chunk +40 lines, -0 lines 0 comments Download
M gyp/bench.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M src/core/SkWriter32.cpp View 1 2 chunks +15 lines, -13 lines 0 comments Download
M tests/Writer32Test.cpp View 6 chunks +55 lines, -0 lines 0 comments Download

Messages

Total messages: 3
Leon
12 years, 4 months ago (2012-07-24 18:58:18 UTC) #1
reed1
nice change/addition overall. minor nit on strlen caching. other than that, lgtm https://codereview.appspot.com/6438045/diff/1/bench/WriterBench.cpp File bench/WriterBench.cpp ...
12 years, 4 months ago (2012-07-24 19:22:58 UTC) #2
Leon
12 years, 4 months ago (2012-07-24 20:43:42 UTC) #3
https://codereview.appspot.com/6438045/diff/1/bench/WriterBench.cpp
File bench/WriterBench.cpp (right):

https://codereview.appspot.com/6438045/diff/1/bench/WriterBench.cpp#newcode25
bench/WriterBench.cpp:25: for (int i = 0; i < SkBENCHLOOP(800); i++) {
On 2012/07/24 19:22:59, reed1 wrote:
> cache len outside of the loop?

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b