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

Check bloom filter in Get function #2846

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

rjl493456442
Copy link
Contributor

@rjl493456442 rjl493456442 commented Aug 24, 2023

This pull request enhances the 'Get' function by incorporating a bloom filter check before loading data. This offers the advantage of minimizing avoidable disk reads, thereby contributing positively to the overall read performance.

The implementation is quite straightforward, as recommended by @jbowens. I use iterator.SeekPrefixGE, a method that internally performs a bloom filter check.

Informs #197

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jbowens jbowens requested review from a team and sumeerbhola August 24, 2023 16:05
@rjl493456442
Copy link
Contributor Author

rjl493456442 commented Aug 25, 2023

-- random-read-filter-exp-1000mb-non-existent-cp (2046 events) total time: 278.4032s
 total size: 1047756600 bytes
  mean mb/s: 3.589 (+- 0.707)
-- random-read-filter-master-1000mb-non-existent-cp (2046 events) total time: 368.5611s
 total size: 1047756600 bytes
  mean mb/s: 2.711 (+- 0.405)

Construct a 1.8gb size database and perform reading for non-existent entries, this PR is slightly faster than master.

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.
:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rjl493456442)


level_iter.go line 811 at r1 (raw file):

	if l.split != nil {
		n = l.split(l.iterFile.LargestPointKey.UserKey)
	}

can you add
// Else l.split == nil, so we use the length of the key. This case can occur when getIter uses SeekPrefixGE.

@rjl493456442
Copy link
Contributor Author

I have added the comments, please take another look.

@sumeerbhola
Copy link
Collaborator

Looks good!

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