Make order matching parallel in EndBlock#159
Conversation
2c077d5 to
c518e10
Compare
x/dex/cache/cache.go
Outdated
| s.BlockOrders.Range(func(key, val any) bool { | ||
| contractAddr := key.(typesutils.ContractAddress) | ||
| _map := val.(*sync.Map) | ||
| _map.Range(func(pkey, pval any) bool { | ||
| pair := pkey.(typesutils.PairString) | ||
| blockOrders := pval.(*BlockOrders) | ||
| for _, blockOrder := range *blockOrders { | ||
| copy.GetBlockOrders(contractAddr, pair).AddOrder(blockOrder) | ||
| } |
There was a problem hiding this comment.
nit - could probably move this to a helper function where we pass in GetBlockOrders and AddOrder functions as parameters and reuse it for all types below
There was a problem hiding this comment.
yeah good call. I'll make an update
There was a problem hiding this comment.
well since golang doesn't support passing type as parameters or generics, this is probably as far as it goes: https://github.com/sei-protocol/sei-chain/pull/159/files#diff-f8d8121f7fdd1b62bd88a2ffb5afeeb0b601ed29a813686e9d327f538720a514R102
| func NewParallelRunner(runnable func(contract types.ContractInfo), contracts []types.ContractInfo) ParallelRunner { | ||
| contractAddrToInfo := sync.Map{} | ||
| contractsFrontier := sync.Map{} | ||
| for _, contract := range contracts { |
There was a problem hiding this comment.
So i understand correctly - we have a topo sorted graph and here is the logic to grab all frontier nodes (that is - contracts with no dependencies) and execute them together. And the next set of frontier nodes is determined by for len(validContractAddresses) > 0 { in x/dex/module.go
There was a problem hiding this comment.
The runner will figure out what the next nodes to process within a single Run() call. I updated comments for this file and will just paste it here:
We define "frontier contract" as a contract which:
1. Has not finished running yet, and
2. either:
a. has no other contracts depending on it, or
b. for which all contracts that depend on it have already finished.
Consequently, the set of frontier contracts will mutate throughout the
`Run` method, until all contracts finish their runs.
The key principle here is that at any moment, we can have all frontier
contracts running concurrently, since there must be no ancestral
relationships among them due to the definition above.
The simplest implementation would be:
`
while there is any contract left:
run all frontier contracts concurrently
wait for all runs to finish
update the frontier set
`
We can illustrate why this implementation is not optimal with the following
example:
Suppose we have four contracts, where A depends on B, and C depends on
D. The run for A, B, C, D takes 5s, 5s, 8s, 2s, respectively.
With the implementation above, the first iteration would take 8s since
it runs A and C, and the second iteration would take 5s since it runs
B and D. However C doesn't actually need to wait for B to finish, and
if C runs immediately after A finishes, the whole process would take
max(5 + 5, 8 + 2) = 10s, which is 3s faster than the implementation
above.
So we can optimize the implementation to be:
`
while there is any contract left:
run all frontier contracts concurrently
wait for any existing run (could be from previous iteration) to finish
update the frontier set
`
With the example above, the whole process would take 3 iterations:
Iter 1 (A, C run): 5s since it finishes when A finishes
Iter 2 (B run): 3s since it finishes when C finishes
Iter 3 (D run): 2s since it finishes when B, D finish
There was a problem hiding this comment.
I see, so it's improving among the concurrency in the sense that the simple solution is basically sort of like bfs and we process all nodes at level n before moving to level n+1, while the optimized solution is to remove the restriction of processing all contracts at the same level. And this optimization is done by keeping a list which may contain existing contracts when checking frontier contracts each loop. This is awesome, tyty
x/dex/module.go
Outdated
| } | ||
| } | ||
| } | ||
| runner := contract.NewParallelRunner(runnable, validContractInfo) |
There was a problem hiding this comment.
nit - can we make this validContractsInfo so we know its multiple contracts rather than one
fec6100 to
b45caf0
Compare
|
do we still need this PR? |
b45caf0 to
cac6775
Compare
71328fa to
02b3dbe
Compare
|
@LCyson yes this is to actually add the core parallelization logic, using the data structure utilities merged in other PRs |
c301565 to
8c1bc20
Compare
## Describe your changes and provide context Fixing some error handling in x/bank/keeper module ## Testing performed to validate your change Existing unit test should cover this Co-authored-by: Yiming Zang <yzang@twitter.com>
## Describe your changes and provide context Fixing some error handling in x/bank/keeper module ## Testing performed to validate your change Existing unit test should cover this Co-authored-by: Yiming Zang <yzang@twitter.com>
…s for ConsensusParams (#156) * Remove unresponsive witnesses + consensusParams return on first resp * Add Witness blacklist * Default TTL * add comment * Fix indentation * Add jitter to retry * Update tests * blacklistTTL configurable * Update tests with blacklist TTL * Update test * Update blacklist tests * gofmt * Update mapstructure * Remove unused struct * Update light/client.go Co-authored-by: Steven Landers <steven.landers@gmail.com> * Add a cancel when lb is received * Child Context to cancel inner witness goroutines * Update comments * defer childCancel * childCancel * remove def * Remove witness when removing provider for p2p-down (#159) * remove witness when removing provider for p2p-down * add tests * Add comments + split up blacklist function * fmt * Only blacklist in detect divergence --------- Co-authored-by: Kartik Bhat <kartikbhat@kartiks-mbp-2.mynetworksettings.com> Co-authored-by: Steven Landers <steven.landers@gmail.com>
EDIT: going to break this down into smaller PRs
This PR includes the following changes:
maps used during order matching tosync.MapnumIncomingPathsfield onContractInfothat's automatically calculated upon contract registration. This field indicates the number of contracts that depend on the current contractParallelRunnerthat takes in a runnable and a list ofContractInfo, and invoke runnable forContractInfoaccording to their topological order while maximizing parallelism. Note that the runner implicitly performs topological sort so the list being passed in doesn't have to be topologically sorted already. (The oldTopologicalSortutility is now only used for validation upon contract registration)ParallelRunnerforendBlockForContractindexEndBlock