Optimise ReadString() and WriteString() #574

saurabhagrawal-86 · 2023-05-23T07:25:05Z

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)

sywhang

This looks good to me.

@r-hang PTAL.

r-hang

Logic looks good, I left a question about the structure of the benchmark.

Since this change concerns a go1.20 feature, I think we should merge this after #575 goes to dev.

protocol/binary/stream_reader_test.go

r-hang

lgtm! I think you'll need to rebase to pull in the contents of #575.

CLAassistant · 2023-05-30T04:56:28Z

All committers have signed the CLA.

sywhang · 2023-05-30T04:59:07Z

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

r-hang · 2023-05-30T05:11:15Z

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

sywhang · 2023-05-30T05:12:38Z

@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.

After rebasing #574 on top of #577 and running GO111MODULE=on make lint I still see some errors. cmd/thriftbreak/main_test.go:26:2: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (SA1019) gen/string.go:66:15: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead. (SA1019) After this change: $ git log --oneline a054ce6 (HEAD -> optimise_read_string) Fix remaining lint errors 2c1d285 build tags 8a1aa7d address review comments f26170a inline 41dacbd remove bench reports 4ad4d9c optimise write string fb22d6e optimise readstring() 6268dfe (origin/dev, origin/HEAD) Remove all usages of io/ioutil (#577) $ GO111MODULE=on make lint Checking gofmt Checking govet Checking golint Checking staticcheck $ echo $? 0

After rebasing #574 on top of #577 and running GO111MODULE=on make lint I still see some errors. cmd/thriftbreak/main_test.go:26:2: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (SA1019) gen/string.go:66:15: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead. (SA1019) After this change: $ git log --oneline a054ce6 (HEAD -> optimise_read_string) Fix remaining lint errors 2c1d285 build tags 8a1aa7d address review comments f26170a inline 41dacbd remove bench reports 4ad4d9c optimise write string fb22d6e optimise readstring() 6268dfe (origin/dev, origin/HEAD) Remove all usages of io/ioutil (#577) $ GO111MODULE=on make lint Checking gofmt Checking govet Checking golint Checking staticcheck $ echo $? 0 I've cherry-picked the contents of the HEAD commit displayed above into a separate branch for this PR to dev.

r-hang · 2023-05-30T23:48:38Z

@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.

prashantv · 2023-05-31T04:10:02Z

protocol/binary/string_post_go120.go

+// ReadString reads a Thrift encoded string.
+func (sr *StreamReader) ReadString() (string, error) {
+	bs, err := sr.ReadBinary()
+	return unsafe.String(unsafe.SliceData(bs), len(bs)), err


As the name implies, any use of unsafe should be used very carefully. I'd strongly recommend adding a comment that indicates what assumptions are being made and why this is safe.

In this case, bs returned from ReadBinary has no mutable references (it creates a copy in that method), so it seems safe.

An alternate implementation which avoids unsafe by using strings.Builder (which internally uses unsafe) would also work, and avoiding separate implementations by version

var b strings.Builder io.Copy(&b, sr.reader, length) return b.String()

Thanks for the suggestion. I've added a comment explaining why "unsafe" is safe to use here.

In your alternate implementation, I'm assuming you meant io.CopyN (not io.Copy). If yes, that ends up being even slower than the original implementation.

When copying from a bytes.Buffer to a strings.Builder, I understand that io.Copy would avoid an allocation and a copy. However, io.CopyN is unable to make those optimisations and allocates a staging area for the copy operation. That ends up outweighing the efficiency of the strings.Builder's String() function.

benchmark results are here - saurabhagrawal-86#1

@prashantv, the benchmark results and evaluation of io.CopyN looks fine to me. We'll wait for your follow up!

codecov · 2023-05-31T17:26:49Z

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%)`	⬇️
protocol/binary/stream_writer.go	`67.21% <ø> (+1.05%)`	⬆️
protocol/binary/string_post_go120.go	`100.00% <100.00%> (ø)`
protocol/binary/string_pre_go120.go	`100.00% <100.00%> (ø)`

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

protocol/binary_test.go

protocol/binary/stream_reader_test.go

protocol/binary_test.go

protocol/binary/string_post_go120.go

saurabhagrawal-86 changed the title ~~Optimise ReadString()~~ Optimise ReadString() and WriteString() May 23, 2023

r-hang self-requested a review May 26, 2023 04:29

sywhang approved these changes May 29, 2023

View reviewed changes

r-hang reviewed May 29, 2023

View reviewed changes

protocol/binary/stream_reader_test.go Outdated Show resolved Hide resolved

r-hang approved these changes May 30, 2023

View reviewed changes

saurabhagrawal-86 force-pushed the optimise_read_string branch from 2591bc8 to 8f1f31b Compare May 30, 2023 05:29

r-hang mentioned this pull request May 30, 2023

Fix remaining lint errors #578

Merged

saurabhagrawal-86 added 6 commits May 31, 2023 03:58

optimise readstring()

d2772e2

optimise write string

9d1406b

remove bench reports

2642659

inline

29e6bd8

address review comments

8360a80

build tags

1a079b6

saurabhagrawal-86 force-pushed the optimise_read_string branch from 4d27df8 to 1a079b6 Compare May 31, 2023 04:12

prashantv reviewed May 31, 2023

View reviewed changes

add comment about unsafe

5fc3def

saurabhagrawal-86 added 2 commits June 1, 2023 07:23

test writestring failure

1797518

fix lint error

04da60a

r-hang reviewed Jun 4, 2023

View reviewed changes

protocol/binary_test.go Outdated Show resolved Hide resolved

protocol/binary/stream_reader_test.go Outdated Show resolved Hide resolved

protocol/binary_test.go Outdated Show resolved Hide resolved

protocol/binary_test.go Show resolved Hide resolved

saurabhagrawal-86 added 2 commits June 5, 2023 04:28

remove debug print stmnt

9928f89

remove unnecessary message from require

558c4e1

r-hang reviewed Jun 5, 2023

View reviewed changes

protocol/binary/string_post_go120.go Outdated Show resolved Hide resolved

saurabhagrawal-86 added 3 commits June 6, 2023 04:14

comment on unsafe usage in write path

7779403

fix typo:

0400ce4

remove launch.json

057a4c6

r-hang merged commit e4f0c88 into thriftrw:dev Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimise ReadString() and WriteString() #574

Optimise ReadString() and WriteString() #574

saurabhagrawal-86 commented May 23, 2023 •

edited

Loading

sywhang left a comment

r-hang left a comment

r-hang left a comment •

edited

Loading

CLAassistant commented May 30, 2023 •

edited

Loading

sywhang commented May 30, 2023

r-hang commented May 30, 2023

sywhang commented May 30, 2023

r-hang commented May 30, 2023

prashantv May 31, 2023

prashantv May 31, 2023

saurabhagrawal-86 May 31, 2023

saurabhagrawal-86 Jun 1, 2023

r-hang Jun 1, 2023

codecov bot commented May 31, 2023 •

edited

Loading

Optimise ReadString() and WriteString() #574

Optimise ReadString() and WriteString() #574

Conversation

saurabhagrawal-86 commented May 23, 2023 • edited Loading

sywhang left a comment

Choose a reason for hiding this comment

r-hang left a comment

Choose a reason for hiding this comment

r-hang left a comment • edited Loading

Choose a reason for hiding this comment

CLAassistant commented May 30, 2023 • edited Loading

sywhang commented May 30, 2023

r-hang commented May 30, 2023

sywhang commented May 30, 2023

r-hang commented May 30, 2023

prashantv May 31, 2023

Choose a reason for hiding this comment

prashantv May 31, 2023

Choose a reason for hiding this comment

saurabhagrawal-86 May 31, 2023

Choose a reason for hiding this comment

saurabhagrawal-86 Jun 1, 2023

Choose a reason for hiding this comment

r-hang Jun 1, 2023

Choose a reason for hiding this comment

codecov bot commented May 31, 2023 • edited Loading

Codecov Report

saurabhagrawal-86 commented May 23, 2023 •

edited

Loading

r-hang left a comment •

edited

Loading

CLAassistant commented May 30, 2023 •

edited

Loading

codecov bot commented May 31, 2023 •

edited

Loading