-
Notifications
You must be signed in to change notification settings - Fork 324
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
Process batch serialise concurrently #3877
Changes from 10 commits
996573e
3b3504e
e705bba
3a272b4
3a229d9
2309ef9
59eb5a0
d117115
088bc8e
37e9fb0
3d00c43
4b10b27
7b1f120
796d7c9
e8d4733
aeddae8
6e7fa46
41c26b7
5716c32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,19 +96,30 @@ func (b *baseKVStoreBatch) Entry(index int) (*WriteInfo, error) { | |
func (b *baseKVStoreBatch) SerializeQueue(serialize WriteInfoSerialize, filter WriteInfoFilter) []byte { | ||
b.mutex.Lock() | ||
defer b.mutex.Unlock() | ||
// 1. This could be improved by being processed in parallel | ||
// 2. Digest could be replaced by merkle root if we need proof | ||
bytes := make([]byte, 0) | ||
// 1. Digest could be replaced by merkle root if we need proof | ||
bytesChan := make(chan []byte, len(b.writeQueue)) | ||
wg := sync.WaitGroup{} | ||
wg.Add(len(b.writeQueue)) | ||
for _, wi := range b.writeQueue { | ||
if filter != nil && filter(wi) { | ||
continue | ||
} | ||
if serialize != nil { | ||
bytes = append(bytes, serialize(wi)...) | ||
} else { | ||
bytes = append(bytes, wi.Serialize()...) | ||
} | ||
go func(info *WriteInfo) { | ||
if filter != nil && filter(info) { | ||
return | ||
} | ||
pocockn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if serialize != nil { | ||
bytesChan <- serialize(info) | ||
} else { | ||
bytesChan <- info.Serialize() | ||
} | ||
wg.Done() | ||
}(wi) | ||
} | ||
wg.Wait() | ||
|
||
bytes := make([]byte, 0) | ||
for itemBytes := range bytesChan { | ||
bytes = append(bytes, itemBytes...) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in general this is fine, but probably won't work for our case. Because the total bytes will be hashed to serve as a digest of the batch's content, which is used later in validation. So the bytes has to be in fixed order, and working parallel breaks this order There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, does this mean the PR is no longer needed? Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should ensure that the order of writing bytes matches the order in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @envestcc modified it so the writing of the bytes matches the order 👍 |
||
|
||
return bytes | ||
} | ||
|
||
|
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.
should the mtx be kept?
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.
Hey @Liuhaai , @envestcc said it's not needed as the concurrent writes now happen on different indices within the slice
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.
https://stackoverflow.com/questions/42238425/golang-mutex-ranging-over-shared-array-in-goroutines
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.
could write an unit test to verify the concurrency problem
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.
mtx is for
b.writeQueue
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.
Added the mtx back @Liuhaai
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.
does the
Unlock
can be called earlier? It seems like that it can be placed just beforewg.Wait()
.but it seems that this is unrelated to the current issue. It may not improve the serialization process. Perhaps it can be discussed in a new issue.