Add MaxHeap
#5076
Add MaxHeap
#5076
Conversation
|
| struct Node { | ||
| uint256 value; | ||
| uint256 heapIndex; | ||
| } |
There was a problem hiding this comment.
Maybe we can do something like:
- assuming the
heapIndexfits into something smaller. I'm not expecting any onchain structure to have more thantype(uint32).maxelements. Maybe justtype(uint16).maxis enough (that is 65k elements). - limit the value to uint224
So that Node fits into a single slot.
| mapping(uint256 => uint256) tree; | ||
| mapping(uint256 => Node) items; |
There was a problem hiding this comment.
I'm expecting one of these mapping (maybe both) can be implemented using array. That would result in better data locality when we move to verkle.
There was a problem hiding this comment.
also, using array with a value smaller than uint256 (for tree) would compress the data (shared slots)
| (heap.tree[fpos], heap.tree[spos]) = (heap.tree[spos], heap.tree[fpos]); | ||
| (heap.items[heap.tree[fpos]].heapIndex, heap.items[heap.tree[spos]].heapIndex) = (fpos, spos); |
There was a problem hiding this comment.
| (heap.tree[fpos], heap.tree[spos]) = (heap.tree[spos], heap.tree[fpos]); | |
| (heap.items[heap.tree[fpos]].heapIndex, heap.items[heap.tree[spos]].heapIndex) = (fpos, spos); | |
| ( | |
| heap.tree[fpos], heap.items[heap.tree[fpos]].heapIndex, | |
| heap.tree[spos], heap.items[heap.tree[spos]].heapIndex | |
| ) = ( | |
| heap.tree[spos], spos, | |
| heap.tree[fpos], fpos | |
| ); |
| } | ||
|
|
||
| function heapify(MaxHeap storage heap, uint256 pos) internal { | ||
| if (pos >= (heap.size / 2) && pos <= heap.size) return; |
There was a problem hiding this comment.
I think pos <= heap.size should be pos < heap.size.
Also pos >= heap.size should be an error if that is a user call ... but that would require changes to the recursive calls
| uint256 heapIndex; | ||
| } | ||
|
|
||
| struct MaxHeap { |
There was a problem hiding this comment.
Random though: maybe we want a Heap, instead of a MaxHeap, with a function pointer for the comparator.
| } | ||
| } | ||
|
|
||
| function insert(MaxHeap storage heap, uint256 itemId, uint256 value) internal { |
There was a problem hiding this comment.
My understand is that itemId must be unique, so that all items are clearly identified ... but is then useless to the user. Maybe it should be automatically generated ... so that the user doesn't have to worry about it, and can just push a value.
|
Closing in favor of improved implementation at #5084 |
Fixes #3410 — WIP
PR Checklist
npx changeset add)