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

test(bench): improve go-faster/jx benchmark #1

Merged
merged 1 commit into from
Jan 10, 2022

Conversation

ernado
Copy link
Contributor

@ernado ernado commented Jan 10, 2022

  • Use ObjBytes to avoid key allocations
  • Use Skip instead of parsing
  • Avoid unrelated allocation (add strToBytes)
name                                     old time/op    new time/op    delta
CalcStats/gofaster-jx/tiny-32               237ns ± 2%      60ns ± 1%   -74.77%  (p=0.008 n=5+5)
CalcStats/gofaster-jx/small-32             1.28µs ± 9%    0.88µs ± 4%   -30.85%  (p=0.008 n=5+5)
CalcStats/gofaster-jx/large-32              151ms ± 1%      41ms ± 1%   -72.85%  (p=0.008 n=5+5)
CalcStats/gofaster-jx_withpath/tiny-32      296ns ± 3%      74ns ± 2%   -75.08%  (p=0.008 n=5+5)
CalcStats/gofaster-jx_withpath/small-32    2.10µs ± 6%    1.53µs ± 6%   -26.91%  (p=0.008 n=5+5)
CalcStats/gofaster-jx_withpath/large-32     199ms ± 1%     164ms ± 2%   -17.66%  (p=0.008 n=5+5)

name                                     old alloc/op   new alloc/op   delta
CalcStats/gofaster-jx/tiny-32               40.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)
CalcStats/gofaster-jx/small-32              80.0B ± 0%     16.0B ± 0%   -80.00%  (p=0.008 n=5+5)
CalcStats/gofaster-jx/large-32             35.2MB ± 0%     0.0MB ± 7%  -100.00%  (p=0.008 n=5+5)
CalcStats/gofaster-jx_withpath/tiny-32      40.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)
CalcStats/gofaster-jx_withpath/small-32      144B ± 0%       80B ± 0%   -44.44%  (p=0.008 n=5+5)
CalcStats/gofaster-jx_withpath/large-32    57.9MB ± 0%    52.3MB ± 0%    -9.67%  (p=0.008 n=5+5)

name                                     old allocs/op  new allocs/op  delta
CalcStats/gofaster-jx/tiny-32                2.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
CalcStats/gofaster-jx/small-32               8.00 ± 0%      2.00 ± 0%   -75.00%  (p=0.008 n=5+5)
CalcStats/gofaster-jx/large-32              1.12M ± 0%     0.00M       -100.00%  (p=0.008 n=5+5)
CalcStats/gofaster-jx_withpath/tiny-32       2.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
CalcStats/gofaster-jx_withpath/small-32      17.0 ± 0%      13.0 ± 0%   -23.53%  (p=0.008 n=5+5)
CalcStats/gofaster-jx_withpath/large-32     1.77M ± 0%     1.54M ± 0%   -12.53%  (p=0.008 n=5+5)

* Use ObjBytes to avoid key allocations
* Use Skip instead of parsing
* Avoid unrelated allocation (add strToBytes)
@@ -232,15 +234,20 @@ func CalcStatsJsoniterWithPath(str string) (s Stats) {
return
}

// strToBytes returns byte slice from string without allocations.
func strToBytes(s string) []byte {
return unsafe.Slice((*byte)(unsafe.Pointer((*reflect.StringHeader)(unsafe.Pointer(&s)).Data)), len(s))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally get your point and it sure is a bit unfair to have the extra overhead because of a different input type but I'm still not sure how I feel about introducing unsafe to the party since usually it's not welcome in production code 😕

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this library is focusing on processing strings - it's fair to compare only legal and safe methods of use for the context of this library only. If any user of a library requires performance at the cost of the risks of using unsafe then it's his/her responsibility to do so.

However, we can let it pass under the assumption that the user of go-faster/jx is willing to pay that price (which many folks often do) just in order to eliminate the overhead for a fair comparison's sake.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of a background:

I actually started out with unsafe but quickly encountered a problem on my M1 Max macOS 12.1 Go 1.17.6. Seemingly random bugs started to appear after converting string to []byte using the unsafe method from valyala/fasthttp. I couldn't reproduce them neither on another Intel i7 macOS 12.1 Go 1.17.6 MBP, nor on go.dev/play. Only later did I realize that it's me who did the stupid mistake of using an unsafe []byte -> string method for string -> []byte conversion, which, IMHO, only proves the dangers and cost of utilizing unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is unsafe, but not sure how to compare libraries fairly, because string -> byte allocation is unrelated.

If you will find another way I'm ok with it!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's unrelated and slightly falsifies the results.
The solution is to just provide a new function:

func Scan(o Options, s []byte, fn func(*IteratorBytes) (err bool)) Error

This will be the most widely used function anyway.

@@ -294,15 +298,15 @@ func CalcStatsGofasterJx(str string) (s Stats) {
}
case gofasterjx.Object:
s.TotalObjects++
if err := d.Obj(func(d *gofasterjx.Decoder, key string) error {
if err := d.ObjBytes(func(d *gofasterjx.Decoder, key []byte) error {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip!

@romshark romshark merged commit a15f887 into romshark:main Jan 10, 2022
@ernado ernado deleted the test/bench-improbe-jx branch January 11, 2022 16:00
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.

None yet

2 participants