thriftrw / thriftrw-go

A Thrift encoding code generator and library for Go
MIT License
100 stars 53 forks source link

Optimise ReadString() and WriteString() #574

Closed saurabhagrawal-86 closed 1 year ago

saurabhagrawal-86 commented 1 year ago

Optimise StreamReader's ReadString CPU by 35% and memory by 50% using the new String and SliceData functions introduced in the unsafe package in go 1.20

goos: linux
goarch: amd64
pkg: go.uber.org/thriftrw/protocol/binary
cpu: AMD EPYC 7B13
             │ bench_string_dev.out │        bench_string_opt.out         │
             │        sec/op        │   sec/op     vs base                │
ReadString              81.41n ± 2%   53.61n ± 3%  -34.14% (p=0.000 n=10)
ReadString-2            85.67n ± 6%   55.75n ± 6%  -34.92% (p=0.000 n=10)
ReadString-4            87.71n ± 5%   57.14n ± 5%  -34.86% (p=0.000 n=10)
geomean                 84.89n        55.48n       -34.64%

             │ bench_string_dev.out │        bench_string_opt.out        │
             │         B/op         │    B/op     vs base                │
ReadString               96.00 ± 0%   48.00 ± 0%  -50.00% (p=0.000 n=10)
ReadString-2             96.00 ± 0%   48.00 ± 0%  -50.00% (p=0.000 n=10)
ReadString-4             96.00 ± 0%   48.00 ± 0%  -50.00% (p=0.000 n=10)
geomean                  96.00        48.00       -50.00%

             │ bench_string_dev.out │        bench_string_opt.out        │
             │      allocs/op       │ allocs/op   vs base                │
ReadString               2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.000 n=10)
ReadString-2             2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.000 n=10)
ReadString-4             2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.000 n=10)
geomean                  2.000        1.000       -50.00%

Similar changes to optimise StreamWriter's WriteString() method.

name           old time/op    new time/op    delta
WriteString      21.2ns ± 3%    13.3ns ± 2%  -37.48%  (p=0.000 n=10+9)
WriteString-2    20.8ns ± 2%    13.4ns ± 4%  -35.42%  (p=0.000 n=9+10)
WriteString-4    20.8ns ± 2%    13.4ns ± 8%  -35.86%  (p=0.000 n=9+10)
CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

sywhang commented 1 year ago

@r-hang i think you pulled in the changes incorrectly to this branch.

r-hang commented 1 year ago

@sywhang, the author reached out after their rebase. I'm working with them now.

sywhang commented 1 year ago

@r-hang mb, apologies for assuming :p saw your commits show up all of a sudden so I assumed you pushed to the branch. Thanks.

r-hang commented 1 year ago

@saurabhagrawal-86 we've updated dev again to resolve some outstanding staticcheck issues. Could you rebase again and see if the remote go1.19 and go1.20 builds pass? Your change built for me locally when I last tested it out once rebased on dev.

codecov[bot] commented 1 year ago

Codecov Report

Merging #574 (057a4c6) into dev (1e96648) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #574      +/-   ##
==========================================
+ Coverage   68.00%   68.01%   +0.01%     
==========================================
  Files         140      142       +2     
  Lines       23872    23878       +6     
==========================================
+ Hits        16233    16241       +8     
+ Misses       4578     4577       -1     
+ Partials     3061     3060       -1     
Impacted Files Coverage Δ
protocol/binary/stream_reader.go 95.97% <ø> (-0.05%) :arrow_down:
protocol/binary/stream_writer.go 67.21% <ø> (+1.05%) :arrow_up:
protocol/binary/string_post_go120.go 100.00% <100.00%> (ø)
protocol/binary/string_pre_go120.go 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more