Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefer strings.Builder over unsafebytes #8

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

diamondburned
Copy link
Contributor

This pull request removes the use of unsafe and instead prefers strings.Builder. This causes a slight performance loss during encoding and none during decoding:

# Old: unsafe
# New: strings.Builder

name          old time/op    new time/op    delta
Encode-8         276ns ±18%     292ns ± 3%  +5.61%  (p=0.010 n=15+15)
EncodeTo-8      1.87µs ± 3%    1.85µs ± 7%    ~     (p=0.056 n=13+15)
Decode-8        3.16µs ± 2%    3.18µs ± 7%    ~     (p=0.784 n=13+14)
DecodeFrom-8    3.74µs ± 4%    3.73µs ± 2%    ~     (p=0.562 n=15+13)

name          old speed      new speed      delta
Encode-8      43.0MB/s ±12%  41.1MB/s ± 3%  -4.38%  (p=0.018 n=14+15)
EncodeTo-8     175MB/s ± 3%   177MB/s ± 6%    ~     (p=0.060 n=13+15)
Decode-8       103MB/s ± 2%   103MB/s ± 6%    ~     (p=0.756 n=13+14)
DecodeFrom-8  87.4MB/s ± 4%  87.7MB/s ± 2%    ~     (p=0.563 n=15+13)

@nihaals
Copy link
Member

nihaals commented Feb 23, 2021

What's the advantage of avoiding unsafe here? Is there potential UB?

@diamondburned
Copy link
Contributor Author

No, it's just cleaner. There's just no unsafe.

@nihaals
Copy link
Member

nihaals commented Feb 23, 2021

I'm unsure, I think performance is an interesting goal, especially with such a small project. Code maintainability isn't a big issue as it would require a very odd implementation for the code to become unreadable or too spaghetti. The only required change I see in the future is #1, other than that, there shouldn't need to be any large changes to functionality or features, so I'm not sure how much of a positive difference this will have (compared to even a fairly small increase of performance).

@diamondburned
Copy link
Contributor Author

The performance loss was likely because of the preoccupied load on my laptop. A higher sample size would make up for the offset.

@nihaals
Copy link
Member

nihaals commented Feb 24, 2021

Based on the GitHub Actions log (new, previous):

Previous

BenchmarkEncode-2       	 7240875	       167 ns/op	  71.80 MB/s
BenchmarkEncodeTo-2     	  805804	      1317 ns/op	 248.26 MB/s
BenchmarkDecode-2       	  509618	      2067 ns/op	 158.24 MB/s
BenchmarkDecodeFrom-2   	  427752	      2345 ns/op	 139.46 MB/s

New

BenchmarkEncode-2       	 6624477	       174 ns/op	  68.83 MB/s
BenchmarkEncodeTo-2     	  915034	      1251 ns/op	 261.34 MB/s
BenchmarkDecode-2       	  607250	      2019 ns/op	 161.98 MB/s
BenchmarkDecodeFrom-2   	  564886	      2089 ns/op	 156.51 MB/s

This looks like random variation so I agree, doesn't seem to be a real impact on performance

@nihaals nihaals merged commit dff5550 into bottom-software-foundation:main Feb 24, 2021
@nihaals
Copy link
Member

nihaals commented Feb 24, 2021

Thanks

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