Skip to content

Commit 18325c9

Browse files
hsanjuanhsanjuan
committed
Fix #3783: Improve IsPinned() lookups for indirect pins
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]>
1 parent 7d02274 commit 18325c9

File tree

2 files changed

+146
-21
lines changed

2 files changed

+146
-21
lines changed

‎pin/pin.go

+13-9
Original file line numberDiff line numberDiff line change
@@ -276,8 +276,9 @@ func (p *pinner) isPinnedWithType(c *cid.Cid, mode PinMode) (string, bool, error
276276
}
277277

278278
// Default is Indirect
279+
visitedSet := cid.NewSet()
279280
for _, rc := range p.recursePin.Keys() {
280-
has, err := hasChild(p.dserv, rc, c)
281+
has, err := hasChild(p.dserv, rc, c, visitedSet.Visit)
281282
if err != nil {
282283
return "", false, err
283284
}
@@ -519,7 +520,9 @@ func (p *pinner) PinWithMode(c *cid.Cid, mode PinMode) {
519520
}
520521
}
521522

522-
func hasChild(ds mdag.LinkService, root *cid.Cid, child *cid.Cid) (bool, error) {
523+
// hasChild recursively looks for a Cid among the children of a root Cid.
524+
// The visit function can be used to shortcut already-visited branches.
525+
func hasChild(ds mdag.LinkService, root *cid.Cid, child *cid.Cid, visit func(*cid.Cid) bool) (bool, error) {
523526
links, err := ds.GetLinks(context.Background(), root)
524527
if err != nil {
525528
return false, err
@@ -529,14 +532,15 @@ func hasChild(ds mdag.LinkService, root *cid.Cid, child *cid.Cid) (bool, error)
529532
if lnk.Cid.Equals(child) {
530533
return true, nil
531534
}
535+
if visit(c) {
536+
has, err := hasChild(ds, c, child, visit)
537+
if err != nil {
538+
return false, err
539+
}
532540

533-
has, err := hasChild(ds, c, child)
534-
if err != nil {
535-
return false, err
536-
}
537-
538-
if has {
539-
return has, nil
541+
if has {
542+
return has, nil
543+
}
540544
}
541545
}
542546
return false, nil

‎pin/pin_test.go

+133-12
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,17 @@ func assertPinned(t *testing.T, p Pinner, c *cid.Cid, failmsg string) {
3535
}
3636
}
3737

38+
func assertUnpinned(t *testing.T, p Pinner, c *cid.Cid, failmsg string) {
39+
_, pinned, err := p.IsPinned(c)
40+
if err != nil {
41+
t.Fatal(err)
42+
}
43+
44+
if pinned {
45+
t.Fatal(failmsg)
46+
}
47+
}
48+
3849
func TestPinnerBasic(t *testing.T) {
3950
ctx := context.Background()
4051

@@ -70,13 +81,12 @@ func TestPinnerBasic(t *testing.T) {
7081

7182
// Create new node b, to be parent to a and c
7283
b, _ := randNode()
73-
err = b.AddNodeLink("child", a)
74-
if err != nil {
84+
85+
if err = b.AddNodeLink("child", a); err != nil {
7586
t.Fatal(err)
7687
}
7788

78-
err = b.AddNodeLink("otherchild", c)
79-
if err != nil {
89+
if err = b.AddNodeLink("otherchild", c); err != nil {
8090
t.Fatal(err)
8191
}
8292

@@ -86,8 +96,7 @@ func TestPinnerBasic(t *testing.T) {
8696
}
8797

8898
// recursively pin B{A,C}
89-
err = p.Pin(ctx, b, true)
90-
if err != nil {
99+
if err = p.Pin(ctx, b, true); err != nil {
91100
t.Fatal(err)
92101
}
93102

@@ -114,22 +123,19 @@ func TestPinnerBasic(t *testing.T) {
114123
}
115124

116125
// Add D{A,C,E}
117-
err = p.Pin(ctx, d, true)
118-
if err != nil {
126+
if err = p.Pin(ctx, d, true); err != nil {
119127
t.Fatal(err)
120128
}
121129

122130
dk := d.Cid()
123131
assertPinned(t, p, dk, "pinned node not found.")
124132

125133
// Test recursive unpin
126-
err = p.Unpin(ctx, dk, true)
127-
if err != nil {
134+
if err = p.Unpin(ctx, dk, true); err != nil {
128135
t.Fatal(err)
129136
}
130137

131-
err = p.Flush()
132-
if err != nil {
138+
if err = p.Flush(); err != nil {
133139
t.Fatal(err)
134140
}
135141

@@ -145,6 +151,121 @@ func TestPinnerBasic(t *testing.T) {
145151
assertPinned(t, np, bk, "could not find recursively pinned node")
146152
}
147153

154+
func TestIsPinnedLookup(t *testing.T) {
155+
// We are going to test that lookups work in pins which share
156+
// the same branches. For that we will construct this tree:
157+
//
158+
// A5->A4->A3->A2->A1->A0
159+
// / /
160+
// B------- /
161+
// \ /
162+
// C---------------
163+
//
164+
// We will ensure that IsPinned works for all objects both when they
165+
// are pinned and once they have been unpinned.
166+
aBranchLen := 6
167+
if aBranchLen < 3 {
168+
t.Fatal("set aBranchLen to at least 3")
169+
}
170+
171+
ctx := context.Background()
172+
dstore := dssync.MutexWrap(ds.NewMapDatastore())
173+
bstore := blockstore.NewBlockstore(dstore)
174+
bserv := bs.New(bstore, offline.Exchange(bstore))
175+
176+
dserv := mdag.NewDAGService(bserv)
177+
178+
// TODO does pinner need to share datastore with blockservice?
179+
p := NewPinner(dstore, dserv, dserv)
180+
181+
aNodes := make([]*mdag.ProtoNode, aBranchLen, aBranchLen)
182+
aKeys := make([]*cid.Cid, aBranchLen, aBranchLen)
183+
for i := 0; i < aBranchLen; i++ {
184+
a, _ := randNode()
185+
if i >= 1 {
186+
err := a.AddNodeLink("child", aNodes[i-1])
187+
if err != nil {
188+
t.Fatal(err)
189+
}
190+
}
191+
192+
ak, err := dserv.Add(a)
193+
if err != nil {
194+
t.Fatal(err)
195+
}
196+
//t.Logf("a[%d] is %s", i, ak)
197+
aNodes[i] = a
198+
aKeys[i] = ak
199+
}
200+
201+
// Pin A5 recursively
202+
err := p.Pin(ctx, aNodes[aBranchLen-1], true)
203+
if err != nil {
204+
t.Fatal(err)
205+
}
206+
207+
// Create node B and add A3 as child
208+
b, _ := randNode()
209+
err = b.AddNodeLink("mychild", aNodes[3])
210+
if err != nil {
211+
t.Fatal(err)
212+
}
213+
214+
// Create C node
215+
c, _ := randNode()
216+
// Add A0 as child of C
217+
err = c.AddNodeLink("child", aNodes[0])
218+
if err != nil {
219+
t.Fatal(err)
220+
}
221+
222+
// Add C
223+
ck, err := dserv.Add(c)
224+
if err != nil {
225+
t.Fatal(err)
226+
}
227+
//t.Logf("C is %s", ck)
228+
229+
// Add C to B and Add B
230+
err = b.AddNodeLink("myotherchild", c)
231+
if err != nil {
232+
t.Fatal(err)
233+
}
234+
bk, err := dserv.Add(b)
235+
if err != nil {
236+
t.Fatal(err)
237+
}
238+
//t.Logf("B is %s", bk)
239+
240+
// Pin C recursively
241+
err = p.Pin(ctx, c, true)
242+
if err != nil {
243+
t.Fatal(err)
244+
}
245+
246+
// Pin B recursively
247+
err = p.Pin(ctx, b, true)
248+
if err != nil {
249+
t.Fatal(err)
250+
}
251+
252+
assertPinned(t, p, aKeys[0], "A0 should be pinned")
253+
assertPinned(t, p, aKeys[1], "A1 should be pinned")
254+
assertPinned(t, p, ck, "C should be pinned")
255+
assertPinned(t, p, bk, "B should be pinned")
256+
257+
// Unpin A5 recursively
258+
err = p.Unpin(ctx, aKeys[5], true)
259+
assertPinned(t, p, aKeys[0], "A0 should still be pinned through B")
260+
assertUnpinned(t, p, aKeys[4], "A4 should be unpinned")
261+
262+
// Unpin B recursively
263+
err = p.Unpin(ctx, bk, true)
264+
assertUnpinned(t, p, bk, "B should be unpinned")
265+
assertUnpinned(t, p, aKeys[1], "A1 should be unpinned")
266+
assertPinned(t, p, aKeys[0], "A0 should still be pinned through C")
267+
}
268+
148269
func TestDuplicateSemantics(t *testing.T) {
149270
ctx := context.Background()
150271
dstore := dssync.MutexWrap(ds.NewMapDatastore())

0 commit comments

Comments
 (0)