From 53f05d0cddf545a7aeb83378be012b4ebef2b71a Mon Sep 17 00:00:00 2001 From: Michael Zhu Date: Mon, 10 Oct 2022 14:27:33 -0400 Subject: [PATCH 1/7] Pack heap size and array length into one storage slot --- contracts/HeapOrdering.sol | 49 +++++++++++++---------- test-foundry/TestHeapOrdering.t.sol | 6 +-- test-foundry/TestRandomHeapOrdering.t.sol | 12 +++--- test-foundry/TestStessHeapOrdering.t.sol | 9 +++-- 4 files changed, 41 insertions(+), 35 deletions(-) diff --git a/contracts/HeapOrdering.sol b/contracts/HeapOrdering.sol index 73b544af..ad1f5d99 100644 --- a/contracts/HeapOrdering.sol +++ b/contracts/HeapOrdering.sol @@ -10,8 +10,9 @@ library HeapOrdering { } struct HeapArray { - Account[] accounts; // All the accounts. - uint256 size; // The size of the heap portion of the structure, should be less than accounts length, the rest is an unordered array. + mapping(uint256 => Account) accounts; // All the accounts. + uint128 arrayLength; // The length of the array represented by the `accounts` mapping. + uint128 size; // The size of the heap portion of the structure, should be less than `arrayLength`, the rest is an unordered array. mapping(address => uint256) indexOf; // A mapping from an address to an index in accounts. From index i, the parent index is (i-1)/2, the left child index is 2*i+1 and the right child index is 2*i+2. } @@ -38,13 +39,13 @@ library HeapOrdering { address _id, uint256 _formerValue, uint256 _newValue, - uint256 _maxSortedUsers + uint128 _maxSortedUsers ) internal { uint96 formerValue = SafeCast.toUint96(_formerValue); uint96 newValue = SafeCast.toUint96(_newValue); - uint256 size = _heap.size; - uint256 newSize = computeSize(size, _maxSortedUsers); + uint128 size = _heap.size; + uint128 newSize = computeSize(size, _maxSortedUsers); if (size != newSize) _heap.size = newSize; if (formerValue != newValue) { @@ -62,7 +63,7 @@ library HeapOrdering { /// @param _size The old size of the heap. /// @param _maxSortedUsers The maximum size of the heap. /// @return The new size computed. - function computeSize(uint256 _size, uint256 _maxSortedUsers) private pure returns (uint256) { + function computeSize(uint128 _size, uint128 _maxSortedUsers) private pure returns (uint128) { while (_size >= _maxSortedUsers) _size >>= 1; return _size; } @@ -126,7 +127,7 @@ library HeapOrdering { /// @param _heap The heap to modify. /// @param _index The index of the account to move. function shiftDown(HeapArray storage _heap, uint256 _index) private { - uint256 size = _heap.size; + uint128 size = _heap.size; Account memory accountToShift = _heap.accounts[_index]; uint256 valueToShift = accountToShift.value; uint256 childIndex = (_index << 1) + 1; @@ -168,18 +169,19 @@ library HeapOrdering { HeapArray storage _heap, address _id, uint96 _value, - uint256 _maxSortedUsers + uint128 _maxSortedUsers ) private { // `_heap` cannot contain the 0 address. if (_id == address(0)) revert AddressIsZero(); // Put the account at the end of accounts. - uint256 accountsLength = _heap.accounts.length; - _heap.accounts.push(Account(_id, _value)); + uint128 accountsLength = _heap.arrayLength; + _heap.accounts[accountsLength] = Account(_id, _value); + _heap.arrayLength = accountsLength + 1; _heap.indexOf[_id] = accountsLength; // Move the account at the end of the heap and restore the invariant. - uint256 size = _heap.size; + uint128 size = _heap.size; swap(_heap, size, accountsLength); shiftUp(_heap, size); _heap.size = computeSize(size + 1, _maxSortedUsers); @@ -212,11 +214,11 @@ library HeapOrdering { HeapArray storage _heap, address _id, uint96 _newValue, - uint256 _maxSortedUsers + uint128 _maxSortedUsers ) private { uint256 index = _heap.indexOf[_id]; _heap.accounts[index].value = _newValue; - uint256 size = _heap.size; + uint128 size = _heap.size; if (index < size) shiftUp(_heap, index); else { @@ -237,12 +239,16 @@ library HeapOrdering { uint96 _removedValue ) private { uint256 index = _heap.indexOf[_id]; - uint256 accountsLength = _heap.accounts.length; + uint128 accountsLength = _heap.arrayLength; // Swap the last account and the account to remove, then pop it. swap(_heap, index, accountsLength - 1); - if (_heap.size == accountsLength) _heap.size--; - _heap.accounts.pop(); + unchecked { + // accountsLength - 1 is performed in the line above, so these cannot underflow. + delete _heap.accounts[accountsLength - 1]; + _heap.arrayLength = accountsLength - 1; + if (_heap.size == accountsLength) _heap.size = accountsLength - 1; + } delete _heap.indexOf[_id]; // If the swapped account is in the heap, restore the invariant: its value can be smaller or larger than the removed value. @@ -258,7 +264,7 @@ library HeapOrdering { /// @param _heap The heap parameter. /// @return The length of the heap. function length(HeapArray storage _heap) internal view returns (uint256) { - return _heap.accounts.length; + return _heap.arrayLength; } /// @notice Returns the value of the account linked to `_id`. @@ -267,7 +273,7 @@ library HeapOrdering { /// @return The value of the account. function getValueOf(HeapArray storage _heap, address _id) internal view returns (uint256) { uint256 index = _heap.indexOf[_id]; - if (index >= _heap.accounts.length) return 0; + if (index >= _heap.arrayLength) return 0; Account memory account = _heap.accounts[index]; if (account.id != _id) return 0; else return account.value; @@ -277,7 +283,7 @@ library HeapOrdering { /// @param _heap The heap to get the head. /// @return The address of the head. function getHead(HeapArray storage _heap) internal view returns (address) { - if (_heap.accounts.length > 0) return _heap.accounts[ROOT].id; + if (_heap.arrayLength > 0) return _heap.accounts[ROOT].id; else return address(0); } @@ -285,7 +291,7 @@ library HeapOrdering { /// @param _heap The heap to get the tail. /// @return The address of the tail. function getTail(HeapArray storage _heap) internal view returns (address) { - uint256 accountsLength = _heap.accounts.length; + uint128 accountsLength = _heap.arrayLength; if (accountsLength > 0) return _heap.accounts[accountsLength - 1].id; else return address(0); } @@ -308,8 +314,7 @@ library HeapOrdering { /// @return The address of the next account. function getNext(HeapArray storage _heap, address _id) internal view returns (address) { uint256 index = _heap.indexOf[_id]; - if (index + 1 >= _heap.accounts.length || _heap.accounts[index].id != _id) - return address(0); + if (index + 1 >= _heap.arrayLength || _heap.accounts[index].id != _id) return address(0); else return _heap.accounts[index + 1].id; } } diff --git a/test-foundry/TestHeapOrdering.t.sol b/test-foundry/TestHeapOrdering.t.sol index e619690b..bc582173 100644 --- a/test-foundry/TestHeapOrdering.t.sol +++ b/test-foundry/TestHeapOrdering.t.sol @@ -14,7 +14,7 @@ contract TestHeapOrdering is DSTest { address[] public accounts; uint256 public NB_ACCOUNTS = 50; - uint256 public MAX_SORTED_USERS = 50; + uint128 public MAX_SORTED_USERS = 50; address public ADDR_ZERO = address(0); HeapOrdering.HeapArray internal heap; @@ -420,7 +420,7 @@ contract TestHeapOrdering is DSTest { for (uint256 i = 10; i < 15; i++) update(accounts[i], 0, i - 9); - for (uint256 i = 10; i < 15; i++) assertLe(heap.accounts[i].value, 10); + for (uint128 i = 10; i < 15; i++) assertLe(heap.accounts[i].value, 10); } function testInsertWrap() public { @@ -488,7 +488,7 @@ contract TestHeapOrdering is DSTest { uint256 sizeAfter = heap.size; assertLt(sizeAfter, sizeBefore); - assertEq(heap.accounts.length, 2); + assertEq(heap.arrayLength, 2); } function testRemoveShiftDown() public { diff --git a/test-foundry/TestRandomHeapOrdering.t.sol b/test-foundry/TestRandomHeapOrdering.t.sol index 10cd51cc..4addb15f 100644 --- a/test-foundry/TestRandomHeapOrdering.t.sol +++ b/test-foundry/TestRandomHeapOrdering.t.sol @@ -43,7 +43,7 @@ contract Helper is Random { function updateHeap( address _id, uint256 _newValue, - uint256 _max_sorted_users + uint128 _max_sorted_users ) public { HeapOrdering.HeapArray storage heap = getCurrentHeap(); uint256 formerValue = HeapOrdering.getValueOf(heap, _id); @@ -59,11 +59,11 @@ contract Helper is Random { // Check that for the parent value (at rank i) is always greater than the ones of his children // (at rank 2i, 2i + 1), for ranks <= head size HeapOrdering.HeapArray storage heap = getCurrentHeap(); - for (uint256 rank = 1; rank < getSize(); rank++) { + for (uint128 rank = 1; rank < getSize(); rank++) { HeapOrdering.Account memory user = heap.accounts[rank - 1]; // child = 0 (left) or child = 1 (right) - for (uint256 child; child <= 1; child++) { - uint256 childRank = rank * 2 + child; + for (uint128 child; child <= 1; child++) { + uint128 childRank = rank * 2 + child; if (childRank <= getSize()) { HeapOrdering.Account memory childUser = heap.accounts[childRank - 1]; require( @@ -87,7 +87,7 @@ contract TestHeapRandomHeapOrdering is DSTest { uint256 N_USERS = n; uint256 RANGE_VALUES = 2 * n; uint256 N_ITERS = 3 * n; - uint256 max_sorted_users = helper.randomUint256(N_USERS); + uint128 max_sorted_users = uint128(helper.randomUint256(N_USERS)); for (uint256 iter; iter < N_ITERS; iter++) { address user = helper.randomAddress(N_USERS); uint256 newValue; @@ -97,7 +97,7 @@ contract TestHeapRandomHeapOrdering is DSTest { } if (helper.randomUint256(N_ITERS) <= 3) { // change max users approximately 3 times per test - max_sorted_users = helper.randomUint256(N_USERS); + max_sorted_users = uint128(helper.randomUint256(N_USERS)); } helper.updateHeap(user, newValue, max_sorted_users); } diff --git a/test-foundry/TestStessHeapOrdering.t.sol b/test-foundry/TestStessHeapOrdering.t.sol index 079640eb..a276a255 100644 --- a/test-foundry/TestStessHeapOrdering.t.sol +++ b/test-foundry/TestStessHeapOrdering.t.sol @@ -7,15 +7,16 @@ import "@contracts/HeapOrdering.sol"; contract HeapStorage { HeapOrdering.HeapArray internal heap; - uint256 public TESTED_SIZE = 10000; - uint256 public MAX_SORTED_USERS = TESTED_SIZE; + uint128 public TESTED_SIZE = 10000; + uint128 public MAX_SORTED_USERS = TESTED_SIZE; uint256 public incrementAmount = 5; function setUp() public { for (uint256 i = 0; i < TESTED_SIZE; i++) { address id = address(uint160(i + 1)); - heap.indexOf[id] = heap.accounts.length; - heap.accounts.push(HeapOrdering.Account(id, uint96(TESTED_SIZE - i))); + heap.indexOf[id] = heap.arrayLength; + heap.accounts[heap.arrayLength] = HeapOrdering.Account(id, uint96(TESTED_SIZE - i)); + heap.arrayLength++; heap.size = MAX_SORTED_USERS; } } From 9a0215e58b9c316652e21973faa1ec467a786bab Mon Sep 17 00:00:00 2001 From: Michael Zhu Date: Tue, 11 Oct 2022 10:33:28 -0400 Subject: [PATCH 2/7] Keep (accountsLength - 1) in local variable --- contracts/HeapOrdering.sol | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/contracts/HeapOrdering.sol b/contracts/HeapOrdering.sol index ad1f5d99..5987140c 100644 --- a/contracts/HeapOrdering.sol +++ b/contracts/HeapOrdering.sol @@ -242,12 +242,13 @@ library HeapOrdering { uint128 accountsLength = _heap.arrayLength; // Swap the last account and the account to remove, then pop it. - swap(_heap, index, accountsLength - 1); + uint128 newArrayLength = accountsLength - 1; + swap(_heap, index, newArrayLength); unchecked { // accountsLength - 1 is performed in the line above, so these cannot underflow. - delete _heap.accounts[accountsLength - 1]; - _heap.arrayLength = accountsLength - 1; - if (_heap.size == accountsLength) _heap.size = accountsLength - 1; + delete _heap.accounts[newArrayLength]; + _heap.arrayLength = newArrayLength; + if (_heap.size == accountsLength) _heap.size = newArrayLength; } delete _heap.indexOf[_id]; From 068ce5bc6ab4e44759d4ba4e8fc5fa5eb7a3008f Mon Sep 17 00:00:00 2001 From: MathisGD Date: Thu, 13 Oct 2022 00:18:55 +0200 Subject: [PATCH 3/7] perf: avoid uint128 padding --- contracts/HeapOrdering.sol | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/contracts/HeapOrdering.sol b/contracts/HeapOrdering.sol index 5987140c..5902fd6d 100644 --- a/contracts/HeapOrdering.sol +++ b/contracts/HeapOrdering.sol @@ -39,14 +39,14 @@ library HeapOrdering { address _id, uint256 _formerValue, uint256 _newValue, - uint128 _maxSortedUsers + uint256 _maxSortedUsers ) internal { uint96 formerValue = SafeCast.toUint96(_formerValue); uint96 newValue = SafeCast.toUint96(_newValue); - uint128 size = _heap.size; - uint128 newSize = computeSize(size, _maxSortedUsers); - if (size != newSize) _heap.size = newSize; + uint256 size = _heap.size; + uint256 newSize = computeSize(size, _maxSortedUsers); + if (size != newSize) _heap.size = uint128(newSize); if (formerValue != newValue) { if (newValue == 0) remove(_heap, _id, formerValue); @@ -63,7 +63,7 @@ library HeapOrdering { /// @param _size The old size of the heap. /// @param _maxSortedUsers The maximum size of the heap. /// @return The new size computed. - function computeSize(uint128 _size, uint128 _maxSortedUsers) private pure returns (uint128) { + function computeSize(uint256 _size, uint256 _maxSortedUsers) private pure returns (uint256) { while (_size >= _maxSortedUsers) _size >>= 1; return _size; } @@ -127,7 +127,7 @@ library HeapOrdering { /// @param _heap The heap to modify. /// @param _index The index of the account to move. function shiftDown(HeapArray storage _heap, uint256 _index) private { - uint128 size = _heap.size; + uint256 size = _heap.size; Account memory accountToShift = _heap.accounts[_index]; uint256 valueToShift = accountToShift.value; uint256 childIndex = (_index << 1) + 1; @@ -169,22 +169,22 @@ library HeapOrdering { HeapArray storage _heap, address _id, uint96 _value, - uint128 _maxSortedUsers + uint256 _maxSortedUsers ) private { // `_heap` cannot contain the 0 address. if (_id == address(0)) revert AddressIsZero(); // Put the account at the end of accounts. - uint128 accountsLength = _heap.arrayLength; + uint256 accountsLength = _heap.arrayLength; _heap.accounts[accountsLength] = Account(_id, _value); - _heap.arrayLength = accountsLength + 1; + _heap.arrayLength = uint128(accountsLength + 1); _heap.indexOf[_id] = accountsLength; // Move the account at the end of the heap and restore the invariant. - uint128 size = _heap.size; + uint256 size = _heap.size; swap(_heap, size, accountsLength); shiftUp(_heap, size); - _heap.size = computeSize(size + 1, _maxSortedUsers); + _heap.size = uint128(computeSize(size + 1, _maxSortedUsers)); } /// @notice Decreases the amount of an account in the `_heap`. @@ -214,17 +214,17 @@ library HeapOrdering { HeapArray storage _heap, address _id, uint96 _newValue, - uint128 _maxSortedUsers + uint256 _maxSortedUsers ) private { uint256 index = _heap.indexOf[_id]; _heap.accounts[index].value = _newValue; - uint128 size = _heap.size; + uint256 size = _heap.size; if (index < size) shiftUp(_heap, index); else { swap(_heap, size, index); shiftUp(_heap, size); - _heap.size = computeSize(size + 1, _maxSortedUsers); + _heap.size = uint128(computeSize(size + 1, _maxSortedUsers)); } } @@ -239,16 +239,16 @@ library HeapOrdering { uint96 _removedValue ) private { uint256 index = _heap.indexOf[_id]; - uint128 accountsLength = _heap.arrayLength; + uint256 accountsLength = _heap.arrayLength; // Swap the last account and the account to remove, then pop it. - uint128 newArrayLength = accountsLength - 1; + uint256 newArrayLength = accountsLength - 1; swap(_heap, index, newArrayLength); unchecked { // accountsLength - 1 is performed in the line above, so these cannot underflow. delete _heap.accounts[newArrayLength]; - _heap.arrayLength = newArrayLength; - if (_heap.size == accountsLength) _heap.size = newArrayLength; + _heap.arrayLength = uint128(newArrayLength); + if (_heap.size == accountsLength) _heap.size = uint128(newArrayLength); } delete _heap.indexOf[_id]; @@ -292,7 +292,7 @@ library HeapOrdering { /// @param _heap The heap to get the tail. /// @return The address of the tail. function getTail(HeapArray storage _heap) internal view returns (address) { - uint128 accountsLength = _heap.arrayLength; + uint256 accountsLength = _heap.arrayLength; if (accountsLength > 0) return _heap.accounts[accountsLength - 1].id; else return address(0); } From fc7a78ca11079731528e9ca3b2d032fa372984cd Mon Sep 17 00:00:00 2001 From: MathisGD Date: Wed, 19 Apr 2023 15:43:12 +0200 Subject: [PATCH 4/7] pref: remove useless unchecked --- src/HeapOrdering.sol | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/HeapOrdering.sol b/src/HeapOrdering.sol index a2bf7d4f..78c8f2ad 100644 --- a/src/HeapOrdering.sol +++ b/src/HeapOrdering.sol @@ -255,13 +255,10 @@ library HeapOrdering { // Swap the last account and the account to remove, then pop it. uint256 newArrayLength = accountsLength - 1; swap(_heap, index, newArrayLength); - unchecked { - // accountsLength - 1 is performed in the line above, so these cannot underflow. - delete _heap.accounts[newArrayLength]; - _heap.arrayLength = uint128(newArrayLength); - } - if (_heap.size == accountsLength) _heap.size = uint128(--_size); + delete _heap.accounts[newArrayLength]; delete _heap.indexOf[_id]; + _heap.arrayLength = uint128(newArrayLength); + if (_heap.size == accountsLength) _heap.size = uint128(--_size); // If the swapped account is in the heap, restore the invariant: its value can be smaller or larger than the removed value. if (index < _size) { From ef96f44153590bcf93407e57c894c5e336840297 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Wed, 19 Apr 2023 15:48:16 +0200 Subject: [PATCH 5/7] test: remove useless changes --- test/TestCommonHeapOrdering.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/TestCommonHeapOrdering.t.sol b/test/TestCommonHeapOrdering.t.sol index 8c0cadcf..0f6cff99 100644 --- a/test/TestCommonHeapOrdering.t.sol +++ b/test/TestCommonHeapOrdering.t.sol @@ -12,7 +12,7 @@ abstract contract TestCommonHeapOrdering is Test { address[] public accounts; uint256 public NB_ACCOUNTS = 50; - uint128 public MAX_SORTED_USERS = 50; + uint256 public MAX_SORTED_USERS = 50; address public ADDR_ZERO = address(0); function update( @@ -296,7 +296,7 @@ abstract contract TestCommonHeapOrdering is Test { for (uint256 i = 10; i < 15; i++) update(accounts[i], 0, i - 9); - for (uint128 i = 10; i < 15; i++) assertLe(heap.accountsValue(i), 10); + for (uint256 i = 10; i < 15; i++) assertLe(heap.accountsValue(i), 10); } function testDecreaseIndexChanges() public { From a373890c1b3a09c338d76d7f876cecbeba3a630a Mon Sep 17 00:00:00 2001 From: MathisGD Date: Wed, 19 Apr 2023 15:48:46 +0200 Subject: [PATCH 6/7] perf: further optimizations --- src/HeapOrdering.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/HeapOrdering.sol b/src/HeapOrdering.sol index 78c8f2ad..dd62e499 100644 --- a/src/HeapOrdering.sol +++ b/src/HeapOrdering.sol @@ -285,7 +285,7 @@ library HeapOrdering { if (index >= _heap.arrayLength) return 0; Account memory account = _heap.accounts[index]; if (account.id != _id) return 0; - else return account.value; + return account.value; } /// @notice Returns the address at the head of the `_heap`. @@ -293,7 +293,7 @@ library HeapOrdering { /// @return The address of the head. function getHead(HeapArray storage _heap) internal view returns (address) { if (_heap.arrayLength > 0) return _heap.accounts[ROOT].id; - else return address(0); + return address(0); } /// @notice Returns the address at the tail of unsorted portion of the `_heap`. @@ -302,7 +302,7 @@ library HeapOrdering { function getTail(HeapArray storage _heap) internal view returns (address) { uint256 accountsLength = _heap.arrayLength; if (accountsLength > 0) return _heap.accounts[accountsLength - 1].id; - else return address(0); + return address(0); } /// @notice Returns the address coming before `_id` in accounts. @@ -313,7 +313,7 @@ library HeapOrdering { function getPrev(HeapArray storage _heap, address _id) internal view returns (address) { uint256 index = _heap.indexOf[_id]; if (index > ROOT) return _heap.accounts[index - 1].id; - else return address(0); + return address(0); } /// @notice Returns the address coming after `_id` in accounts. @@ -324,6 +324,6 @@ library HeapOrdering { function getNext(HeapArray storage _heap, address _id) internal view returns (address) { uint256 index = _heap.indexOf[_id]; if (index + 1 >= _heap.arrayLength || _heap.accounts[index].id != _id) return address(0); - else return _heap.accounts[index + 1].id; + return _heap.accounts[index + 1].id; } } From f5ae58d37e4e2467f2bdd8d3bca01ff294388ff0 Mon Sep 17 00:00:00 2001 From: MathisGD Date: Thu, 20 Apr 2023 19:19:08 +0200 Subject: [PATCH 7/7] perf: remove useless storage read and reorder lines --- src/HeapOrdering.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/HeapOrdering.sol b/src/HeapOrdering.sol index dd62e499..e5f29e21 100644 --- a/src/HeapOrdering.sol +++ b/src/HeapOrdering.sol @@ -252,13 +252,13 @@ library HeapOrdering { uint256 index = _heap.indexOf[_id]; uint256 accountsLength = _heap.arrayLength; - // Swap the last account and the account to remove, then pop it. + // Swap the last account and the account to remove, then "pop" it. uint256 newArrayLength = accountsLength - 1; swap(_heap, index, newArrayLength); + if (_size == accountsLength) _heap.size = uint128(--_size); delete _heap.accounts[newArrayLength]; - delete _heap.indexOf[_id]; _heap.arrayLength = uint128(newArrayLength); - if (_heap.size == accountsLength) _heap.size = uint128(--_size); + delete _heap.indexOf[_id]; // If the swapped account is in the heap, restore the invariant: its value can be smaller or larger than the removed value. if (index < _size) {