Fix #3783: Improve IsPinned() lookups for indirect pins #3809
Conversation
hsanjuan
added
the
kind/enhancement
hsanjuan
added
status/in-progress
| for _, rc := range p.recursePin.Keys() { | ||
| has, err := hasChild(p.dserv, rc, c) | ||
| has, err := hasChild(p.dserv, rc, c, visitedSet.Visit) |
There was a problem hiding this comment.
I dont think we can reuse the same visitedSet across pin checks, unless we pass all the pins we're checking into the call at once. otherwise we might mark a subtree as visited that contains the next pin we're checking
There was a problem hiding this comment.
Oh wait a second, i misread. 'c' is the thing we're looking for, and it remains the same throughout the entire operation. This LGTM then
There was a problem hiding this comment.
@whyrusleeping well, don't you need to revoke the "request changes" review or something? I have no idea if that is possible though.
There was a problem hiding this comment.
Hrm... i think it wants me to submit another review with 'changed approved'
| func hasChild(ds mdag.LinkService, root *cid.Cid, child *cid.Cid) (bool, error) { | ||
| // hasChild recursively looks for a Cid among the children of a root Cid. | ||
| // The visit function can be used to shortcut already-visited branches. | ||
| func hasChild(ds mdag.LinkService, root *cid.Cid, child *cid.Cid, visit func(*cid.Cid) bool) (bool, error) { |
There was a problem hiding this comment.
Why not make a type with the signature of go func(*cid.Cid) bool ?
There was a problem hiding this comment.
Because it creates an extra indirection which does not help to figure out what this is doing any more than having the definition directly as parameter. Moreover, this is the only parameter which uses that in the module, it's not hard to read and it is called from a single place which clearly shows that expected usage is to pass cid.Set.Visit function.
There was a problem hiding this comment.
At least change the hasChild func format in someting like this.
func hasChild(
ds mdag.LinkService,
root *cid.Cid,
child *cid.Cid,
visit func(*cid.Cid) bool,
) (bool, error) {
//body
}This should be documented somewhere to enforce a number of columns a func can have.
There was a problem hiding this comment.
I do not believe this is done anywhere else in the IPFS code base.
There was a problem hiding this comment.
Yeah, in general I don't mind having the function declaration be long. Simply breaking at the most convenient place onto the next line is fine. Breaking it out into separate lines per parameter brings back unpleasant memories of really really old C code.
There was a problem hiding this comment.
I don't like going over 80 chars, but if the codebase style keeps declarations in the same line, I'll leave it as it is for the moment because I like consistency.
|
I'll add an extra test with a shared tree |
hsanjuan
force-pushed
the
faster-pin-ls
branch
2 times, most recently
from
5632d9a to
41e3656
Compare
|
@whyrusleeping @hoenirvili I have added an extra test. |
pin/pin_test.go
Outdated
|
|
||
| // Create node B and add A3 as child | ||
| b, _ := randNode() | ||
| err = b.AddNodeLink("mychild", aNodes[3]) |
There was a problem hiding this comment.
Can you make all error checks one line like if err := func(); err != nil { //code
There was a problem hiding this comment.
@hsanjuan I think you changed the error checks in the wrong test, lol
pin/pin_test.go
Outdated
| assertPinned(t, p, bk, "B should be pinned") | ||
|
|
||
| // Unpin A5 recursively | ||
| err = p.Unpin(ctx, aKeys[5], true) |
There was a problem hiding this comment.
check this error (and the one below)
|
@hsanjuan One more comment about changing the error checking in the wrong test |
|
@whyrusleeping sorry! I have fixed it. Honestly hope I haven't broken something else. |
This avoids revisiting already-searched branches and cut down the IsPinned() check times considerably when recursive pins share big underlying DAGs. A test has been added which double-checks that pinned and unpinned items lookups respond as expected with shared branches. License: MIT Signed-off-by: Hector Sanjuan <[email protected]>
This avoids revisiting already-searched branches and cut down
the IsPinned() check times considerably when recursive pins
share big underlying DAGs. Same mechanism as
merkledag.EnumerateChildren().Here are some benchmarks with a randomly-selected list of 10 of my pins (and bad pins):
This runs
ipfs pin ls $pinfor each of the listed pins and measures how long it takes it.With 0.4.7:
With this patch: