storage/cmdq: create new signal type for cmd completion signaling#32164
storage/cmdq: create new signal type for cmd completion signaling#32164craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
0da46f8 to
59717fa
Compare
petermattis
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/storage/cmdq/signal.go, line 38 at r1 (raw file):
// whether the operation has completed. It is ~75x faster than // using a channel for this purpose. // 2. the type's channel is lazily initialized when signalChan()
s/type/receiver/g
pkg/storage/cmdq/signal.go, line 86 at r1 (raw file):
close(c) } return c
The returned channel might not be closed even though the signal has been signaled. This seems like a rare scenario, though I'm wondering if we should close it. I think that can be done by adding another sigHalfClosed state:
if atomic.CompareAndSwapInt32(&s.a, sig. sigHalfClosed) {
close(c)
atomic.StoreInt32(&s.a, sigClosed)
} else {
for atomic.LoadInt32(&s.a) == sigHalfClosed {
runtime.GoSched()
}
}
You'd wrap this up in a close() method so it could be shared with signal().
signal is a type that can signal the completion of an operation. The type has three benefits over using a channel directly and closing the channel when the operation completes: 1. signaled() uses atomics to provide a fast-path for checking whether the operation has completed. It is ~75x faster than using a channel for this purpose. 2. the type's channel is lazily initialized when signalChan() is called, avoiding the allocation when one is not needed. 3. because of 2, the type's zero value can be used directly. Release note: None
59717fa to
a08d232
Compare
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/cmdq/signal.go, line 38 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
s/type/receiver/g
Done.
pkg/storage/cmdq/signal.go, line 86 at r1 (raw file):
The returned channel might not be closed even though the signal has been signaled.
Is that a problem though? It will be closed immediately after (before signal() returns), and the caller of signalChan will block on the channel either way. It shouldn't be any different than if signalChan is called before signal().
tbg
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status:
petermattis
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/cmdq/signal.go, line 86 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
The returned channel might not be closed even though the signal has been signaled.
Is that a problem though? It will be closed immediately after (before
signal()returns), and the caller ofsignalChanwill block on the channel either way. It shouldn't be any different than ifsignalChanis called beforesignal().
Yeah, I suppose I was overthinking this. Without the atomic the behavior would be the same. Carry on.
|
I never mentioned the motivation for this change in the PR description. With the new approach in #31997, we end up "waiting" on a much larger number of prerequisites while iterating over prerequisites. There are two reasons for this:
This means that there's a much better chance that we reach prereq TFTRs. bors r+ |
32164: storage/cmdq: create new signal type for cmd completion signaling r=nvanbenschoten a=nvanbenschoten `signal` is a type that can signal the completion of an operation. This is a component of the larger change in #31997. The type has three benefits over using a channel directly and closing the channel when the operation completes: 1. signaled() uses atomics to provide a fast-path for checking whether the operation has completed. It is ~75x faster than using a channel for this purpose. 2. the type's channel is lazily initialized when signalChan() is called, avoiding the allocation when one is not needed. 3. because of 2, the type's zero value can be used directly. Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
signalis a type that can signal the completion of an operation.This is a component of the larger change in #31997.
The type has three benefits over using a channel directly and
closing the channel when the operation completes:
whether the operation has completed. It is ~75x faster than
using a channel for this purpose.
is called, avoiding the allocation when one is not needed.
Release note: None