perf: remove repeated reallocations in swap step iterations #5211
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,10 @@ type SwapState struct { | |
| feeGrowthGlobal sdk.Dec | ||
| } | ||
|
|
||
| var ( | ||
| smallestDec = sdk.SmallestDec() | ||
| ) | ||
|
|
||
| // updateFeeGrowthGlobal updates the swap state's fee growth global per unit of liquidity | ||
| // when liquidity is positive. | ||
| // | ||
|
|
@@ -263,9 +267,12 @@ func (k Keeper) computeOutAmtGivenIn( | |
| return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, types.NoSpotPriceWhenNoLiquidityError{PoolId: poolId} | ||
| } | ||
|
|
||
| var ( | ||
| tickSpacing = p.GetTickSpacing() | ||
| asset0 = p.GetToken0() | ||
| asset1 = p.GetToken1() | ||
| tokenAmountInSpecified = tokenInMin.Amount.ToDec() | ||
| ) | ||
|
|
||
| // If swapping asset0 for asset1, zeroForOne is true | ||
| zeroForOne := tokenInMin.Denom == asset0 | ||
|
|
@@ -309,8 +316,11 @@ func (k Keeper) computeOutAmtGivenIn( | |
| // initialize swap state with the following parameters: | ||
| // as we iterate through the following for loop, this swap state will get updated after each required iteration | ||
| swapState := SwapState{ | ||
| // N.B. We clone tokenAmountInSpecified because swapState.amountSpecifiedRemaining will get | ||
| // mutated during the compute swap step loop. However, we still need the original | ||
| // value of tokenAmountInSpecified to calculate the amount of tokenOut to return. | ||
| amountSpecifiedRemaining: tokenAmountInSpecified.Clone(), // tokenIn | ||
|
There was a problem hiding this comment. I see you used clone here, but for computeIn in the same step you did not. Just pointing this out There was a problem hiding this comment. IMO we do not need to use clone here There was a problem hiding this comment. if its remotely concerning, we can definitely keep cloning (An extra heap allocation is better for confidence in 0 chance of bugs) I'm supportive of doing clone to be more future proof here. The other changes are pretty trivial / isolated to convince yourself about, this one not as much There was a problem hiding this comment. Added comment: This does not apply to in given out. That's why we don't clone there |
||
| amountCalculated: sdk.ZeroDec(), // tokenOut | ||
| sqrtPrice: curSqrtPrice, | ||
| // Pad (or don't pad) current tick based on swap direction to avoid off-by-one errors | ||
| tick: swapStrategy.InitializeTickValue(p.GetCurrentTick()), | ||
|
|
@@ -328,7 +338,7 @@ func (k Keeper) computeOutAmtGivenIn( | |
| // TODO: for now, we check if amountSpecifiedRemaining is GT 0.0000001. This is because there are times when the remaining | ||
| // amount may be extremely small, and that small amount cannot generate and amountIn/amountOut and we are therefore left | ||
| // in an infinite loop. | ||
| for swapState.amountSpecifiedRemaining.GT(smallestDec) && !swapState.sqrtPrice.Equal(sqrtPriceLimit) { | ||
| // Log the sqrtPrice we start the iteration with | ||
| sqrtPriceStart := swapState.sqrtPrice | ||
|
|
||
|
|
@@ -378,15 +388,15 @@ func (k Keeper) computeOutAmtGivenIn( | |
| // Update the swapState with the new sqrtPrice from the above swap | ||
| swapState.sqrtPrice = sqrtPrice | ||
| // We deduct the amount of tokens we input in the computeSwapStep above from the user's defined tokenIn amount | ||
| swapState.amountSpecifiedRemaining.SubMut(amountIn.Add(feeCharge)) | ||
| // We add the amount of tokens we received (amountOut) from the computeSwapStep above to the amountCalculated accumulator | ||
| swapState.amountCalculated.AddMut(amountOut) | ||
|
|
||
| // If the computeSwapStep calculated a sqrtPrice that is equal to the nextSqrtPrice, this means all liquidity in the current | ||
| // tick has been consumed and we must move on to the next tick to complete the swap | ||
| if nextTickSqrtPrice.Equal(sqrtPrice) { | ||
| // Retrieve the liquidity held in the next closest initialized tick | ||
| liquidityNet, err := k.crossTick(ctx, poolId, nextTick, sdk.NewDecCoinFromDec(tokenInMin.Denom, swapState.feeGrowthGlobal)) | ||
| if err != nil { | ||
| return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, err | ||
| } | ||
|
|
@@ -399,7 +409,7 @@ func (k Keeper) computeOutAmtGivenIn( | |
|
|
||
| liquidityNet = swapStrategy.SetLiquidityDeltaSign(liquidityNet) | ||
| // Update the swapState's liquidity with the new tick's liquidity | ||
| newLiquidity := swapState.liquidity.AddMut(liquidityNet) | ||
|
There was a problem hiding this comment. yeah coming to think about it, we did not need the AddLiquidity method There was a problem hiding this comment. (note I am not checking this in PR review, trusting people with more context) |
||
| swapState.liquidity = newLiquidity | ||
|
|
||
| // Update the swapState's tick with the tick we retrieved liquidity from | ||
|
|
@@ -408,7 +418,7 @@ func (k Keeper) computeOutAmtGivenIn( | |
| // Otherwise if the sqrtPrice calculated from computeSwapStep does not equal the sqrtPrice we started with at the | ||
| // beginning of this iteration, we set the swapState tick to the corresponding tick of the sqrtPrice calculated from computeSwapStep | ||
| price := sqrtPrice.Mul(sqrtPrice) | ||
| swapState.tick, err = math.PriceToTickRoundDown(price, tickSpacing) | ||
| if err != nil { | ||
| return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, err | ||
| } | ||
|
|
@@ -459,8 +469,12 @@ func (k Keeper) computeInAmtGivenOut( | |
| return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, types.NoSpotPriceWhenNoLiquidityError{PoolId: poolId} | ||
| } | ||
|
|
||
| var ( | ||
| tickSpacing = p.GetTickSpacing() | ||
| asset0 = p.GetToken0() | ||
| asset1 = p.GetToken1() | ||
| tokenAmountOutSpecified = desiredTokenOut.Amount.ToDec() | ||
| ) | ||
|
|
||
| // if swapping asset0 (in) for asset1 (out), zeroForOne is true | ||
| zeroForOne := desiredTokenOut.Denom == asset1 | ||
|
|
@@ -479,7 +493,7 @@ func (k Keeper) computeInAmtGivenOut( | |
| } | ||
|
|
||
| // set the swap strategy | ||
| swapStrategy := swapstrategy.New(zeroForOne, sqrtPriceLimit, k.storeKey, swapFee, tickSpacing) | ||
|
|
||
| // get current sqrt price from pool | ||
| curSqrtPrice := p.GetCurrentSqrtPrice() | ||
|
|
@@ -504,8 +518,8 @@ func (k Keeper) computeInAmtGivenOut( | |
| // initialize swap state with the following parameters: | ||
| // as we iterate through the following for loop, this swap state will get updated after each required iteration | ||
| swapState := SwapState{ | ||
| amountSpecifiedRemaining: tokenAmountOutSpecified, // tokenOut | ||
p0mvn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| amountCalculated: sdk.ZeroDec(), // tokenIn | ||
| sqrtPrice: curSqrtPrice, | ||
| tick: swapStrategy.InitializeTickValue(p.GetCurrentTick()), | ||
| liquidity: p.GetLiquidity(), | ||
|
|
@@ -514,7 +528,7 @@ func (k Keeper) computeInAmtGivenOut( | |
|
|
||
| // TODO: This should be GT 0 but some instances have very small remainder | ||
| // need to look into fixing this | ||
| for swapState.amountSpecifiedRemaining.GT(smallestDec) && !swapState.sqrtPrice.Equal(sqrtPriceLimit) { | ||
| // log the sqrtPrice we start the iteration with | ||
| sqrtPriceStart := swapState.sqrtPrice | ||
|
|
||
|
|
@@ -556,29 +570,28 @@ func (k Keeper) computeInAmtGivenOut( | |
|
|
||
| // update the swapState with the new sqrtPrice from the above swap | ||
| swapState.sqrtPrice = sqrtPrice | ||
| swapState.amountSpecifiedRemaining = swapState.amountSpecifiedRemaining.SubMut(amountOut) | ||
|
There was a problem hiding this comment. what is the reason using mut here improves performance? There was a problem hiding this comment. Basically, every Int/Decimal operation in the SDK allocates a new big.Int Every new big.Int requires a heap allocation, which takes more time. (And then has to get garbage collected later) This page talks a bit about perf differences stack/heap: https://stackoverflow.com/questions/36512509/why-is-stack-memory-cheaper-than-heap-memory-in-c (and keep in mind that in C you manually free things. Garbage collection is far more expensive than a free call) Lmk if your unfamiliar with stack/heap! (In which case the above post won't make any sense) There was a problem hiding this comment. So this line is going from However in golang, we don't have interfaces for basic arithmetic that can get compiler optimized. So it can't infer that Rust doesn't have this problem There was a problem hiding this comment. Ah #5211 (comment) this change was the question that I had in mind, everythings clear now thanks! |
||
| swapState.amountCalculated = swapState.amountCalculated.AddMut(amountIn.Add(feeChargeTotal)) | ||
|
|
||
| // if the computeSwapStep calculated a sqrtPrice that is equal to the nextSqrtPrice, this means all liquidity in the current | ||
| // tick has been consumed and we must move on to the next tick to complete the swap | ||
| if sqrtPriceNextTick.Equal(sqrtPrice) { | ||
| // retrieve the liquidity held in the next closest initialized tick | ||
| liquidityNet, err := k.crossTick(ctx, poolId, nextTick, sdk.NewDecCoinFromDec(desiredTokenOut.Denom, swapState.feeGrowthGlobal)) | ||
| if err != nil { | ||
| return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, err | ||
| } | ||
| liquidityNet = swapStrategy.SetLiquidityDeltaSign(liquidityNet) | ||
| // update the swapState's liquidity with the new tick's liquidity | ||
| swapState.liquidity = swapState.liquidity.AddMut(liquidityNet) | ||
|
|
||
| // update the swapState's tick with the tick we retrieved liquidity from | ||
| swapState.tick = nextTick | ||
| } else if !sqrtPriceStart.Equal(sqrtPrice) { | ||
| // otherwise if the sqrtPrice calculated from computeSwapStep does not equal the sqrtPrice we started with at the | ||
| // beginning of this iteration, we set the swapState tick to the corresponding tick of the sqrtPrice calculated from computeSwapStep | ||
| price := sqrtPrice.Mul(sqrtPrice) | ||
| swapState.tick, err = math.PriceToTickRoundDown(price, tickSpacing) | ||
| if err != nil { | ||
| return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, err | ||
| } | ||
|
|
||
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
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.
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.
this is so sad that we have to manually do this
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.
yeah lol