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

Update all fmt.Errorf occurrences with errors.New using either fmt.Sprintf or strings.Join #454

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jarri-abidi
Copy link

fixes issue #426

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #454 into master will decrease coverage by 0.02%.
The diff coverage is 74.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #454      +/-   ##
==========================================
- Coverage   86.67%   86.64%   -0.03%     
==========================================
  Files          41       41              
  Lines        5104     5108       +4     
==========================================
+ Hits         4424     4426       +2     
- Misses        540      542       +2     
  Partials      140      140              
Impacted Files Coverage Δ
any_str.go 96.59% <0.00%> (ø)
reflect_map.go 87.86% <0.00%> (ø)
any.go 80.20% <50.00%> (ø)
reflect_array.go 77.58% <50.00%> (ø)
reflect_slice.go 96.61% <50.00%> (ø)
stream_float.go 91.17% <50.00%> (ø)
any_invalid.go 81.08% <66.66%> (ø)
reflect_struct_encoder.go 84.03% <66.66%> (ø)
iter.go 90.09% <100.00%> (ø)
reflect.go 93.00% <100.00%> (+0.14%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53b9d06...d4b7061. Read the comment docs.

@jarri-abidi
Copy link
Author

Could anyone review this?
I'm not sure if tests for these changes would be overkill.
Nevertheless I did add some benchmarks as justification for the changes.

@AllenX2018
Copy link
Collaborator

Thanks for the patch. I'm not familiar with fmt package and could you pls explain why using errors.New(fmt.Sprintf()) would decrease the heap usage other than using fmt.Errorf()? Besides, I understand that you add that benchmark test as justification but I don't think its a good idea to put it into json-iterator.It's good enough to just put it in the pr's description.

@jarri-abidi
Copy link
Author

jarri-abidi commented Apr 16, 2020

So if we take a look at the benchmarks below taken from the added tests, we can see that fmt.Errorf() on its own uses 40 B/op and 3 allocs/op.
errors.New(fmt.Sprintf()) on the other hand uses 24 B/op and 2 allocs/op.
It's also faster in terms of execution time.

152 ns/op 40 B/op 3 allocs/op
119 ns/op 24 B/op 2 allocs/op

In addition to that, where possible, I have replaced fmt.Errorf() with a strings.join() approach that is much faster than either of the above mentioned approaches.

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