-
Notifications
You must be signed in to change notification settings - Fork 141
Use make instead of append to reduce memory usage #23
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
=========================================
+ Coverage 92.78% 92.8% +0.02%
=========================================
Files 8 8
Lines 970 973 +3
=========================================
+ Hits 900 903 +3
Misses 48 48
Partials 22 22
Continue to review full report at Codecov.
|
@@ -64,7 +64,11 @@ func (c *cache) getValue() *Value { | |||
if cap(c.vs) > len(c.vs) { | |||
c.vs = c.vs[:len(c.vs)+1] | |||
} else { | |||
c.vs = append(c.vs, Value{}) | |||
if len(c.vs) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
append
already does something similar underneath - see https://play.golang.org/p/wFzqBZvEXPl . If append
works slower than this code, then a bug must be filled at https://github.com/golang/go/issues
Uh I didn't notice this line. I thought the patch is for performance optimization, not a memory optimization. Then it looks valid. Could you add a benchmark with |
In fact, this will also improve performance by avoiding the copy. The benchmarks are here: https://gist.github.com/hewenyang/91618e54342f65809f4cb3c9c3aa1240 |
@valyala can this be merged? |
Since the pointers to original values are always held, it doesn't make sense to use append() and keep the original values anyway.
I expect this change to reduce memory consumption by about 30% for large JSON values and avoid copying.