-
Notifications
You must be signed in to change notification settings - Fork 168
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
Allow reuse of slices exposed through Ready #51
base: main
Are you sure you want to change the base?
Allow reuse of slices exposed through Ready #51
Conversation
Signed-off-by: caojiamingalan <[email protected]>
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.
Flushing some quick cosmetic comments while I'm looking at the allocations story. Couldn't quickly grasp where the savings come from: we still reallocate and copy the slice.
@pavelkalinnikov Thank you for your comments! I've resolved them.
The trick is at when we put back the slice:
This clears the slice but reserves the underlying array, which we can reuse so that we don't need to allocate a new one each iteration. |
@@ -428,6 +438,10 @@ func (rn *RawNode) acceptReady(rd Ready) { | |||
} | |||
} | |||
rn.raft.msgs = nil | |||
// Release the msgsAfterAppend slice and put it back into the msgsAfterAppendPool | |||
if cap(rn.raft.msgsAfterAppend) > 0 { | |||
rn.raft.msgsAfterAppendPool.Put(rn.raft.msgsAfterAppend[: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.
nit: add a comment explaining the logic of this optimization. Similar to what you've said (#51 (comment))
rawnode.go
Outdated
m.Responses = r.msgsAfterAppend | ||
|
||
// msgsAfterAppend is a pooled slice, so we need to make a deep copy | ||
msgs, needResp := len(r.msgsAfterAppend), false |
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.
nit: msgsCount
@@ -424,6 +426,11 @@ func newRaft(c *Config) *raft { | |||
preVote: c.PreVote, | |||
readOnly: newReadOnly(c.ReadOnlyOption), | |||
disableProposalForwarding: c.DisableProposalForwarding, | |||
msgsAfterAppendPool: &sync.Pool{ | |||
New: func() interface{} { | |||
return make([]pb.Message, 0, 8) |
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.
why 8?
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.
This is the number used in the example given by #15 (comment) . Actually I don't really know what might be a good number, so adopted that directly.
Signed-off-by: caojiamingalan <[email protected]>
b8758fe
to
72e5c3d
Compare
Solve #15:
Use pooled slice to reduce the total bytes allocated.
Benchmark result: