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.

Sign up for GitHub

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

Jump to bottom

FindFileInRange: devirtualize comparator and add prefix cache #10646

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

FindFileInRange: devirtualize comparator and add prefix cache #10646

wants to merge 2 commits into from

Conversation

rockeet
Copy link
Contributor

@rockeet rockeet commented Sep 7, 2022

This PR is based on #10645.

If comparator is BytewiseComparator or ReverseBytewiseComparator:

  1. devirtualize comparator: specialize the impl by direct call memcmp
  2. add prefix cache: narrow the search range by prefix cache, then find by comparator

@rockeet rockeet changed the title FindFileInRange: devirtualize comparator and prefix cache FindFileInRange: devirtualize comparator and add prefix cache Sep 7, 2022
@rockeet rockeet mentioned this pull request Sep 7, 2022
Open
anand1976
anand1976 reviewed Sep 30, 2022
View reviewed changes
Copy link
Contributor

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

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

Interesting! Do you have db_bench results that you can share? Also, what is the performance impact of the comparator devirtualization specifically? Wondering if that can be applied to other places, like DataBlockIter.

db/version_set.cc
// Find File in LevelFilesBrief data structure
// Within an index range defined by left and right
int FindFileInRange(const InternalKeyComparator& icmp,
const LevelFilesBrief& file_level,
const Slice& key,
uint32_t left,
uint32_t right) {
if (IsForwardBytewiseComparator(icmp.user_comparator())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The comparator is not a mutable option, so is it worth doing this check once and reuse the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This checks data field opt_cmp_type_, it is just a trivial uint8_t compare.

db/version_set.cc
mid = (lo + hi) / 2;
exact_search:
#ifdef __GNUC__
__builtin_prefetch(a[mid].largest_key.data_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how effective this prefetch will be, since we use it right away.

Copy link
Contributor Author

@rockeet rockeet Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

before the memcmp accessing a[mid].largest_key.data_, there are enough other independent instructions, so it is worth to prefetch.

@rockeet
Copy link
Contributor Author

rockeet commented Oct 3, 2022

Interesting! Do you have db_bench results that you can share? Also, what is the performance impact of the comparator devirtualization specifically? Wondering if that can be applied to other places, like DataBlockIter.

We generated flame graphs, CPU time of FindFileInRange was dropped from more than 30% to about 1.5%, this result is easy to be reproduced.

This was referenced Oct 21, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anand1976 anand1976 anand1976 left review comments

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rockeet @anand1976 @facebook-github-bot