-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[R4R]: Redesign triePrefetcher to make it thread safe #972
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
setunapo
changed the title
[WIP]: Redesign triePrefetch to make it thread safe
[WIP]: Redesign triePrefetcher to make it thread safe
Jun 30, 2022
setunapo
force-pushed
the
redesign_triePrefetch
branch
4 times, most recently
from
July 1, 2022 02:17
a8ee0a1
to
244f26c
Compare
qinglin89
reviewed
Jul 1, 2022
too many channels and messages communication, actually only |
setunapo
commented
Jul 1, 2022
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.
Replied
setunapo
force-pushed
the
redesign_triePrefetch
branch
3 times, most recently
from
July 4, 2022 02:09
843785c
to
faf01f0
Compare
setunapo
changed the title
[WIP]: Redesign triePrefetcher to make it thread safe
[R4R]: Redesign triePrefetcher to make it thread safe
Jul 4, 2022
setunapo
requested review from
flywukong,
unclezoro,
qinglin89,
yutianwu and
forcodedancing
July 4, 2022 02:16
qinglin89
reviewed
Jul 4, 2022
setunapo
force-pushed
the
redesign_triePrefetch
branch
2 times, most recently
from
July 5, 2022 01:18
1cd350f
to
8900d55
Compare
qinglin89
reviewed
Jul 5, 2022
qinglin89
previously approved these changes
Jul 5, 2022
yutianwu
reviewed
Jul 5, 2022
forcodedancing
previously approved these changes
Jul 5, 2022
unclezoro
reviewed
Jul 5, 2022
unclezoro
reviewed
Jul 5, 2022
unclezoro
reviewed
Jul 5, 2022
yutianwu
previously approved these changes
Jul 6, 2022
unclezoro
previously approved these changes
Jul 6, 2022
j75689
previously approved these changes
Jul 6, 2022
qinglin89
previously approved these changes
Jul 6, 2022
There are 2 types of triePrefetcher instances: 1.New created triePrefetcher: it is key to do trie prefetch to speed up validation phase. 2.Copied triePrefetcher: it only copy the prefetched trie information, actually it won't do prefetch at all, the copied tries are all kept in p.fetches. Here we try to improve the new created one, to make it concurrent safe, while the copied one's behavior stay unchanged(its logic is very simple). As commented in triePrefetcher struct, its APIs are not thread safe. So callers should make sure the created triePrefetcher should be used within a single routine. As we are trying to improve triePrefetcher, we would use it concurrently, so it is necessary to redesign it for concurrent access. The design is simple: ** start a mainLoop to do all the work, APIs just send channel message. Others: ** remove the metrics copy, since it is useless for copied triePrefetcher ** for trie(), only get subfetcher through channel to reduce the workload of mainloop
** make close concurrent safe ** fix potential deadlock
For APIs like: trie(), copy(), used(), it is simpler and more efficient to use a RWMutex instead of channel communicaton. Since the mainLoop would be busy handling trie request, while these trie request can be processed in parallism. We would only keep prefetch and close within the mainLoop, since they could update the fetchers
trie prefetcher’s behavior has changed, prefetch() won't create subfetcher immediately. it is reasonable, but break the UT, to fix the failed UT
setunapo
force-pushed
the
redesign_triePrefetch
branch
from
July 7, 2022 01:23
67071ff
to
936530b
Compare
unclezoro
approved these changes
Jul 7, 2022
brilliant-lx
approved these changes
Jul 7, 2022
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.
SGTM
qinglin89
approved these changes
Jul 7, 2022
j75689
approved these changes
Jul 7, 2022
yutianwu
approved these changes
Jul 7, 2022
KeefeL
approved these changes
Jul 7, 2022
This was referenced Jul 28, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There are 2 types of triePrefetcher instances.
1.New created
triePrefetcher
: it is the one to do trie prefetch to speed up validation phase.2.Copied
triePrefetcher
: it just do a copy of the prefetched trie, it won't do prefetch at all. The copied tries are all kept in p.fetches.In thie PR, we try to improve the new created one, to make it concurrent safe, while the copied one's behavior stay unchanged(its logic is very simple).
Rationale
As commented in
triePrefetcher
struct, its APIs are not thread safe. So callers should make sure the created triePrefetcher should be used within a single routine. As we are trying to improve triePrefetcher, we would use it concurrently, so it is necessary to redesign it for concurrent access.The redesigned workflow:
1.start a mainLoop to do the real trie prefetch work, caller could schedule prefetch request through channel message.
2.as the trie prefetch close operation is only allowed to do once and will be processed in the mainLoop too, since it will update the fetchers informantion.
3.The other operations will be done outside of the mainLoop for performance concern, since mainLoop could be too busy if all the works are done within the mainLoop. The action lists are: trie(),used(), copy(), these actions can be done in parallel with a simple read lock.
There is no obvious performance impact.
Changes
No impact to users.