Reuse postings and avoid fmt.Sprintf to reduce mem allocations #3767
Conversation
…re fingerprints directly.
![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.
Check the status or cancel PullRequest code review here.
There was a problem hiding this comment.
| 1 Message | |
|---|---|
| 📂 | Since this pull request is no longer open, the code review job will remain active in the PullRequest network until you cancel it - or - 48 hours pass with no activity after feedback has been posted. Read More |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain)
posting/list.go, line 429 at r1 (raw file):
// here to save memory allocations. // Not entirely sure about effect on collision chances due to this simple XOR with uid. return farm.Fingerprint64(key) ^ uid
Can you also explain what the XOR is for?
Not sure what testing for this would be like but make sure you run the full suite (not sure if it runs already by default).
There was a problem hiding this comment.
Changes here make sense to me switching to an xor-ed fingerprint for a faster hash.
Reviewed with ❤️ by PullRequest
posting/list.go
Outdated
| }, | ||
| } | ||
|
|
||
| func (l *List) Release() { |
There was a problem hiding this comment.
May consider adding a docstring for this exported method to match others in the file and help with go doc generation.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain and @pullrequest[bot])
posting/list.go, line 404 at r1 (raw file):
Previously, pullrequest[bot] wrote…
May consider adding a docstring for this exported method to match others in the file and help with go doc generation.
Actually, I don't think this method needs to be exported for now.
There was a problem hiding this comment.
Addressed the comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr and @pullrequest[bot])
posting/list.go, line 404 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Actually, I don't think this method needs to be exported for now.
Made it private.
posting/list.go, line 429 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Can you also explain what the XOR is for?
Not sure what testing for this would be like but make sure you run the full suite (not sure if it runs already by default).
We have to make UID part of the conflict key. Now that we're operating on a hash, the best I could think of was to XOR it with the UID, both being uint64.
* Let's reuse postings * Do not use fmt.Sprintf to generate collision key. Instead use and store fingerprints directly. * Make release private (cherry picked from commit 6fd4dc4)
This change is