Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify sockopts #230

Merged
merged 12 commits into from
Dec 1, 2021
Merged

Simplify sockopts #230

merged 12 commits into from
Dec 1, 2021

Conversation

9seconds
Copy link
Owner

Please take a look #208, #220 and #229

From early days mtg was not using TCP keepalive in a favor of its own "connection aborter" and allowing to tune buffer size. This was made with a purpose: I didn't find TCP keepalive to work as I wanted and we all spent so many time to find out the best buffer size so I've even made it configurable. Right now both of them look redundand for me and moreover, complexifies everything a lot. They are hackish.

This PR is an attempt to start wiping them out and I hope, this will also help us to achieve the better performance. I'm going to test it out in various conditions (fortunately or not, I'm going to connect to many different networks in forecoming weeks) to see how it performs and behaves. If I won't find any problem, I'll merge.

Any assistance or testing is also appreciated.

@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2021

Codecov Report

Merging #230 (e7416bc) into master (bae5407) will decrease coverage by 1.72%.
The diff coverage is 64.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   84.20%   82.47%   -1.73%     
==========================================
  Files          68       69       +1     
  Lines        1868     1929      +61     
==========================================
+ Hits         1573     1591      +18     
- Misses        210      237      +27     
- Partials       85      101      +16     
Impacted Files Coverage Δ
internal/config/config.go 36.36% <ø> (ø)
internal/config/parse.go 84.61% <ø> (ø)
internal/utils/net_listener.go 0.00% <0.00%> (ø)
mtglib/conns.go 100.00% <ø> (ø)
mtglib/internal/faketls/conn.go 93.33% <ø> (ø)
mtglib/internal/obfuscated2/conn.go 81.81% <ø> (ø)
mtglib/proxy_opts.go 88.88% <ø> (+2.22%) ⬆️
mtglib/stream_context.go 91.30% <ø> (ø)
network/circuit_breaker.go 76.92% <ø> (-7.70%) ⬇️
network/load_balanced_socks5.go 100.00% <ø> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bae5407...e7416bc. Read the comment docs.

@EkkoG
Copy link

EkkoG commented Nov 28, 2021

I'm trying to test this PR with daily usage.

@EkkoG
Copy link

EkkoG commented Nov 29, 2021

With this PR, the performance is much better now.

Legacy

mtg is slower than official MTProxy about 20%, official version 320ms, mtg 390 ms

Download speed

with the same server and same video resource.

official version 3.0M/s, mtg 2.5M/s, also about 20% slower.

@9seconds
Copy link
Owner Author

Thanks. Seems we are on the right track.

@9seconds
Copy link
Owner Author

@EkkoG i'm going to tryout a couple of other options. I'm curious about your testing methodology, would you mind to share how you test a throughput?

@9seconds
Copy link
Owner Author

9seconds commented Dec 1, 2021

Yet another big update. Seems I've made a couple of other improvements, that improved performance a lot. At least, first results are encouraging.

@9seconds 9seconds merged commit 3f8f96b into master Dec 1, 2021
@9seconds 9seconds deleted the simplify-sockopts branch December 1, 2021 12:03
@database64128
Copy link

  1. There's no need to set TCP keepalive in mtg, because Go already sets it by default with a 15 second interval.
  2. Calling net.TCPConn.CloseRead(), or the shutdown(SHUT_RD) syscall on a TCP socket doesn't actually do anything, other than making subsequent read(2) calls fail, which shouldn't happen anyway because there won't be any subsequent read(2) calls. Calling net.TCPConn.CloseWrite() or shutdown(SHUT_WR) alone is enough, which sends FIN to terminate data transfer of this direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants