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

use bufio for mse write #960

Closed
wants to merge 4 commits into from
Closed

use bufio for mse write #960

wants to merge 4 commits into from

Conversation

trim21
Copy link

@trim21 trim21 commented Jul 10, 2024

there is no need to start a new goroutine for writing small chunks.

If you want concurrently read write we can use chan, using slice, lock and cond to mock a chan doesn't sound like a good idea.

@anacrolix
Copy link
Owner

It's very old code, I'm sure I'd do it differently now.

Channels are usually over used in Go, they're also much worse for performance than people realise. I doubt this situation is affected.

Could you provide a benchmark to demonstrate the improvement?

Thanks for the contribution!

@trim21
Copy link
Author

trim21 commented Jul 11, 2024

A local echo server.

before:
cpu: Intel(R) Pentium(R) Gold G6405 CPU @ 4.10GHz
BenchmarkMSE-4 4141 301669 ns/op 35309 B/op 220 allocs/op

after:
cpu: Intel(R) Pentium(R) Gold G6405 CPU @ 4.10GHz
BenchmarkMSE-4 4305 292442 ns/op 42028 B/op 207 allocs/op

@trim21
Copy link
Author

trim21 commented Jul 11, 2024

emm I guess current implment is right. using bufio may cause a dead lock problem.

We should create a new goroutine to do the writing.

@trim21 trim21 closed this Jul 11, 2024
@anacrolix
Copy link
Owner

anacrolix commented Jul 12, 2024

It's important that people test assumptions still hold true old code like this., so thanks for the effort and the conclusion.

@trim21 trim21 deleted the bufio branch July 12, 2024 10:57
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.

2 participants