-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
@ngaut PTAL |
levels.go
Outdated
if pair.found { | ||
continue | ||
} | ||
val, err := h.get(pair.key) // Calls h.RLock() and h.RUnlock(). |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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++ { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for _, pair := range pairs
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@ngaut PTAL |
level_handler.go
Outdated
return []*table.Table{tbl} | ||
} | ||
|
||
// refTablesForKeys return tables for pairs. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/tableHandlers/table
LGTM |
No description provided.