Help review and get DHT pipelining merged (go-libp2p-kad-dht)

[laser]

Split out from @whyrusleeping 's original catch-all issue.

Description

vyzo's DHT Request Pipelining story appears to be code-complete, but requires additional tests in order for @whyrusleeping to feel good about merging it:

@vyzo what would make me feel better about merging this, is having a test that tests delayed responses. We could do this by using a datastore with delays (https://github.com/ipfs/go-datastore/blob/master/delayed/delayed.go) and asserting that the requests in aggregate took below a certain amount of time.

[laser]

PR which adds tests to go-libp2p-kad-dht here: libp2p/go-libp2p-kad-dht#149

[laser]

I've rebased vyzo's branch onto master and fixed some conflicts. As per my conversation with @whyrusleeping , changes introduced in commit 97741ed has impacted the performance of the request pipelining branch. Specifically, all PutValue requests were finishing in ~19 seconds (pre-rebase) and are now taking ~37 seconds (on my machine).

I will spend the remainder of the day looking through the contents of 97741ed in order to get some insights into why the changeset slowed down PutValue calls.

[laser]

I've run some tests and found some interesting data:

| Branch                                | PutValue    |             | GetValue    |              | Total       |             | 
|---------------------------------------|-------------|-------------|-------------|--------------|-------------|-------------| 
|                                       | mean (sec)  | stdev       | mean        | stdev        | mean        | stdev       | 
| master                                | 64.50748129 | 10.64644363 | 19.69437027 | 11.29886723  | 87.42311111 | 10.64644363 | 
| feat/request-pipeline (rebased)       | 63.20182138 | 11.4527859  | 21.17909721 | 17.55022852  | 86.2031     | 11.4527859  | 
| feat/request-pipeline (before rebase) | 22.43344761 | 2.43136656  | 6.437939408 | 0.5471811575 | 29.3784     | 2.43136656  | 

https://docs.google.com/spreadsheets/d/1f4fWeepjNOAOh_-eEGREkxcyRCjxfFxCQGHvPvATL3o/edit?usp=sharing

I ran my perf test 10 times with requests=60 against the master branch (no pipelining), pipelining branch (pre-rebase) and pipelining (post-rebase). The data show that, post-rebase, the pipelining branch doesn't perform much better than master (which doesn't include pipelining).

[whyrusleeping]

Good find, I think you talked to @Stebalien and are moving forward with a different method of simulating latency, right?

[Stebalien]

Yes. The intrusive (not suitable for test cases) method I tried (introducing ~100ms of latency in the handler loop) showed a ~5x performance increase on rights and ~6x on reads.

[laser]

I just pushed an update to the PR's branch which replaces the delayed datastore with a Mocknet configured with 100ms of latency.

libp2p/go-libp2p-kad-dht#92

[mishmosh]

Cole will review on Wed 6/13.

[dignifiedquire]

@laser @acruikshank is this still blocked?

[laser]

@dignifiedquire - Just spoke with Cole about this. He said:

was actually just talking about this. essentially no one picked up the ball on this one... it's quite close so i'll make the stylistic changes and we should be ogod

[laser]

@mishmosh - Can you help me figure out how to get this thing merged? I could use the help.

[phritz]

@laser I'll get @mgoelzer on it

[mishmosh]

Almost-Halloween is a great time for PRs to come back from the dead 👻. Got 75% of the way through this recap before @mgoelzer was summoned, so here it is anyway. I'll watch for pings, but otherwise over to you @mgoelzer.

IIUC here is the state of things:

• @vyzo created this DHT Request pipelining libp2p/go-libp2p-kad-dht#92 (DHT Request Pipelining). During review, a different approach to testing was requested.
• @laser did that work in Feat/request pipeline tests libp2p/go-libp2p-kad-dht#149, which was merged into PR92's libp2p:feat/request-pipeline branch (but not master).
• In August, @bigs and @vyzo agreed PR92 was good to go.
• In September, when @bigs and @vyzo started to rebase and merge. They ran into 2 issues, listed below.

Current blockers for libp2p/go-libp2p-kad-dht#92:

1. Failing tests
2. bufferedDelimitedWriter disappearance (details here)

Does it make sense for @laser to investigate? Or someone from libp2p land?

[whyrusleeping]

let's try and get someone from libp2p-land to investigate, but its not a super high priority, so not too much pressure on them.

[ghost]

Hi @phritz @mishmosh @laser, I did some digging into this and I think that @vyzo probably needs to be the one to rebase this. It changes code in places that have since changed in master, so it's going to be a complex rebase.

One question: is there a particular reason why this bubbled up again recently? We're definitely aware of sporadic instances of extremely slow DHT lookups (on the order of minutes), but our solution to that is to instrument the code and run it in a test harness to understand what's happening before making any changes. Right now, we don't have any data to suggest it's related to this particular lock. By the way, if you all have a reproducible case where the DHT is either slow or loses values, we would love to study that.

[laser]

Hi @mgoelzer

One question: is there a particular reason why this bubbled up again recently?

Not a particularly-good one. I noticed that the issue was still assigned to me and had been for several weeks in spite of me having no plan to move the work forward.

Thanks again for following up.

[ghost]

@vyzo What's the status on this?

[vyzo]

@anacrolix is taking the torch on merging; we will remove the timing test and fix the regression before merging.

[phritz]

@ZenGround0 assigning over to you as the person with the relationship on the gfc side

[anacrolix]

I'm actively working on this now. I'm hoping to get it wrapped up soon.

[raulk]

Update: @anacrolix has been working on an alternative pool-based approach to achieve the same effect (enable parallel requests to the same peer). Our tests show no worthwhile improvement under the 99th percentile, under test loads. See discussion in the linked issue for more details.

We should validate these results with more realistic loads (e.g. within an IPFS node or a DHT booster).