Skip to content
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

raft: reduce allocation #2391

Merged
merged 4 commits into from
Oct 19, 2017
Merged

raft: reduce allocation #2391

merged 4 commits into from
Oct 19, 2017

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Oct 18, 2017

No description provided.

src/raft/raft.rs Outdated
let mci = mis[self.quorum() - 1];
let term = self.term;
self.raft_log.maybe_commit(mci, term)
let mci = if self.prs.len() <= 5 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why < 5 here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the matched IDs can be put in a [0; 5] array. Generally we only have 3 or 5 replicas.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use it? I think the final code should look almost the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a benchmark case here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code will hardly change, an additional benchmark case seems redundant. My benchmark shows that use array is about 50% quicker.

When len is 3:
test bench_commit_array ... bench: 13 ns/iter (+/- 0)
test bench_commit_vec ... bench: 28 ns/iter (+/- 0)

When len is 5:
test bench_commit_array ... bench: 20 ns/iter (+/- 0)
test bench_commit_vec ... bench: 35 ns/iter (+/- 0)

src/raft/raft.rs Outdated
mis[self.quorum() - 1]
} else {
let mut mis = Vec::with_capacity(self.prs.len());
for p in self.prs.values() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we change vec to slice and unify the two similar codes?

..Default::default()
};
if !raft.msgs.is_empty() {
mem::swap(&mut raft.msgs, &mut rd.messages);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any benchmark here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very obvious that mem::swap does not copy the heap values however drain.collect will allocate a new buffer then copy all the content from original.

For a 100 length u64 vector:
test bench_drain_collect ... bench: 105 ns/iter (+/- 2)
test bench_swap ... bench: 2 ns/iter (+/- 0)

@BusyJay BusyJay force-pushed the busyjay/reduce-allocation branch from e0a6e6d to 23a6c39 Compare October 19, 2017 06:00
@BusyJay
Copy link
Member Author

BusyJay commented Oct 19, 2017

/run-unit-test

Copy link
Contributor

@siddontang siddontang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BusyJay
Copy link
Member Author

BusyJay commented Oct 19, 2017

/run-integration-test

@BusyJay BusyJay merged commit 895b76a into master Oct 19, 2017
@BusyJay BusyJay deleted the busyjay/reduce-allocation branch October 19, 2017 09:53
BusyJay added a commit to BusyJay/tikv that referenced this pull request Oct 30, 2017
@BusyJay BusyJay mentioned this pull request Oct 30, 2017
siddontang pushed a commit that referenced this pull request Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants