exp/orderbook: Speed up path finding by changing edgeSet data structure #3965
Conversation
There was a problem hiding this comment.
We never use edgeSet to do map lookups in the dfs.
I think I passively observed this during the latest changeset, but was too deep in a refactor to give it proper consideration. Super exciting to see a follow-up on it! 🎉
A few minor DRY-related comments and other questions below ⬇️
|
|
||
| offers := e.values[i].offers | ||
| updatedOffers := offers |
There was a problem hiding this comment.
Can we modify offers directly?
There was a problem hiding this comment.
I didn't want to modify offers while iterating through it in the for i, offer := range offers loop
There was a problem hiding this comment.
Ah good point, I see. It won't keep iterating after it's modified since it breaks immediately, but I can see why you would do this to avoid confusion when reading the code.
|
|
||
| offers := e.values[i].offers | ||
| updatedOffers := offers |
There was a problem hiding this comment.
Ah good point, I see. It won't keep iterating after it's modified since it breaks immediately, but I can see why you would do this to avoid confusion when reading the code.
|
|
||
| // addOffer will insert the given offer into the edge set | ||
| func (e edgeSet) addOffer(key string, offer xdr.OfferEntry) { | ||
| func (e edgeSet) addOffer(key string, offer xdr.OfferEntry) edgeSet { |
There wasn't a noticeable difference but the code was cleaner using an |
I did some profiling on path finding and found that iterating through entries in the edgeSet
mapwas particularly expensive. We never useedgeSetto do map lookups in the dfs. We only use the data structure to iterate through the key value pairs. So I changed the representation to a pair of lists and that resulted in a significant improvement in the path finding performance.Before applying the changes in this commit:
After applying the changes in this commit: