Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize logic and reduce alloc #17

Open
wants to merge 4 commits into
base: feat/uint256
Choose a base branch
from
Open

optimize logic and reduce alloc #17

wants to merge 4 commits into from

Conversation

it4rb
Copy link
Member

@it4rb it4rb commented Jan 22, 2024

  • caller will pass pointer to the callee so it can write the result there, instead of callee allocating and returning new value
  • add GetOutputAmountV2 to return only necessary data (without calling NewPool again)
  • optimize MulDiv following univ3-core code (avoiding bigInt)

}
}

state.sqrtPriceX96, step.amountIn, step.amountOut, step.feeAmount, err = utils.ComputeSwapStep(state.sqrtPriceX96, targetValue, state.liquidity, state.amountSpecifiedRemaining, p.Fee)
var nxtSqrtPriceX96 utils.Uint160
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nxt typo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

um I don't get your question, it's for the price in next loop so I named it like that

@lehainam-dev
Copy link
Member

I'm reviewing your PR, but please give me some time to understand it.


// result=floor(a×b÷denominator), remainder=a×b%denominator
// (pass remainder=nil if not required)
func MulDivV2(a, b, denominator, result, remainder *uint256.Int) error {
Copy link
Member

@lehainam-dev lehainam-dev Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was kind of difficult for me to realize that remainder is an optimization to skip re-calculating mulmod(a, b, denominator) from the caller, especially when the mulDiv function in UniswapV3 contract doesn't return this value.

I would really appreciate it if we have a few comments to explain this optimization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really sorry for that! I'll add comment here and update part 1 PR description as well, please let me know if there is anything unclear there, thanks!

Copy link
Member

@lehainam-dev lehainam-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside changes related to move from *big.Int into uint256/int256, I see that most of the changes are pass the result into the function as pointer, to init variable in stack/reduce memory allocation in heap (?). So for me, this PR is not something with a lot of breaking changes.

Memory allocation also reduces by 10x after the improvement (I ran some tests myself). So yeah, I think this PR is good to go!

@it4rb
Copy link
Member Author

it4rb commented Jan 25, 2024

Thanks a lot, man!
Yes, this PR only contains optimizations on top of part1, most of the bigInt replacements are there (#16), please help to also review it when you have some time (this is not a bug fix so nothing urgent that you had to work so late like that)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants