TXM In-memory: add priority queue: STEP 1 #12121
Conversation
poopoothegorilla
temporarily deployed
to
sdlc
GitHub Actions
Inactive
|
I see that you haven't updated any README files. Would it make sense to do so? |
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
poopoothegorilla
requested review from
patrick-dowell,
prashantkumar1982 and
amit-momin
common/txmgr/priority_queue.go
Outdated
| pq.Lock() | ||
| defer pq.Unlock() | ||
|
|
||
| return heap.Pop(pq).(*txmgrtypes.Tx[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) |
There was a problem hiding this comment.
Would it make sense to capture these low level ops as a separate clean generic heap, without the tx queue specifics? Then this can just have a field of type Heap[*txmgrtypes.Tx[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE] without mixing in explicit casts.
There was a problem hiding this comment.
Hmm... I might not be understanding what you are proposing fully. We will need a method which has the ability to remove a Tx by ID (for pruning purposes)
There was a problem hiding this comment.
Lets hold off on refactoring to make more generic for a future PR after all this stuff gets merged
There was a problem hiding this comment.
Overall this looks reasonable, but I notice a lot of redundant code, mainly I think because TxPriorityQueue is thinly wrapping PriorityQueue. I wonder if it'd be a little cleaner to just collapse these two files together and save some code lines / complexity.
poopoothegorilla
temporarily deployed
to
sdlc
GitHub Actions
Inactive
| pq.Lock() | ||
|
|
||
| if pq.ph.Len() == pq.ph.Cap() { | ||
| heap.Pop(pq.ph) |
There was a problem hiding this comment.
Restating my understanding:
priorityHeap is meant to store tx ordered by creation time, with the oldest tx at index 0, and the newest tx at index length - 1.
The current implementation of Pop on priorityHeap:
func (pq *priorityHeap[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) Pop() any {
old := pq.txs
n := len(old)
tx := old[n-1]
old[n-1] = nil // avoid memory leak
pq.txs = old[0 : n-1]
delete(pq.idToIndex, tx.ID)
return tx
}
Here, IFIUC, we remove the tx at index length - 1 from both the array and the index map, which is the tx with the most recent creation time.
Then, in AddTx, we have
if pq.ph.Len() == pq.ph.Cap() {
heap.Pop(pq.ph)
}
So, when the queue is full, doesn't the current code remove the newest transaction?
There was a problem hiding this comment.
It is using the heap pkg... so that Pop is actually https://cs.opensource.google/go/go/+/refs/tags/go1.22.1:src/container/heap/heap.go;l=59
So it should take the item that is next on the Heap
|
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
This PR is the start of a chain of PRs to make the review process easier. The PR adds a priority queue that will be used by the in-memory TXM.
NOTES:
container/heappkg https://pkg.go.dev/container/heap