tighten lock around appending new chunks of read data in stream #28
Conversation
|
ref: #26 |
util.go
Outdated
| } | ||
|
|
||
| func (s *segmentedBuffer) Len() int { | ||
| return int(atomic.LoadUint64(&s.len)) |
There was a problem hiding this comment.
What was the motivation for the atomics? We end up doing a lot of atomic operations just to allow checking the length without taking a lock.
(I haven't profiled it so it may be fine, I'm just wondering).
There was a problem hiding this comment.
reads will be regularly checking if the length has extended. my intuition was this would be faster than locking, reading, and unlocking.
There was a problem hiding this comment.
Yeah, my real concern is whether that outweighs the cost of multiple atomic operations (and potential memory barriers) while we're holding a lock anyways. But it works so I have no real objections.
There was a problem hiding this comment.
i suspect a lot of that has to do with the list of buffers rather than the length atomics 🤷♂️
I'm also going to revert back to uint32's throughout, as they were before. There was an underflow that i was confirming, which is why i bumped them up to 64 bits.
util.go
Outdated
| s.bm.Lock() | ||
| defer s.bm.Unlock() | ||
|
|
||
| currentWindow := atomic.LoadUint64(&s.len) + atomic.LoadUint64(&s.cap) + s.pending |
There was a problem hiding this comment.
nit: I think we can drop the atomic loads here given that these values are only ever modified under the lock. But I'm really not sure about that.
There was a problem hiding this comment.
i think i agree, but it seemed like a sane pattern to keep all accesses atomic when it's needed at least some of the time to prevent unexpected glitches.
util.go
Outdated
|
|
||
| currentWindow := atomic.LoadUint64(&s.len) + atomic.LoadUint64(&s.cap) + s.pending | ||
| if currentWindow > max { | ||
| // Not an error, but rather an in-flight reservation hasn't hit. |
|
The lock contention appears to be completely gone. |
util.go
Outdated
| if n == length { | ||
| break LOOP | ||
| } | ||
| case io.EOF: |
There was a problem hiding this comment.
I actually wonder if we should ignore the error any time we read a full message. But that's probably an edge-case that really doesn't matter.
Co-authored-by: Steven Allen <[email protected]>
we can likely save some allocations by keeping reads segmented rather than forcing them to go through a single continuous slice of bytes.
There's an interleaving that becomes possible with this change, where reads want to know the capacity as though writes haven't "landed", but writes want to know the remaining "un-reserved" space. Figuring out how to represent that sanely still.