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

support MultiGet operation #80

Merged
merged 5 commits into from
Mar 4, 2019
Merged

support MultiGet operation #80

merged 5 commits into from
Mar 4, 2019

Conversation

coocood
Copy link
Member

@coocood coocood commented Mar 1, 2019

No description provided.

@coocood
Copy link
Member Author

coocood commented Mar 1, 2019

@ngaut PTAL

levels.go Outdated
if pair.found {
continue
}
val, err := h.get(pair.key) // Calls h.RLock() and h.RUnlock().
Copy link
Member

@ngaut ngaut Mar 1, 2019

Choose a reason for hiding this comment

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

Can we reduce calling of RLock and RUnlock?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can move the getTablesForKey and decrTableRef out of h.get, then move RLock out of getTablesForKey, but after I do it, I found the code become much harder to read.
So I prefer not to do so.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try another way to do it.

db.go Outdated
}()
y.NumGets.Add(int64(len(pairs)))
var foundCount int
for i := 0; i < len(tables); i++ {
Copy link
Member

Choose a reason for hiding this comment

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

use range pattern.

y.NumGets.Add(int64(len(pairs)))
var foundCount int
for _, table := range tables {
for j := range pairs {
Copy link
Member

Choose a reason for hiding this comment

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

for _, pair := range pairs

Copy link
Member Author

Choose a reason for hiding this comment

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

This way, we cannot modify the pair in the slice because the pair is a value, not a pointer.

var expectedValues []string
for i := 0; i < n; i += 100 {
multiGetKeys = append(multiGetKeys, data(i))
expectedValues = append(expectedValues, fmt.Sprintf("zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz%9d", i))
Copy link
Member

Choose a reason for hiding this comment

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

Remove "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"

Copy link
Member Author

Choose a reason for hiding this comment

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

This test needs the value to be large enough, so there would be multiple levels in DB.

Copy link
Member

Choose a reason for hiding this comment

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

please add comments.

level_handler.go Outdated

if th.DoesNotHave(keyNoTs) {
func (s *levelHandler) decrTableRefs(tables []*table.Table) {
Copy link
Member

Choose a reason for hiding this comment

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

Make decrTableRefs as a pure function, so we can reuse it in multiget method.

transaction.go Outdated
@@ -366,6 +363,46 @@ func (txn *Txn) Get(key []byte) (item *Item, rerr error) {
return item, nil
}

type keyValuePair struct {
key []byte
found bool
Copy link
Member

Choose a reason for hiding this comment

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

move found down.

level_handler.go Outdated
return y.ValueStruct{}
}

func (s *levelHandler) getInTable(key []byte, th *table.Table) (result y.ValueStruct) {
Copy link
Member

Choose a reason for hiding this comment

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

s/th/table/

return s.refLevel0Tables()
}
out := make([]*table.Table, len(pairs))
for i, pair := range pairs {
Copy link
Member

Choose a reason for hiding this comment

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

There are some redundant tables, these tables may cause unnecessary search.
We should group by level, table, and keys. Also we do a lot of redundant work, such as recalculate key's hash when do bloom filter looking up.

Copy link
Member

Choose a reason for hiding this comment

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

We can do more optimization in further PR.

@coocood
Copy link
Member Author

coocood commented Mar 2, 2019

@ngaut PTAL

level_handler.go Outdated
return []*table.Table{tbl}
}

// refTablesForKeys return tables for pairs.
Copy link
Member

Choose a reason for hiding this comment

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

s/return/returns/

level_handler.go Outdated
@@ -224,23 +234,50 @@ func (s *levelHandler) close() error {
return errors.Wrap(err, "levelHandler.close")
}

// getTableForKey acquires a read-lock to access s.tables. It returns a list of tableHandlers.
func (s *levelHandler) getTableForKey(key []byte) []*table.Table {
// refTablesForKey acquires a read-lock to access s.tables. It returns a list of tableHandlers.
Copy link
Member

Choose a reason for hiding this comment

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

s/tableHandlers/table

@ngaut
Copy link
Member

ngaut commented Mar 4, 2019

LGTM

@ngaut ngaut merged commit 948fe04 into master Mar 4, 2019
@ngaut ngaut deleted the multi-get branch March 4, 2019 03:59
coocood added a commit that referenced this pull request Mar 4, 2019
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.

2 participants