Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'ipfs pin update' command #3754

Closed
whyrusleeping opened this issue Mar 7, 2017 · 13 comments
Closed

Add 'ipfs pin update' command #3754

whyrusleeping opened this issue Mar 7, 2017 · 13 comments
Labels
kind/enhancement A net-new feature or improvement to an existing feature status/in-progress In progress
Milestone

Comments

@whyrusleeping
Copy link
Member

Usecase: I have a large graph pinned. I make a change to this graph, and now i want to pin the new root, and unpin the old root.

You can accomplish this today by just ipfs pin add <NEW> and ipfs pin rm <OLD> but this has a couple drawbacks. First, it has to rescan the entire new graph. If we had the context of the old pin around, we could skip traversing subtrees that are in the old graph since theyre already pinned (and guaranteed as far as this process is concerned to be there). This would make the pin update process much faster. The second concern, which is relatively minor in comparison is that doing add and rm is two operations on the pinset, where an update could be coalesced into just one. Pinset modifications currently get expensive with a very large number of pins, and this might be noticeable.

@whyrusleeping
Copy link
Member Author

cc @ianopolous for further usecase discussion

@kevina
Copy link
Contributor

kevina commented Mar 15, 2017

This sounds fairly easy and self-contained. @whyrusleeping is this what you had in mind for the syntax?

ipfs pin update <OLD> <NEW>

@kevina kevina self-assigned this Mar 15, 2017
@whyrusleeping
Copy link
Member Author

@kevina Yeap! That is indeed

@whyrusleeping whyrusleeping added this to the Ipfs 0.4.8 milestone Mar 15, 2017
@Kubuxu Kubuxu added the kind/enhancement A net-new feature or improvement to an existing feature label Mar 15, 2017
@kevina
Copy link
Contributor

kevina commented Mar 16, 2017

Okay I will work on implementing it this week.

@kevina kevina added the status/in-progress In progress label Mar 16, 2017
@ianopolous
Copy link
Member

Let me give some more details around our use case:
Currently when a user commits a write to their file system in Peergos we have a 3 step process:

  1. pin the new file system root
  2. CAS in the new root into our IPNS equivalent
  3. unpin the old root

This allows us to guarantee to not be in an inconsistent state if the process aborts at any stage, including relative to arbitrary concurrent GCs. If the process aborts after 1, the IPNS is still pointing to the old root which is still pinned. If it aborts after 2, then we have committed the write, but just have an extra pin which is fine, and can be corrected later.

For us to be able to use this in step 1 it can't unpin the old root, that would still need to be step 3. That could be an option though, whether or not to unpin the old root. Thoughts? @whyrusleeping

@whyrusleeping
Copy link
Member Author

Yeah, we can definitely have a --unpin-old option that defaults to true.

@kevina
Copy link
Contributor

kevina commented Mar 16, 2017

Based on my understand how pin add and pin rm work currently the potential for saving is limited. In order to "skip traversing subtrees that are in the old graph since they are already pinned" we would have to know what those subtrees are, which will mean traversing the old graph. Currently as far as I know pin rm does not do that for recursive pins, so just to discover the pinned nodes we would likely need to traverse the graph anyway. Now if the new pin uses the root of the old pin, there is a savings there. Also if the new pin uses nodes at a shallow dept, for example if the graph was:

     a
  /   |  \
b    c   d
| 
e

and the new pin uses b c or d we could optimize the code to a depth limited traversal of the old pin to discover likely pinned subtrees to be used by the new pin. (My apologizes if I made a mistake in my analysis here.)

Let me know if you still think it is worth it.

Another option I consider is to rework the pinning code to support batch operations. I am not sure how much we can save here for this use case, but in the case when a full traversal of all recursive pins is required the savings can be significant.

@ianopolous
Copy link
Member

Our use case involves a btree. If our btree is depth n it has O(16^n) nodes. During a write, we only have to modify O(n) elements, from the new/modified element down to the root. So if you lazily traverse the old graph to discover already pinned sub-trees as you pin the new one, you will get reduction from O(16^n) to O(n) for the pin add component.

@whyrusleeping whyrusleeping modified the milestones: Ipfs 0.4.9, Ipfs 0.4.8 Mar 24, 2017
@whyrusleeping
Copy link
Member Author

whyrusleeping commented Mar 24, 2017

@kevina This is the optimization, lets say we have a pin on node a here.

     a
  /   |  \
b    c   d
| 
e

And lets say 'a' is a directory that we patch a new file into:

       a
  /   |  \   \
b    c   d   f
| 
e

Now, with ipfs pin update X Y we can start a traversal from both roots simultaneously. Since both roots have a child named 'b' with the same hash, we can skip going down that route. Now both roots also share c and d, so we can skip those as well. The only difference is the new pin has 'f' and the old one does not. So we will have to traverse through 'f' to ensure all those blocks are around.

The savings here get much high the larger the graphs are, this is a very common thing, and it will actually make mfs changes MUCH faster

@ianopolous
Copy link
Member

The relevant PR is #3846

@kevina kevina removed their assignment Apr 16, 2017
@kevina
Copy link
Contributor

kevina commented Apr 16, 2017

Removing my assignment. @whyrusleeping decided to do the p.r.

@whyrusleeping
Copy link
Member Author

PR is up, will likely merge in 0.4.10

@whyrusleeping whyrusleeping modified the milestones: Ipfs 0.4.10, Ipfs 0.4.9 May 8, 2017
@magik6k magik6k modified the milestones: Ipfs 0.4.10, Ipfs 0.4.11 Jul 28, 2017
@magik6k
Copy link
Member

magik6k commented Aug 18, 2017

closing as #3846 has been merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature status/in-progress In progress
Projects
None yet
Development

No branches or pull requests

5 participants