More and slower active connections in 7.1.x

[zwoop]

I think, but still working on confirming, that the commit in 425b696. The net effect is that

1. I'm seeing roughly twice as many active connections, even though it's pushing the exact same amount of Gbps.

2. Effectively, I think the average throughput / client is 1/2 of what it should be.

[shinrich]

So do you think this is making a real performance difference? Or are we just not timing out connections as quickly now?

I hadn't noticed a difference in number of active connections, but I'll keep an eye out for that in my testing.

[oknet]

Our product based on 6.0.x and backport some commits from master.

With the commit 425b696 , we did not found any vc leaks and performance decrease.

Can you show me how to re-produce it ?

[zwoop]

On our boxes, it was very noticeable. I can try again, but the net result was roughly 2-3x the number of active connections, and each transaction was 1/2 - 1/3 the speed (throughput in Mbps). This was measured for large downloads, greater than 1MB in object size.

Also, positive that the cause was 425b696, I tested it with and without this single PR, on 7.0.0.

@bryancall Didn't you reproduce / see the same issue with 425b696 ?

[bryancall]

I backed out the change on the 7.1.x branch and didn't see a difference with or without this change.

[zwoop]

I'm gonna try it again on our prod boxes, but it was very noticeable at high throughput (5-10Gbps).

[zwoop]

Yeh, I'm still seeing this. It's roughly half the throughput on each connection, which means I'm also seeing 2x the number of connections. Backing out 425b696 fixes it.

[keesspoelstra]

@zwoop Maybe this has something to do with it:

425b696#diff-907feb7aed591abaaf6cbd94105160f2R413

In read_disable and write_disable
vc->set_inactivity_timeout(0); is not the same as vc->next_inactivity_timeout_at = 0;
The former also resets the inactivity_timeout_in, which might prohibit the net_activity function from setting the inactivity value when stuff gets enabled again and thus not timing out. Or timing out on the active timeout, which might be later. I have not followed codepaths to check this.

[keesspoelstra]

@zwoop @bryancall when backing out this commit this pull for 6.2.1 might become applicable to 7.1 #1675

[zwoop]

I backed out this from 7.1.x, but this is still on master, so we still have to address that there. I'm ok with a fix, or reverting as well. @oknet Any ideas ?