Move method freePages into freelist.go #783
Conversation
|
Link to #773 |
ahrtr
force-pushed
the
refactor_freelist_20240701
branch
from
f219a03
to
f2b2bba
Compare
| @@ -24,6 +24,7 @@ type pidSet map[common.Pgid]struct{} | |||
| type freelist struct { | |||
| freelistType FreelistType // freelist type | |||
| ids []common.Pgid // all free and available free page ids. | |||
| readonlyTXIDs []common.Txid // all readonly transaction IDs. | |||
There was a problem hiding this comment.
do we really need this? this seems tracked in the pending map already
There was a problem hiding this comment.
okay now I get it, this is a subset of the transactions in the pending map. I think we need to refactor a dedicated transaction tracker
There was a problem hiding this comment.
The TXIDs included in pending are actually writing TXIDs.
freelist is one of the most sensitive & important parts of bbolt, let's do it step by step. We need to ensure each step is well understood.
| last := len(f.readonlyTXIDs) - 1 | ||
| f.readonlyTXIDs[i] = f.readonlyTXIDs[last] | ||
| f.readonlyTXIDs = f.readonlyTXIDs[:last] | ||
| break |
There was a problem hiding this comment.
there's slices.Delete() which we can use. I'm wondering if it's better to have a map here too, so we can also panic on duplicated transactions. This removal would just delete the first occurrence.
There was a problem hiding this comment.
I'm wondering if it's better to have a map here too
It's trade-off. The freePages needs to sort before use. If I understand it correctly, freePages needs to allocate memory for sort.
There was a problem hiding this comment.
there's
slices.Delete()which we can use.
slices.Delete is O(len(s)-i) and it copes/moves all elements after the removed element, while our implementation is O(1).
I'm wondering if it's better to have a map here too, so we can also panic on duplicated transactions.
No, we can't. Multiple readonly TXNs may have the same txid, and it's expected.
| last := len(f.readonlyTXIDs) - 1 | ||
| f.readonlyTXIDs[i] = f.readonlyTXIDs[last] | ||
| f.readonlyTXIDs = f.readonlyTXIDs[:last] | ||
| break |
There was a problem hiding this comment.
I'm wondering if it's better to have a map here too
It's trade-off. The freePages needs to sort before use. If I understand it correctly, freePages needs to allocate memory for sort.
The motivation is to get all freelist related logic included in freelist.go. We are going to introduce freelist interface in the next step. Signed-off-by: Benjamin Wang <[email protected]>
ahrtr
force-pushed
the
refactor_freelist_20240701
branch
from
f2b2bba
to
263e75d
Compare
The motivation is to get all freelist related logic included in freelist.go. We are going to introduce freelist interface in the next step.
The
freePagesis the most important logic of freelist management, but it's outside of freelist management. Moving it into freelist.go is the base for introducing freelist interface and introduce more dedicated test cases.cc @tjungblu @fuweid @ivanvc