diff --git a/rbf/cursor.go b/rbf/cursor.go index 697956ebd..205acb0f1 100644 --- a/rbf/cursor.go +++ b/rbf/cursor.go @@ -22,6 +22,9 @@ const ( // // If you mutate the b-tree that a cursor is attached to then you'll need to // re-initialize the cursor. This may be changed in the future though. +// +// Cursors can be reused in some cases; if you're keeping a cursor around, +// be sure to nil out the tx, or it will hold a reference to it. type Cursor struct { tx *Tx buffered bool // if true, Next() and Prev() do not move the cursor position @@ -1464,11 +1467,22 @@ func (c *Cursor) difference(key uint64, data *roaring.Container) (bool, error) { return false, nil } +// Close closes a cursor out, invalidating it for future use, and puts it +// back in a pool to reduce allocations. Contrast with unpooledClose, which +// you probably shouldn't use. func (c *Cursor) Close() { assert(c != nil) + c.unpooledClose() cursorSyncPool.Put(c) } +// unpooledClose is the part of a close operation which does not put the +// cursor back in the pool. It is useful only for cases where we're reusing +// a cursor, such as a Db's freelistCursor. +func (c *Cursor) unpooledClose() { + c.tx = nil +} + func keysFromParents(parents []branchCell) (ckeys []int) { for _, par := range parents { ckeys = append(ckeys, int(par.LeftKey)) diff --git a/rbf/tx.go b/rbf/tx.go index 9c7bc2d56..192ee3697 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -170,6 +170,7 @@ func (tx *Tx) truncateLastFreePage() (truncated bool, outErr error) { defer tx.freelistCleanup(&outErr) c := tx.db.getFreelistCursor(tx) + defer c.unpooledClose() if err := c.Last(); err == io.EOF { return false, nil } else if err != nil { @@ -1121,6 +1122,7 @@ func (tx *Tx) freelistCleanup(outErr *error) { return } c := tx.db.getFreelistCursor(tx) + defer c.unpooledClose() for len(tx.pendingFreelistAdds) > 0 { var pass []uint32 pass, tx.pendingFreelistAdds = tx.pendingFreelistAdds, nil @@ -1157,6 +1159,7 @@ func (tx *Tx) allocatePgno() (_ uint32, outErr error) { tx.modifyingFreelist = true defer tx.freelistCleanup(&outErr) c := tx.db.getFreelistCursor(tx) + defer c.unpooledClose() if err := c.First(); err == io.EOF { return tx.allocateNewPgno(), nil } else if err != nil { @@ -1200,6 +1203,7 @@ func (tx *Tx) freePgno(pgno uint32) (outErr error) { return nil } c := tx.db.getFreelistCursor(tx) + defer c.unpooledClose() tx.modifyingFreelist = true defer tx.freelistCleanup(&outErr) @@ -1750,8 +1754,13 @@ type containerFilter struct { } func (s *containerFilter) Close() { + // note that the cursor gets put back in the pool, but that cursor.Close + // zeroes out the cursor's tx for us. s.cursor.Close() s.cursor = nil + s.tx = nil + s.filter = nil + s.rewriter = nil containerFilterPool.Put(s) } @@ -2077,6 +2086,15 @@ func (tx *Tx) flush() error { tx.pageMap = tx.pageMap.Set(pgno, walID) } + // At this point, it is safe to nil out the dirtyPages and + // dirtyBitmapPages objects. We don't. The reason we don't is that + // we should never have a Tx lasting for long anyway -- even if we + // end up holding the write lock for a checkpoint, we don't keep the + // associated Tx around. If we nil those out, then a few stray Tx + // objects sticking around won't stick out in a heap profile. If we + // leave them alone, they'll stick out in a heap profile. I think on + // the whole that's better for further observability and debugging. + // Write meta page to WAL. walID, err := tx.writeToWAL(w, tx.meta[:]) if err != nil {