Pass cids instead of nodes around in EnumerateChildrenAsync #3598

Voker57 · 2017-01-14T18:08:42Z

Saves a whole lot of RAM on todobuffer

Voker57 · 2017-01-15T16:19:20Z

whyrusleeping · 2017-01-17T18:09:20Z

merkledag/merkledag.go

 			}

 			if next == nil {
-				next = nc.Node
-				send = feed
+				if len(n.Links()) > 0 {


This can be cleaned up a little bit, Lets add all the new links to the array first, then check for next being nil and handle that case separately.

whyrusleeping

I like this approach better than mine. One small comment, and then i'd also like @Kubuxu to review.

Kubuxu · 2017-01-18T13:26:08Z

merkledag/merkledag.go

-				send = feed
+				if len(n.Links()) > 0 {
+					next = n.Links()[0].Cid
+					for _, l := range n.Links()[1:] {


It might be beneficial to: preallocate array for Cids in .Links() call, write the Cids to that array and do just one append call.

This way the todobuffer will be expanded just once instead of N times.

In general this isnt an issue in go, slices will optmistically allocate extra space under the hood for you, so sequential appends are generally just as performant as preallocating.

Kubuxu · 2017-01-18T15:52:33Z

merkledag/merkledag.go

 			} else {
-				todobuffer = append(todobuffer, nc.Node)
+				for _, l := range n.Links() {
+					todobuffer = append(todobuffer, l.Cid)


Same (https://github.com/ipfs/go-ipfs/pull/3598/files#r96632953) applies here.

The allocation of the additional array will probably be removed by optimization step in golang.

Voker57 · 2017-01-19T12:39:43Z

Addressed both reviews.

Kubuxu · 2017-01-19T22:17:57Z

Can you also change the base of the PR to master, as the branch you are currently pointing to was merged already.

Voker57 · 2017-01-20T17:37:13Z

done

whyrusleeping · 2017-02-12T01:43:49Z

merkledag/merkledag.go

-	feed := make(chan node.Node)
-	out := make(chan *NodeOption)
+	feed := make(chan *cid.Cid)
+	out := make(chan *cid.Cid)
 	done := make(chan struct{})

 	var setlk sync.Mutex

 	for i := 0; i < FetchGraphConcurrency; i++ {


Why are we spawning multiple of these goroutines? Looking at it, we should only need one.

you mean FetchGraphConcurrency ones? You're right, looks like I accidentally removed concurrency altogether :) Fixed version incoming

whyrusleeping · 2017-02-12T01:44:40Z

merkledag/merkledag.go

-				todobuffer = append(todobuffer, nc.Node)
+			links := n.Links()
+			if len(links) > 0 {
+				cids := make([]*cid.Cid, len(links), len(links))


no need to specify capacity if youre already specifying length to be the same

Actually, you can rewrite this whole loop to avoid allocating an extra slice. Just append to the todobuffer

Voker57 · 2017-02-13T14:20:56Z

Modified PR to actually be concurrent. Is second changes' request obsolete now?

whyrusleeping · 2017-02-14T06:15:09Z

merkledag/merkledag.go

+
+				if unseen {
+					cids := make([]*cid.Cid, len(n.Links()))
+					for i, l := range n.Links() {


This seems unecessary, why not just send the node down through out?

my worry is that its an extra allocation per node that we could avoid by simply moving this for loop down into the processing below.

whyrusleeping · 2017-02-14T06:18:39Z

merkledag/merkledag.go

-				send = feed
-			} else {
-				todobuffer = append(todobuffer, nc.Node)
+		case cids := <-out:


if out was changed to be a channel of nodes, this could look like:

case nd := <-out: for _, lnk := range nd.Links() { if next == nil { next = lnk.Cid send = feed continue } todobuffer = append(todobuffer, lnk.Cid) }

whyrusleeping · 2017-02-14T06:19:46Z

merkledag/merkledag.go

-
+
+	errChan := make(chan error)
+	fetchersCtx, cancel := context.WithCancel(ctx)


put a defer cancel() after this call and then remove the other call youre making to cancel

License: MIT Signed-off-by: Iaroslav Gridin <voker57@gmail.com>

Voker57 · 2017-02-14T12:10:14Z

Implemented suggested changes.

whyrusleeping

LGTM, Tested locally and can confirm its much faster already :)

No idea why this was changed this was introduced in: 08f342e (part of #3598) License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Pass cids instead of nodes around in EnumerateChildrenAsync

No idea why this was changed this was introduced in: 08f342e (part of ipfs#3598) License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Voker57 mentioned this pull request Jan 14, 2017

Move fetcher feeding to separate goroutine to avoid deadlocking #3550

Closed

whyrusleeping reviewed Jan 17, 2017

View reviewed changes

whyrusleeping requested changes Jan 17, 2017

View reviewed changes

whyrusleeping requested a review from Kubuxu January 17, 2017 20:15

whyrusleeping added this to the ipfs 0.4.6 milestone Jan 17, 2017

Kubuxu reviewed Jan 18, 2017

View reviewed changes

Voker57 force-pushed the feat/frugal-enumerate branch 4 times, most recently from c978321 to 71da359 Compare January 19, 2017 12:39

Voker57 changed the base branch from feat/better-enum-async to master January 20, 2017 17:37

Kubuxu approved these changes Jan 23, 2017

View reviewed changes

Voker57 mentioned this pull request Jan 28, 2017

Link caching #3642

Closed

whyrusleeping reviewed Feb 12, 2017

View reviewed changes

Voker57 force-pushed the feat/frugal-enumerate branch from 71da359 to a63b4c2 Compare February 13, 2017 14:19

whyrusleeping reviewed Feb 14, 2017

View reviewed changes

Voker57 added 2 commits February 14, 2017 14:09

Pass cids instead of nodes around in EnumerateChildrenAsync

08f342e

License: MIT Signed-off-by: Iaroslav Gridin <voker57@gmail.com>

Re-enable async children enumerating in FetchGraph

6618932

License: MIT Signed-off-by: Iaroslav Gridin <voker57@gmail.com>

Voker57 force-pushed the feat/frugal-enumerate branch from a63b4c2 to 6618932 Compare February 14, 2017 12:09

whyrusleeping approved these changes Feb 14, 2017

View reviewed changes

whyrusleeping merged commit a2f27e1 into ipfs:master Feb 14, 2017

Voker57 deleted the feat/frugal-enumerate branch February 14, 2017 23:11

kevina mentioned this pull request Feb 15, 2017

Ipfs pin add does not fetch raw leaves and thus pins incomplete dag #3688

Closed

whyrusleeping mentioned this pull request Feb 17, 2017

ipfs pin add is very slow / hanging #3505

Closed

Voker57 restored the feat/frugal-enumerate branch November 1, 2017 14:06

Stebalien added a commit that referenced this pull request Mar 27, 2018

only visit nodes in EnumerateChildrenAsync when asked

08388e7

No idea why this was changed this was introduced in: 08f342e (part of #3598) License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien mentioned this pull request Mar 27, 2018

only visit nodes in EnumerateChildrenAsync when asked #4885

Merged

Voker57 deleted the feat/frugal-enumerate branch June 4, 2019 08:41

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

Merge pull request ipfs#3598 from Voker57/feat/frugal-enumerate

2f30bcd

Pass cids instead of nodes around in EnumerateChildrenAsync

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass cids instead of nodes around in EnumerateChildrenAsync #3598

Pass cids instead of nodes around in EnumerateChildrenAsync #3598

Voker57 commented Jan 14, 2017

Voker57 commented Jan 15, 2017

whyrusleeping Jan 17, 2017

whyrusleeping left a comment

Kubuxu Jan 18, 2017

whyrusleeping Jan 20, 2017

Kubuxu Jan 18, 2017

Voker57 commented Jan 19, 2017

Kubuxu commented Jan 19, 2017

Voker57 commented Jan 20, 2017

whyrusleeping Feb 12, 2017

Voker57 Feb 13, 2017

whyrusleeping Feb 12, 2017

whyrusleeping Feb 12, 2017

Voker57 commented Feb 13, 2017

whyrusleeping Feb 14, 2017

whyrusleeping Feb 14, 2017

whyrusleeping Feb 14, 2017

whyrusleeping Feb 14, 2017

Voker57 commented Feb 14, 2017

whyrusleeping left a comment



		errChan := make(chan error)
		fetchersCtx, cancel := context.WithCancel(ctx)

Pass cids instead of nodes around in EnumerateChildrenAsync #3598

Pass cids instead of nodes around in EnumerateChildrenAsync #3598

Conversation

Voker57 commented Jan 14, 2017

Voker57 commented Jan 15, 2017

Choose a reason for hiding this comment

whyrusleeping left a 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

Voker57 commented Jan 19, 2017

Kubuxu commented Jan 19, 2017

Voker57 commented Jan 20, 2017

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

Voker57 commented Feb 13, 2017

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

Voker57 commented Feb 14, 2017

whyrusleeping left a comment

Choose a reason for hiding this comment