pin cmd: stream recursive pins

[MichaelMure]

Add a --stream flag to stream the results instead of
accumulating the final result in memory.

This is a rework of #5005

[MichaelMure]

Notes:

• pins are streamed and start being emitted immediately
• direct and recursive are streamed super fast first, around 76k pins/s on my test machine
• indirect pins are still streamed very slowly, around 5 pins/s, most likely due to no parallelization in go-merkledag: https://github.com/ipfs/go-merkledag/blob/master/merkledag.go#L287

[Stebalien]

This will need to correctly handle CID encoding (introduced since #5005).

Also note: we could also just send back a sequence of {cid: type} objects instead using a union (closer to ipfs ls --stream but definitely more expensive in terms of allocations). I don't feel strongly either way.

[Stebalien]

We're going to have to use "omitempty" for full backwards compatibility.

[MichaelMure]

This will need to correctly handle CID encoding (introduced since #5005).

Any pointer/example about this ? I don't really get what you are talking about.

[lanzafame]

@MichaelMure considering that this is a perf-orientated improvement, could this PR get a go benchmark written for it so we can monitor the on going performance of this call, especially since this is a critical path for large deployments of go-ipfs

[MichaelMure]

A few improvements:

• CID are encoded properly
• better struct naming (they were named after the refs command but are only used by pin/ls now)
• struct field are properly JSON omitempty-ed for backward compat
• output struct are documented to make codeclimate happy

[MichaelMure]

@lanzafame not sure how I would do a go benchmark for this. This PR doesn't fundamentally change anything perf wise, it just start to output the result as it comes. The total run time will be similar.

[MichaelMure]

More improvements:

• fixed a bug where indirect pins were reported as "all"
• output the recursive pins first as they don't require walking the graph
• use EnumerateChildrenAsync instead of EnumerateChildren for faster/parallel walking of the graph

root@ip-10-0-25-166:/tmp# timeout -k 1 59s ./ipfsseq pin ls --stream --type=indirect > indirectseq
12:50:51.021 ERROR       core: failure on stop:  context canceled builder.go:47
Killed
root@ip-10-0-25-166:/tmp# wc -l indirectseq
786 indirectseq
root@ip-10-0-25-166:/tmp# timeout -k 1 59s ./ipfsasync pin ls --stream --type=indirect > indirectasync
12:52:52.668 ERROR       core: failure on stop:  context canceled builder.go:47
Error: canceled
root@ip-10-0-25-166:/tmp# wc -l indirectasync
2645 indirectasync

For indirect pins:
Sequential walking: 13 pins/s
Parallel walking: 44 pins/s

--> ~3.4x faster !

It also looks like this change to use parallel graph walking might be used elsewhere. A quick search show that at least the GC might benefit from that.

[MichaelMure]

Test are now failing due to using EnumerateChildrenAsync. If EnumerateChildren is used instead, tests are ok (at least as far as I can tell, I have some unrelated failing on my machine).

[Stebalien]

is used instead, tests are ok (at least as far as I can tell, I have some unrelated failing on my machine).

We probably need to sort in the tests.

[MichaelMure]

FYI, I removed the commit that just change EnumerateChildren to EnumerateChildrenAsync. As you can see, the CI is now okay-ish. If you have insights on the remaining failure and why using EnumerateChildrenAsync could be problematic, I'm interested.

[Stebalien]

I'm guessing that the traversal order is random, causing the comparison against the expected pin list to fail. We need to sort both before comparing.

[MichaelMure]

I checked that, but the tests look ok to me:
https://github.com/ipfs/interop/blob/master/test/pin.js#L89-L90
https://github.com/ipfs/interop/blob/master/test/pin.js#L155-L157

[Stebalien]

So... this is a bug. EnumerateChildren enumerates children. EnumerateChildrenAsync also enumerates the root CID. I believe we should make it operate like EnumerateChildren but we're going to have to carefully make sure we don't break something in the process.

[MichaelMure]

Hoooo, that explains a lot. One problem I had after adding the recursive pins before the indirect pins to the set and using EnumerateChildrenAsync was that indirect pins would not be listed anymore. It was because the root cid was returned first by EnumerateChildrenAsync, considered as already enumerated and the whole subgraph was pruned.

[Stebalien]

Fix in #6499. Well, not really a fix, I just renamed these functions so that the names/documentation match up with what they do...

[lanzafame]

@lanzafame not sure how I would do a go benchmark for this. This PR doesn't fundamentally change anything perf wise, it just start to output the result as it comes. The total run time will be similar.

With a benchmark, you can run go test -run none -bencmark . -benchmem and you will get number of allocations etc.

[Stebalien]

LGTM but, as an API change, we'll need a signoff from @alanshaw as well.

[Stebalien]

@alanshaw this PR adds a --stream (short -s) option to ipfs pin ls that returns of pins as a stream of :

{"Cid": "Qm1", "Type": "direct"}
{"Cid": "Qm2", "Type": "recursive"}
...

Instead of a single:

{
  "Keys": {
    "Qm1": "direct",
    "Qm2": "recursive",
  }
}

This is completely backwards compatible but is likely something js-ipfs will want to implement. Thoughts? Objections?

[alanshaw]

Yes please. I want this. No objections.

[Stebalien]

🚀!

Thanks @MichaelMure, @lanzafame, and @alanshaw!

[MichaelMure]

Ha, I was waiting for #6499 (comment) to get resolved so I could have the neat 3.4x perf improvement with the parallel walking, but ok !

[Stebalien]

Test case in #6504.

Note: IMO, a benchmark isn't really useful here as this just streams the results, it doesn't reduce overall memory (still need to deduplicate results by storing them in a map).

[Stebalien]

Ha, I was waiting for #6499 (comment) to get resolved so I could have the neat 3.4x perf improvement with the parallel walking, but ok !

Let's do that in a followup. No reason to block this on a different change.