Skip to content

Comments

fix ascii.ValidPrint#21

Merged
achille-roussel merged 3 commits intomasterfrom
ascii-fix-valid-print
Dec 11, 2019
Merged

fix ascii.ValidPrint#21
achille-roussel merged 3 commits intomasterfrom
ascii-fix-valid-print

Conversation

@achille-roussel
Copy link
Contributor

@achille-roussel achille-roussel commented Dec 10, 2019

I noticed an off-by-one error in the function checking whether a string contains only printable ASCII characters, where we were including 0x7F instead of excluding it.

I added a test for strings containing ~ (0x7F) and fixed the code. For some reason the fix also made the function much faster on long strings it seems:

name                             old time/op    new time/op    delta
ValidPrint/12345678901234567...    1.71µs ± 2%    1.24µs ± 2%  -27.14%  (p=0.000 n=9+10)

name                             old speed      new speed      delta
ValidPrint/12345678901234567...  5.86GB/s ± 2%  8.04GB/s ± 2%  +37.25%  (p=0.000 n=9+10)

I'll follow up with an update of the JSON benchmark to verify the impact on the code using ascii.ValidPrint.

@achille-roussel
Copy link
Contributor Author

achille-roussel commented Dec 10, 2019

Short strings also seem to show some improvement actually:

name                        old time/op    new time/op    delta
ValidPrint/Hello_World!       5.07ns ± 2%    4.97ns ± 1%  -2.02%  (p=0.007 n=8+8)
ValidPrint/Hello"World!       5.13ns ± 1%    4.94ns ± 1%  -3.70%  (p=0.000 n=8+8)
ValidPrint/Hello\World!       5.17ns ± 1%    4.96ns ± 1%  -4.04%  (p=0.001 n=7+7)

name                        old speed      new speed      delta
ValidPrint/Hello_World!     2.37GB/s ± 2%  2.42GB/s ± 1%  +2.07%  (p=0.010 n=8+8)
ValidPrint/Hello"World!     2.34GB/s ± 1%  2.43GB/s ± 1%  +3.84%  (p=0.000 n=8+8)
ValidPrint/Hello\World!     2.32GB/s ± 1%  2.42GB/s ± 1%  +4.24%  (p=0.001 n=7+7)

@achille-roussel
Copy link
Contributor Author

Here's a more thorough test, shows a bit slower for short strings, much faster for long strings 🤷‍♂

name                             old time/op    new time/op    delta
ValidPrint/Hello_World!            5.12ns ± 1%    5.42ns ± 2%   +6.02%  (p=0.000 n=7+8)
ValidPrint/12345678901234567...    1.75µs ± 2%    1.24µs ± 1%  -29.08%  (p=0.000 n=8+8)

name                             old speed      new speed      delta
ValidPrint/Hello_World!          2.35GB/s ± 1%  2.21GB/s ± 2%   -5.71%  (p=0.000 n=7+8)
ValidPrint/12345678901234567...  5.73GB/s ± 2%  8.08GB/s ± 1%  +40.97%  (p=0.000 n=8+8)

I did a bit more byte shuffling and maybe got the other validation function to run a bit faster... seems significant but hard to tell if it's due to benchmark just being friendlier to the hardware in this specific case or if it's actually doing less work, we'd have to dig into CPU perf counters to see what's happening here:

name                        old time/op    new time/op     delta
Valid/Hello_World!            4.39ns ± 2%     4.02ns ± 2%   -8.24%  (p=0.000 n=8+8)
Valid/12345678901234567...    1.15µs ± 1%     0.59µs ± 1%  -48.46%  (p=0.000 n=8+8)

name                        old speed      new speed       delta
Valid/Hello_World!          2.74GB/s ± 2%   2.98GB/s ± 2%   +8.99%  (p=0.000 n=8+8)
Valid/12345678901234567...  8.70GB/s ± 1%  16.88GB/s ± 1%  +94.04%  (p=0.000 n=8+8)

Overall, the change seem to show a 3% decrease in decoding time, but also shows ~2% less memory allocations, which doesn't really make sense to me and may be the cause of the faster execution.

name                           old time/op    new time/op    delta
Marshal/*json.codeResponse2      4.21ms ± 4%    4.14ms ± 1%    ~     (p=0.065 n=8+8)
Unmarshal/*json.codeResponse2    8.07ms ± 1%    7.82ms ± 1%  -3.12%  (p=0.000 n=8+8)

name                           old speed      new speed      delta
Marshal/*json.codeResponse2     461MB/s ± 4%   468MB/s ± 1%    ~     (p=0.065 n=8+8)
Unmarshal/*json.codeResponse2   240MB/s ± 1%   248MB/s ± 1%  +3.22%  (p=0.000 n=8+8)

name                           old alloc/op   new alloc/op   delta
Marshal/*json.codeResponse2       0.00B          0.00B         ~     (all equal)
Unmarshal/*json.codeResponse2    7.40kB ± 2%    7.25kB ± 2%  -2.07%  (p=0.013 n=8+8)

name                           old allocs/op  new allocs/op  delta
Marshal/*json.codeResponse2        0.00           0.00         ~     (all equal)
Unmarshal/*json.codeResponse2      33.4 ± 2%      32.4 ± 2%  -3.00%  (p=0.009 n=8+8)

Overall this is probably a no-op change performance-wise, but we get a bug fix :)

@achille-roussel achille-roussel merged commit 56510b3 into master Dec 11, 2019
@achille-roussel achille-roussel deleted the ascii-fix-valid-print branch December 11, 2019 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants