[R4R]: Redesign triePrefetcher to make it thread safe #972
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
setunapo
force-pushed
the
redesign_triePrefetch
branch
4 times, most recently
from
a8ee0a1
to
244f26c
Compare
qinglin89
reviewed
Jul 1, 2022
|
too many channels and messages communication, actually only |
setunapo
force-pushed
the
redesign_triePrefetch
branch
3 times, most recently
from
843785c
to
faf01f0
Compare
setunapo
changed the title
setunapo
requested review from
flywukong,
unclezoro,
qinglin89,
yutianwu and
forcodedancing
qinglin89
reviewed
Jul 4, 2022
setunapo
force-pushed
the
redesign_triePrefetch
branch
2 times, most recently
from
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
67071ff
to
936530b
Compare
unclezoro
approved these changes
Jul 7, 2022
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
triePrefetcherstruct, 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.