Use sync.Pool for posting lists in getInternal and getNew #3829
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
@martinmr you can click here to see the review status or cancel the code review job.
|
Previous PR: #3773. The base branch is deleted so I couldn't reopen it. @danielmai could you see if there's a difference in your machine. I've been using the 1 million dataset. |
There was a problem hiding this comment.
This looks like it matches the description and uses the sync.Pool for those two methods. It looks like there are a good many other places that allocate new PostingLists too but I'm guessing that is out of scope or not as important at this point in time to update those to use the pool.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
I think the sync.Pool is a great tool and I only had a minor comment. My primary concern with its use is the caveat in the docs for when this would be appropriate.
On the other hand, a free list maintained as part of a short-lived object is not a suitable use for a Pool, since the overhead does not amortize well in that scenario. It is more efficient to have such objects implement their own free list.
Source: https://golang.org/pkg/sync/#Pool
That leads me to wonder why you arrived at this as a bottleneck in functions sending the pb message? Have you conducted any microbenchmarks (I can't see them in this PR unfortunately).
Just want to raise the flag that the back pressure you are solving for may be in other parts of the codebase.
In general it appears that the longer the expected lifespan of pl the more appropriate the use of this pool.
Reviewed with ❤️ by PullRequest
| @@ -402,11 +402,19 @@ var postingPool = &sync.Pool{ | |||
| }, | |||
| } | |||
|
|
|||
| var postingListPool = &sync.Pool{ | |||
| New: func() interface{} { | |||
| return &pb.PostingList{} | |||
There was a problem hiding this comment.
Since a posting list is not a pointer on instantiation, I would opt for this method to be consistent with the Go Standard Library:
return new(pb.PostingList)There is a talk by Bill Kennedy that differentiates between value semantics and is why I raise the issue.
martinmr
force-pushed
the
martinmr/reuse-posting-lists
branch
from
3c68d9c to
f38851d
Compare
|
Closing this PR for now. There were some other issues with calling release (#3885 for example) so release is not being called anymore. |
This change is