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 TickMath #273

Closed
wants to merge 26 commits into from
Closed

Conversation

shuhuiluo
Copy link
Contributor

@shuhuiluo shuhuiluo commented Jun 18, 2023

Related Issue

Which issue does this pull request resolve?

Description of Changes

The TickMath library has undergone extensive annotation and optimization, with the implementation ported from Aperture-Finance/uni-v3-lib. The optimizations primarily focus on eliminating branching code that involves conditional statements. An inlined and more efficient version of the mostSignificantBit function from Solady::LibBit has been incorporated, following its introduction in #257. Additional amendments have been made to reduce the number of opcodes and stack/memory operations.

New Forge tests for TickMath have been added to affirm equivalence with the reference implementation and to allow comparisons of gas usage. Gas snapshots were taken both before and after the changes to evaluate the effectiveness of these optimizations. The results are as follows:

Gas Usage Comparison

Library Test Before After Gas Efficiency
TickMath getSqrtRatioAtTick 87496 74032 15.39%
TickMath getTickAtSqrtRatio 233945 188818 23.90%

Note: Each test runs the corresponding function 100 times with deterministic inputs.

@wjmelements
Copy link
Contributor

Please accept this pull request @snreynolds

@hensha256 hensha256 self-requested a review March 19, 2024 20:59
@hensha256
Copy link
Contributor

Hey @shuhuiluo ! We're finally going through our final stages of development so are considering all your optimisations now 🙏 sorry for the delay here!

Would you mind please merging in main and updating the gas snapshots please so we can see what the new values would be?

@shuhuiluo
Copy link
Contributor Author

Hey @shuhuiluo ! We're finally going through our final stages of development so are considering all your optimisations now 🙏 sorry for the delay here!

Would you mind please merging in main and updating the gas snapshots please so we can see what the new values would be?

Will do.

shuhuiluo added 13 commits April 4, 2024 00:11
# Conflicts:
#	.forge-snapshots/initialize.snap
#	.forge-snapshots/mint with empty hook.snap
#	.forge-snapshots/mint with native token.snap
#	.forge-snapshots/mint.snap
#	.forge-snapshots/simple swap.snap
#	.forge-snapshots/swap against liquidity with native token.snap
#	.forge-snapshots/swap against liquidity.snap
#	.forge-snapshots/swap with hooks.snap
#	.forge-snapshots/swap with native.snap
#	test/TickMath.spec.ts
#	test/__snapshots__/PoolManager.gas.spec.ts.snap
#	test/__snapshots__/PoolManager.spec.ts.snap
#	test/__snapshots__/Tick.spec.ts.snap
#	test/__snapshots__/TickMath.spec.ts.snap
#	test/libraries/TickMath.t.sol
# Conflicts:
#	.forge-snapshots/addLiquidity with empty hook.snap
#	.forge-snapshots/addLiquidity with native token.snap
#	.forge-snapshots/addLiquidity.snap
#	.forge-snapshots/initialize.snap
#	.forge-snapshots/poolManager bytecode size.snap
#	.forge-snapshots/removeLiquidity with empty hook.snap
#	.forge-snapshots/removeLiquidity with native token.snap
#	.forge-snapshots/removeLiquidity.snap
#	.forge-snapshots/simple swap with native.snap
#	.forge-snapshots/simple swap.snap
#	.forge-snapshots/swap against liquidity with native token.snap
#	.forge-snapshots/swap against liquidity.snap
#	.forge-snapshots/swap burn 6909 for input.snap
#	.forge-snapshots/swap burn native 6909 for input.snap
#	.forge-snapshots/swap mint native output as 6909.snap
#	.forge-snapshots/swap mint output as 6909.snap
#	.forge-snapshots/swap with dynamic fee.snap
#	.forge-snapshots/swap with hooks.snap
#	.forge-snapshots/update dynamic fee in before swap.snap
#	src/libraries/TickMath.sol
…is in two's complement

This change replaces the shift right (`shr`) operation with the arithmetic shift right (`sar`) operation in the `getTickAtSqrtRatio` function of the `TickMath` library. The aim is to ensure the return value correctly reflects the underlying data representation in two's complement form. The change affects the variables `tickLow` and `tickHi` which store the results of these shifted operations.
shuhuiluo added 7 commits May 18, 2024 13:12
…th.getTickAtSqrtPrice`

This commit includes a major refactoring of the assembly code used to find the most significant bit of a given price in the TickMath.sol file. It replaced the bulky and repetitive lines of code with a more streamlined and condensed version from Solady.
A substantial rewriting of the calculation logic was performed in `TickMath.sol`, with a primary focus on the calculation method of the most significant bits of `sqrtPriceX96` and the int256 `log_2` value. It notably enhances precision by shifting left before right and makes use of floating-point math for approximation of `log_2`. It also removes unused variables and streamlines the assembly code to increase efficiency and readability.
@hensha256 hensha256 closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants