change pinning to happen in a callback #1274

whyrusleeping · 2015-05-21T16:45:32Z

this does the pinning for the add command inline (as it was intended to be) instead of after the fact. This has a significant perf improvement over the current speed of add, and that improvement increases the larger the file being added is.

jbenet · 2015-05-27T18:29:49Z

core/commands/add.go

 				if err != nil {
 					res.SetError(err, cmds.ErrNormal)
 					return
 				}

+				mp := n.Pinning.GetManual()
+				mp.RemovePinWithMode(rnk, pin.Indirect)


why is this being removed?

because it is added below (for all files found) and then re-added here?

why not just not pin anything until the end? i.e. remove the call to pin below and only pin recursively here after adding?

is the worry that something will GC in-between, while adding? (if so, ok makes sense).

why not just not pin anything until the end?

thats what this PR is trying to avoid doing. in order to pin after the fact, you have to pull every single block in the dag back off the disk, and write the pin for it. This is really expensive when we get into the 50+GB range of add operations. (or in the 10MB+ range on my pi)

is the worry that something will GC in-between, while adding? (if so, ok makes sense).

This too, a poorly timed gc could end really badly for us. (although its still not perfectly safe)

maybe mark and sweep is TRTTD

On Wed, May 27, 2015 at 11:42:36AM -0700, Jeromy Johnson wrote:

is the worry that something will GC in-between, while adding? (if
so, ok makes sense).

This too, a poorly timed gc could end really badly for us. (although
its still not perfectly safe)

I think the right approach here is to have transactional changes to
the pin tree (see also the commit sessions discussed in #1187).
Coupling that with something like Git's gc.pruneexpire to avoid
garbage-collecting recently-created objects (say, in the last week?)
would give us better safety here. It would also protect folks from
accidentally GC'ing recent objects they'd forgotten to tag.

Coupling that with something like Git's gc.pruneexpire

that's a great solution. of course it comes from git.

i'm really concerned about safety here--

@whyrusleeping @tv42 -- what's the plan long term?

jbenet · 2015-05-27T18:37:17Z

+1 to the general idea.

indirect remove question (i think it's likely TRTTD), but am worried about leaking pins in crash scenarios. @tv42's suggestion for a mark-sweep gc may be the easier option. (i.e. don't store "indirect" pins, calculate them on gc. it's still O(n) so mark-sweep likely similar perf.
callback - why not provide the actual block? or mdag node?
callback - what is the bool for? named parameters + doc:
rebase

wking · 2015-05-27T18:50:18Z

On Wed, May 27, 2015 at 11:37:19AM -0700, Juan Batiz-Benet wrote:

why not provide the actual block? or mdag node?

what is the bool for? named parameters + doc:

See also my callback suggestions in #1291. The post-chunk/layout
callback there both accepts and returns *dag.Node objects.

whyrusleeping · 2015-05-27T18:52:38Z

yeah, i guess i used a key because i already had them in both places i was calling the callback. a node makes sense.

whyrusleeping · 2015-05-28T22:08:22Z

@jbenet @wking comments addressed, if no further CR, this is RFM

wking · 2015-05-28T22:49:29Z

On Thu, May 28, 2015 at 03:08:23PM -0700, Jeromy Johnson wrote:

@jbenet @wking comments addressed, if no further CR, this is RFM

I'd still drop ‘root’ from your NodeCB signature, and I'd like to drop
indirect pinning in favor of a time-based grace period before
garbage-collecting nodes 1. But as it stands this PR is still a
step in the right direction, so I'm +1 to merge.

jbenet · 2015-05-29T19:25:48Z

importer/helpers/dagbuilder.go

@@ -5,6 +5,13 @@ import (
 	"github.com/ipfs/go-ipfs/pin"
 )

+// NodeCB is callback function for dag generation
+// the `root` flag signifies whether or not this is
+// the root of a dag.


i mean this is weird, right, because every node is the root of a dag.

maybe

the `last` flag signifies whether or not this is the last (top-most root) node being added. useful for things like only pinning the first node recursively.

jbenet · 2015-05-29T19:27:33Z

I'd still drop ‘root’ from your NodeCB signature,

I wold like that too, but i think this needs to switch to mark and sweet.

and I'd like to drop
indirect pinning in favor of a time-based grace period before
garbage-collecting nodes [1].

Well indirect pins need to be permanent. (or you mean only indirect pinning the root?) i think indirect pins can be dropped entirely if we go with mark-and-sweep as @tv42 suggested a while back.

wking · 2015-05-29T19:39:55Z

On Fri, May 29, 2015 at 12:27:34PM -0700, Juan Batiz-Benet wrote:

and I'd like to drop indirect pinning in favor of a time-based
grace period before garbage-collecting nodes 1.

Well indirect pins need to be permanent. (or you mean only indirect
pinning the root?) i think indirect pins can be dropped entirely if
we go with mark-and-sweep as @tv42 suggested a while back.

Yeah, or tri-color marking 1 or whatever. Explicitly using
pin-markers for every object in a recursive tree seems like you're
forcing the client to help you mark, when that should really be
something handled internally by the GC implementation. Of course,
you'd want the time-based grace period or some such to bridge the gap
between adding an ancestor node and pinning the root node, but that
seems manageable.

jbenet · 2015-05-29T19:42:35Z

Let's continue the gc discussion here: ipfs/notes#11

change pinning to happen in a callback

whyrusleeping added the topic/perf Performance label May 21, 2015

whyrusleeping self-assigned this May 21, 2015

whyrusleeping added the status/in-progress In progress label May 21, 2015

whyrusleeping mentioned this pull request May 21, 2015

Improve Add performance #1223

Closed

jbenet mentioned this pull request May 21, 2015

Sprint H ipfs/team-mgmt#9

Closed

52 tasks

whyrusleeping force-pushed the refactor/importer branch 2 times, most recently from 89d7625 to 8dae027 Compare May 22, 2015 03:08

wking mentioned this pull request May 25, 2015

Spec for adder APIs (both low-level and high-level) #1291

Closed

jbenet reviewed May 27, 2015
View reviewed changes

whyrusleeping added 3 commits May 28, 2015 08:52

change pinning to happen in a callback

dd928a2

make callback take a node instead of a key

b660f93

docs

38479ff

whyrusleeping force-pushed the refactor/importer branch from d563763 to 38479ff Compare May 28, 2015 15:52

jbenet reviewed May 29, 2015
View reviewed changes

jbenet mentioned this pull request May 29, 2015

Repo GC strategy. ipfs/notes#11

Open

rename root to last

a0aa07e

jbenet added a commit that referenced this pull request May 30, 2015

Merge pull request #1274 from ipfs/refactor/importer

5d43ebb

change pinning to happen in a callback

jbenet merged commit 5d43ebb into master May 30, 2015

jbenet removed the status/in-progress In progress label May 30, 2015

jbenet deleted the refactor/importer branch May 30, 2015 00:06

wking referenced this pull request in sgotti/oci-store Jul 2, 2016

Initial Commit.

29dd456

ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Apr 7, 2022

Merge pull request ipfs#1274 from ipfs/refactor/importer

2900037

change pinning to happen in a callback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change pinning to happen in a callback #1274

change pinning to happen in a callback #1274

whyrusleeping commented May 21, 2015

jbenet May 27, 2015

jbenet May 27, 2015

whyrusleeping May 27, 2015

whyrusleeping May 27, 2015

jbenet May 27, 2015

wking May 28, 2015

jbenet May 29, 2015

jbenet commented May 27, 2015

wking commented May 27, 2015

whyrusleeping commented May 27, 2015

whyrusleeping commented May 28, 2015

wking commented May 28, 2015

jbenet May 29, 2015

whyrusleeping May 29, 2015

jbenet commented May 29, 2015

wking commented May 29, 2015

jbenet commented May 29, 2015

change pinning to happen in a callback #1274

change pinning to happen in a callback #1274

Conversation

whyrusleeping commented May 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented May 27, 2015

wking commented May 27, 2015

whyrusleeping commented May 27, 2015

whyrusleeping commented May 28, 2015

wking commented May 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented May 29, 2015

wking commented May 29, 2015

jbenet commented May 29, 2015