exp/orderbook: Speed up path finding dfs by avoiding calls to consumeOffers for visited nodes #3933
Conversation
| @@ -73,31 +83,25 @@ func dfs( | |||
| if err := ctx.Err(); err != nil { | |||
| return err | |||
| } | |||
| if currentAssetAmount <= 0 { | |||
There was a problem hiding this comment.
we don't need to check if currentAssetAmount <= 0 because do the same check (nextAssetAmount <= 0) when processing outgoing edges.
tamirms
changed the title
| @@ -59,11 +59,21 @@ type searchState interface { | |||
| ) (xdr.Asset, xdr.Int64, error) | |||
| } | |||
|
|
|||
| func contains(list []string, want string) bool { | |||
| for i := 0; i < len(list); i++ { | |||
| if list[i] == want { | |||
There was a problem hiding this comment.
Any reason why not use xdr.Asset.Equal as before? Is string comparison much faster?
There was a problem hiding this comment.
The string comparison is faster than xdr.Asset.Equal but both of those operations are very fast.
The reason why I can't use xdr.Asset.Equal is because I only have nextAssetString I don't have the xdr.Asset form. Calling String() on each xdr.Asset in the visited list so that I can compare against nextAssetString would be significantly slower because of the String() operation
There was a problem hiding this comment.
I see, maybe we should remove visitedAssets then? If I'm reading the code right we can replace it with visitedAssetStrings everywhere.
There was a problem hiding this comment.
appendToPaths() requires visitedAssets. I think we previously discussed having an asset type which caches the String() result. I can try that out in a separate PR
This PR has been updated to work with liquidity pools as well.
…s. (#3921) * Add a way to simulate exchanges with liquidity pools * Drop unused methods from `OBGraph` interface * Iterate through interleaved set of LPs/offers * Add various test cases: LPs, non-linear ratios, interleaved hops * Speed up path-finding by avoiding edges to visited nodes (#3933) * Integrate venues deeper into the graph for performance * Update ingestion to match new function signature * Add changelog entry Co-authored-by: tamirms <[email protected]>
While I was reviewing #3921 , I realized we could improve the performance of the path finding DFS by moving the visited check from the beginning of the function to the moment we consider each outgoing edge.
Previously, we were doing both the call to
consumeOffers()and the visited check for each outgoing edge. Now, we only callconsumeOffers()if we pass the visited check. We improve the performance of the dfs by removing the expensive calls toconsumeOffers()on edges which lead to nodes we have already visited.Benchmark before changes:
Benchmark after changes:
There is about a 25% improvement in the benchmarks.