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

HOG-07C: Removal Upward Shift Optimization #38

Closed
MerlinEgalite opened this issue Jul 5, 2022 · 1 comment
Closed

HOG-07C: Removal Upward Shift Optimization #38

MerlinEgalite opened this issue Jul 5, 2022 · 1 comment
Assignees
Labels
🎨 enhancement New feature or request 🚫 wontfix This will not be worked on

Comments

@MerlinEgalite
Copy link
Contributor

HOG-07C: Removal Upward Shift Optimization

Type Severity Location
Gas Optimization HeapOrdering.sol:L258

Description:

The shiftUp operation is performed unconditionally whilst a child and parent's value can indeed match, thus executing an upwards shift inefficiently if the element removed was next-to-last in the tree and its children had an equal value to it.

Example:

// If the swapped account is in the heap, restore the invariant: its value can be smaller or larger than the removed value.
if (rank <= _heap.size) {
    if (_removedValue > getAccount(_heap, rank).value) shiftDown(_heap, rank);
    else shiftUp(_heap, rank);
}

Recommendation:

We advise the else statement to be adjusted to an else if (_removedValue < getAccount(_heap, rank).value) one, with the getAccount(_heap, rank).value value being cached to a local variable within the upper-most if clause to avoid the duplicate read gas cost between the if and else if conditional evaluations.

@MerlinEgalite MerlinEgalite added the 🎨 enhancement New feature or request label Jul 5, 2022
@QGarchery QGarchery self-assigned this Jul 6, 2022
@MathisGD MathisGD changed the title Removal Upward Shift Optimization HOG-07C: Removal Upward Shift Optimization Jul 6, 2022
@QGarchery QGarchery added 🚫 wontfix This will not be worked on and removed ready for review labels Jul 9, 2022
@QGarchery
Copy link
Collaborator

These changes seems unlikely to save gas in our case: the amounts will all be different because of the interests earn. So even if 2 person supply exactly the same amount, if one does so one block after the other, the amounts will be different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎨 enhancement New feature or request 🚫 wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants