Use sync.Cond for packetio.Buffer

[paulwe]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 98.59155% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.22%. Comparing base (8754ef1) to head (5f49dbf).

Files	Patch %	Lines
deadline/deadline.go	96.96%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
+ Coverage   83.12%   83.22%   +0.10%     
==========================================
  Files          39       39              
  Lines        2720     2737      +17     
==========================================
+ Hits         2261     2278      +17     
+ Misses        334      333       -1     
- Partials      125      126       +1     

Flag	Coverage Δ
go	83.13% <98.59%> (+0.18%)	⬆️
wasm	65.38% <95.77%> (+0.32%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[edaniels]

@paulwe after sleeping on it, I think I prefer what we landed on last night, sticking to channels. If needed in the future, it will allow us to use context as well pretty easily which would be harder to slot into this solution.

However based on your comments on #304, I’m curious what reasoning has changed to go back in the direction of sync.Cond? Maybe I’m not seeing some other added benefit!

[paulwe]

in #304 we were using a combination of cond and channels and using a background goroutine to synchronize them. i was concerned about the complexity and possible fragility of that approach. i don't have a preference for one or the other i just think we should pick one.

i wrote this because you expressed a preference for cond and it occurred to me that we could make it work if we abstracted the timer utility out of the deadline helper.

the channel based version is marginally faster for concurrent r/w benchmarks

BenchmarkBuffer14-10         	 8565831	       164.2 ns/op	  85.29 MB/s	       2 B/op	       0 allocs/op
BenchmarkBuffer140-10        	 6928408	       144.8 ns/op	 966.76 MB/s	       3 B/op	       0 allocs/op
BenchmarkBuffer1400-10       	 7962511	       167.9 ns/op	8338.08 MB/s	       2 B/op	       0 allocs/op

vs

BenchmarkBuffer14-10         	 7835804	       155.0 ns/op	  90.34 MB/s	       0 B/op	       0 allocs/op
BenchmarkBuffer140-10        	 7690047	       153.2 ns/op	 914.00 MB/s	       2 B/op	       0 allocs/op
BenchmarkBuffer1400-10       	 6457164	       182.6 ns/op	7668.64 MB/s	       3 B/op	       0 allocs/op

[edaniels]

in #304 we were using a combination of cond and channels and using a background goroutine to synchronize them. i was concerned about the complexity and possible fragility of that approach. i don't have a preference for one or the other i just think we should pick one.

i wrote this because you expressed a preference for cond and it occurred to me that we could make it work if we abstracted the timer utility out of the deadline helper.

the channel based version is marginally faster for concurrent r/w benchmarks

BenchmarkBuffer14-10         	 8565831	       164.2 ns/op	  85.29 MB/s	       2 B/op	       0 allocs/op
BenchmarkBuffer140-10        	 6928408	       144.8 ns/op	 966.76 MB/s	       3 B/op	       0 allocs/op
BenchmarkBuffer1400-10       	 7962511	       167.9 ns/op	8338.08 MB/s	       2 B/op	       0 allocs/op

vs

BenchmarkBuffer14-10         	 7835804	       155.0 ns/op	  90.34 MB/s	       0 B/op	       0 allocs/op
BenchmarkBuffer140-10        	 7690047	       153.2 ns/op	 914.00 MB/s	       2 B/op	       0 allocs/op
BenchmarkBuffer1400-10       	 6457164	       182.6 ns/op	7668.64 MB/s	       3 B/op	       0 allocs/op

Yeah FWIW your solution is a lot cleaner than my spawning an extra goroutine which felt ugly. If the benchmark was significantly in favor sync.Cond, I'd say we should use it. Thank you for exploring this!