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

Added get multiple api for badger #1990

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

Conversation

harshil-goel
Copy link
Contributor

@harshil-goel harshil-goel commented Aug 6, 2023

Added get multiple api in badger.
Able to get upto 60% performance improvement in ldbc s10 dataset queries.
4.69 seconds -> 2.61 seconds

db.go Outdated
if vs.Meta == 0 && vs.Value == nil {
continue
}
// Found the required version of the key, return immediately.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be refactored, since we don't return immediately like in db.get()


func TestReadC(t *testing.T) {
t.Skip()
allKeysF, err := os.Open("/home/harshil/all_keys_2")
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit weird to have an explicit user path, make it a temp path or something?

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 s just a temp test for my local validation. Hence the t.Skip() before the test starts. I can't commit this as the directory being used is quite big.


maxVs := make([]y.ValueStruct, len(keys))

y.NumGetsAdd(db.opt.MetricsEnabled, 1)

Choose a reason for hiding this comment

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

If we are using the old metric, we should increment this by len(keys)

@@ -756,6 +758,44 @@ func (db *DB) getMemTables() ([]*memTable, func()) {
// do that. For every get("fooX") call where X is the version, we will search
// for "fooX" in all the levels of the LSM tree. This is expensive but it
// removes the overhead of handling move keys completely.
func (db *DB) getBatch(keys [][]byte, done []bool) ([]y.ValueStruct, error) {

Choose a reason for hiding this comment

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

Nit: can we rename done to keyRead or something similar?

}
}
return results, nil
} else {
Copy link

Choose a reason for hiding this comment

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

@harshil-goel I need to understand this code block (lines 344-373) ... will sync up with you offline.

Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

This PR needs lot more refactoring, testing and cleanup. Only two comments for now but once it is cleaned up, can review again.

// Find the table for which the key is in, and then seek it
getForKey := func(key []byte) (y.ValueStruct, func() error, []*table.Iterator) {
tables, decr := s.getTableForKey(key)
keyNoTs := y.ParseKey(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should just parse the key once, we are doing it again and again

for _, it := range itrs {
it.Seek(key)
if !it.Valid() {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

break?

@netlify
Copy link

netlify bot commented Sep 29, 2023

Deploy Preview for badger-docs ready!

Name Link
🔨 Latest commit 12e3de3
🔍 Latest deploy log https://app.netlify.com/sites/badger-docs/deploys/6516e01ef64a0500092c8e9d
😎 Deploy Preview https://deploy-preview-1990--badger-docs.netlify.app/projects-using-badger
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Harshil Goel seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants