rpc_util: Reuse memory buffer for receiving message #5862
Conversation
|
|
hueypark
force-pushed
the
master
branch
5 times, most recently
from
d198e0a
to
9f84355
Compare
|
Could you please explain what problem you are trying to solve by making this change? Maybe we can have a discussion and see if the current approach is the way to go or if a different approach would serve better.
Do you have any data to show that this change improves performance? And if so, could you please share that with us. Also, we do have a benchmark suite that you can use to run your changes against. Thanks. |
|
I implemented a feature stream counts at the benchmarkmain and ran my workloads. And I have attached all of the benchmark results. cpuProfAfter.zip |
What does this feature do?
This zip file contains a single file and I don't know how to view the results in there. The benchmarks results that you have posted here are for a very specific case. It uses the following settings among others: We would really like to see more comprehensive benchmark runs for a code change which is as fundamental as yours. Also, a benchmark run time of |
|
Also, could you please explain why you want to make the change that you are making? Are you seeing some performance bottlenecks when running specific workloads? |
This feature allows the stream API to send multiple streams.
I apologize. It would probably be better to reproduce the issue locally.
I aggred. I added a benchmark run time of 1m. result: commands: |
I am developing an OLAP database and our map reduce framework relies on gRPC to send and receive large amounts of data. Recently, we discovered that gRPC is allocating unnecessary memory for the stream API, which is causing our system to slow down. To improve performance, we want to reduce the memory allocation. Actual code link: |
easwars
added
Type: Performance
|
Could you please send our your changes to the benchmarking code as a separate PR. Thanks. |
|
Done with #5898. |
|
Thank you for this PR! I'm testing this patch in our dev environment. Description mentions "streaming rpc", but code to get buffer from the pool is also hit for unary methods, and experiment confirms that. It wasn't clear to me whether this would work for unary methods too. Perhaps the description could clarify that? Update: Summary of my findings:
|
| type SharedBufferPool interface { | ||
| // Get returns a buffer with specified length from the pool. | ||
| // | ||
| // The returned byte slice may be not zero initialized. | ||
| Get(length int) []byte | ||
|
|
||
| // Put returns a buffer to the pool. | ||
| Put(*[]byte) | ||
| } |
There was a problem hiding this comment.
I find it strange and inconvenient that this interface gives you a []byte but requires a *[]byte to put it back. This forces the caller to allocate a pointer to put elements back to the pool, which slightly defies the purpose of having a pool.
I think that it should either:
- return a
*[]bytefromGet(int) *[]byte: which might be inconvenient because the caller may not have a place where that pointer would be stored, so it would be forced to allocate a new pointer to put the slice back. - accept a
[]byteinPut([]byte)and let the specificSharedBufferPooldeal with the situation (example).
There's no need to use pointers to the slice, except because the specific implementation below (just like all of them as today) uses sync.Pool which can only hold interface{} values. However, we can expect a generic version of sync.Pool to come in the next versions of go, which would allow us to solve this implementation detail, hence I wouldn't build the exported contract on that.
So, I'd vote for consistency on Get(int) []byte and Put([]byte).
There was a problem hiding this comment.
@hueypark : What are your thoughts on this?
There was a problem hiding this comment.
I made commit 1f4bc35 for this.
accept a []byte in Put([]byte) and let the specific SharedBufferPool deal with the situation (prometheus/prometheus#12189).
There was a problem hiding this comment.
I've reverted change 1f4bc35 due to the argument should be pointer-like to avoid allocations (SA6002).
There was a problem hiding this comment.
Currently, I don't perceive any practical benefits, so it seems most sensible to maintain the current status.
In future, should a feature like generic slice sync.Pool become available, the Put([]byte) could be beneficial.
Perhaps we can consider it after golang/go#47657 (comment).
There was a problem hiding this comment.
Note that by reverting that, you're fixing the linter's complaint, but not the underlying issue: you just hid the issue behind and interface call so now the linter can't see it, but it still causes an extra allocation there.
There was a problem hiding this comment.
Thank you for pointing out. I acknowledge it's not a true fix. We'll need to revisit this for a more effective solution later.
shared_buffer_pool.go
Outdated
| } | ||
|
|
||
| func (nopBufferPool) Put(*[]byte) { | ||
|
|
| type SharedBufferPool interface { | ||
| // Get returns a buffer with specified length from the pool. | ||
| // | ||
| // The returned byte slice may be not zero initialized. | ||
| Get(length int) []byte | ||
|
|
||
| // Put returns a buffer to the pool. | ||
| Put(*[]byte) | ||
| } |
There was a problem hiding this comment.
@hueypark : What are your thoughts on this?
|
Also, looks like there are some merge conflicts in the benchmark files. I would be OK to move them to separate PR as well, if that makes life easier. Thanks. |
# Conflicts: # benchmark/benchmain/main.go # benchmark/stats/stats.go
I have made a commit to resolve the conflicts. 86d999f |
…Put method" This reverts commit 1f4bc35.
| type SharedBufferPool interface { | ||
| // Get returns a buffer with specified length from the pool. | ||
| // | ||
| // The returned byte slice may be not zero initialized. | ||
| Get(length int) []byte | ||
|
|
||
| // Put returns a buffer to the pool. | ||
| Put(*[]byte) | ||
| } |
|
Thank you very much for your contribution @hueypark and apologies for the really drawn out review process on this PR. |
|
This is in v1.57, right? I was surprised not to see it in the release notes. [I'm quite confused because the release shows as June 26th and this PR as merged on June 27, but maybe timezones are causing that] |
|
The commit 1634254 definitely made it to v1.57.0, but I'm not sure how we missed to add it to the release notes. |
The release was JULY 26, not June 26. |
|
Aha! Thanks, I told you I was confused 😄 |
|
Good afternoon! I ran tests and noticed that objects are not added back to the pool (processUnaryRPC in the server.go file). Can you double-check? @pstibrany has already written about this above |
|
@easwars |
This PR aims to reduce memory allocation in the receive message process. Using this with a stream-heavy workload can improve performance significantly.
RELEASE NOTES: