-
Notifications
You must be signed in to change notification settings - Fork 21
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
write benchmarks #77
write benchmarks #77
Conversation
updated benchmarks to consider a case where This causes somewhat unbelievable benchmarks at present:
|
Hm, yeah, something is fishy. Does the lossy packet conn you're using correctly simulate packet loss for a TCP connection? That is, we don't expect data to actually be lost, we just expect things to slow down due to retransmissions. |
The wrapper around the net.Conn blocks the sending thread with a Sleep based on packet size (to cap bandwidth) and latency. It is configured in these benchmarks to not drop any packets. |
Ah, got it. |
c037901
to
4104ff0
Compare
ok, some more reasonable numbers: (a 100kbps link with 30ms latency)
Interesting that the 4-core (more contention) test starts falling off link saturation in the write coalescing case. I'll re-run with a faster network and see how that trends. |
This reverts commit 759e03c.
This is now using libp2p/go-libp2p-testing#21 I believe the top numbers are actually due to imprecision in how go bench reports work done by parallel threads vs overall. I now no longer believe any meaningful change is incurred in mplex from write coalescing. |
474eab7
to
2cf04b5
Compare
The current test failure is a good indication that this is actually testing something meaningful:
I'll push an update to indicate that a 10% slowdown (vs the current 1% limit) is acceptable, since that's what the current code hits on CI (this may also be due to CI running the test with the race detector on) |
for comparison, the current local output
(without)
|
Feedback from sync call:
Write coalescing is targeting jitter, packet-loss, and syscall overhead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits but otherwise the benchmarks look sane.
benchmarks_test.go
Outdated
} | ||
b.SetBytes(int64(receivedBytes)) | ||
defer mpa.Close() | ||
defer mpb.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why defer?
benchmarks_test.go
Outdated
go func() { | ||
defer wg.Done() | ||
lb, _ = slowL.Accept() | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return?
if err != nil { | ||
b.Error(err) | ||
} | ||
defer la.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will panic if err != nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error above should exit already in that case, right?
@willscott could you re-disable write-coalescing so we can get these merged? |
rebased to remove |
Work on #76
Initially, running as
go test -bench . -benchtime 15s -cpu 1,2,4
yields(Without write coalescing):
With Write Coalescing:
This is the expected failure mode though, since we're representing a 0-latency, infinite-bandwidth network.