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

Pack heap size + array length #75

Closed
wants to merge 8 commits into from

Conversation

MathisGD
Copy link
Collaborator

@MathisGD MathisGD commented Oct 12, 2022

Extension of #73 by @moodlezoup.

Replaces the HeapArray.accounts array with a "virtual array" consisting of a mapping and a uint128 length. This lets us pack the array length and heap size into the same storage slot.

I just added the use of uint256 as much as possible, preventing Solidity to go back and forth between uint128 and uint256. I also removed useless and costly else.

image

@MathisGD MathisGD self-assigned this Oct 12, 2022
moodlezoup
moodlezoup previously approved these changes Oct 13, 2022
Copy link

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

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

thanks! feel free to close my PR in favor of this one

contracts/HeapOrdering.sol Outdated Show resolved Hide resolved
contracts/HeapOrdering.sol Outdated Show resolved Hide resolved
@QGarchery QGarchery changed the title Opti #73 Pack heap size + array length Nov 7, 2022
@MathisGD MathisGD requested a review from MerlinEgalite March 19, 2023 17:55
@MerlinEgalite
Copy link
Contributor

Can I review after the fix of the conflicts?

@MathisGD MathisGD force-pushed the perf/heap-ordering-pack-sizes-1 branch from aa22d0a to ba7fb9e Compare April 19, 2023 13:37
@MathisGD
Copy link
Collaborator Author

Rebased and updated @MerlinEgalite

@MathisGD MathisGD requested a review from QGarchery April 19, 2023 13:58
Copy link
Collaborator

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

I don't think we should merge this PR, for the reasons explained in this comment. It would change the data-structure to improve gas consumption, when doing #63 would be better in that regard, and also better because it would spread liquidity evenly. So if we want to change the heap ordering I think we should go all the way and implement #63

src/HeapOrdering.sol Show resolved Hide resolved
src/HeapOrdering.sol Outdated Show resolved Hide resolved
@MathisGD MathisGD requested a review from MerlinEgalite April 24, 2023 17:07
@MerlinEgalite
Copy link
Contributor

I don't think we should merge this PR, for the reasons explained in this comment. It would change the data-structure to improve gas consumption, when doing #63 would be better in that regard, and also better because it would spread liquidity evenly. So if we want to change the heap ordering I think we should go all the way and implement #63

given this comment, should we review? @MathisGD @QGarchery

Copy link
Collaborator

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

given this comment, should we review?

Probably not, unless there are other arguments in favor of merging it I think we should close this PR for now

@MathisGD MathisGD closed this Apr 25, 2023
@MathisGD MathisGD deleted the perf/heap-ordering-pack-sizes-1 branch April 25, 2023 21:14
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