Avoid loading all pins into memory during migration #5
Conversation
gammazero
force-pushed
the
fix/conv-mem-use
branch
from
3aaeee7
to
c4e612e
Compare
…ins into memory, in addition to indirect pins. This is unnecessary for conversion, and much memory can be saved by avoiding loading all pins.
gammazero
force-pushed
the
fix/conv-mem-use
branch
from
c4e612e
to
5d98075
Compare
| @@ -219,13 +219,15 @@ func walkItems(ctx context.Context, dag ipld.DAGService, n *merkledag.ProtoNode, | |||
| // readHdr guarantees fanout is a safe value | |||
| fanout := hdr.GetFanout() | |||
| for i, l := range n.Links()[fanout:] { | |||
| if err := fn(i, l); err != nil { | |||
There was a problem hiding this comment.
why remove the shadow here (and below)? it's fine since we never return err anywhere else, but one of the nice things with shadowing err here is that it's obvious that there's not some higher scope err we're supposed to be keeping track of.
There was a problem hiding this comment.
I thought shadowing err was generally something to avoid. The primary reason being that someone may write a deferred function that closes over err and checks its value, and the author may not notice the shadowing.
However, I can also see the other side of that, that shadowing guarantees that the variable in the outer scope is not overwritten. I generally tend to avoid shadowing as a general pattern since I think more bugs result from shadowing as opposed to not shadowing.
Converting to datastore-based pins requires loading all dag-storage pins into memory, in addition to indirect pins. This is unnecessary for conversion, and much memory can be saved by avoiding loading all pins. An additional fix removes an unnecessary map that was created during conversion, costing even more memory.
On systems with very large pin sets, this fix helps prevent low memory conditions during migration.