share: handle rows concurrently

[vgonkivs]

• Handle multiple rows concurrently;

Closes #184

[Wondertan]

@vgonkivs, thank you for working on this. Unfortunately, this is wrong and I'll try to explain why.

[renaynay]

@vgonkivs Thank you very much for this PR - would you be able to take a first shot at writing a test for this?

[liamsi]

Please revisit this: https://github.com/celestiaorg/celestia-node/pull/241/files#r760551422

I think it would be good to add a benchmark that shows that this parallelization is justified (though it is relatively obvious that it should be faster but still).

would you be able to take a first shot at writing a test for this?

The benchmark could also serve as a test. The current tests do not yet test multiple rows (thx @renaynay for point this out).

celestia-node/service/share/share_test.go

Line 33 in 31cdabc

func TestService_GetSharesByNamespace(t *testing.T) {

[vgonkivs]

Hi @renaynay , @liamsi . Thanks for the idea to add benchmarks and additional tests. I will try to provide them within next week.

[Wondertan]

I think it would be good to add a benchmark that shows that this parallelization is justified (though it is relatively obvious that it should be faster but still).

@liamsi, the optimization is not CPU bound, i.e. we don't parallelize something calculation intensive, but we parallelize IO bound logic, so parallelization is furthermore a wrong word here, while concurrency is a right one(like it is said in PR name). Thus, we might not see any performance improvements and even performance degradation when we mock out network responses.

Particularly, here we start traversing and requesting each row concurrently, instead of doing them in order. Imagine sending 10 network requests in order and compare this to spawning 10 concurrent goroutines. In order requires for n+1 request to wait for n response to land first, while sending concurrently allows requests to be sent independently. In the mock environment, where responses are immediate, goroutines may only add overhead, as there are no responses to wait for.

[vgonkivs]

Thanks for the explanation @Wondertan. I was a bit confused after I had received the results - the code really works slower on mock.
@liamsi , I have update PR with additional test and benchmark.

[Wondertan]

👍🏻

[adlerjohn]

Waiting on review from @liamsi

[renaynay]

thanks so much for this @vgonkivs

The tests are failing on a flakey test, which we have documented here: #173

[Wondertan]

@liamsi required for the merged

[liamsi]

Looks good! Thank you @vgonkivs 🙏🏼

Also thanks @renaynay and @Wondertan for the reviews.