fn: optimize context guard #9361
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
starius
force-pushed
the
optimize-context-guard
branch
from
1f042cd to
b22cc4f
Compare
| }() | ||
| // We don't have to wait on g.quit here: g.quit can be closed only in | ||
| // Quit method, which also closes the context we are waiting for. | ||
| context.AfterFunc(ctx, func() { |
There was a problem hiding this comment.
This doesn't really remove any overhead (since AterContext() calls propagateCancel() which creates the same "wait for context cancel" goroutine. But it's much simpler to read, so nice optimization in any case.
Also nice catch that with the g.quit now being un-exported, we can skip that code path as well.
There was a problem hiding this comment.
propagateCancel() which creates the same "wait for context cancel" goroutine
When the context is derived from cancelCtx (the most common case), propagateCancel avoids starting additional waiting goroutines. The cancelCtx type has an internal mechanism to track its child contexts, allowing it to recursively cancel all children when the parent context is canceled.
I added the test TestContextGuardCountGoroutines to verify that the Create method does not start any waiting goroutines. Without this optimization, the test would initiate 4000 new goroutines.
It's worth noting that a goroutine is indeed started later during context cancellation. However, it immediately calls wg.Done() and completes, so it doesn't exist throughout the context's lifetime. While the number of goroutines remains the same, their total runtime is significantly reduced, improving efficiency.
There was a problem hiding this comment.
Ah, right, that makes sense. Should've looked at the code in propagateCancel more closely I guess 🙈 Thanks for the clarification! And the additional unit test.
There was a problem hiding this comment.
Thanks for this PR! @starius I understand the core change is to use context.AfterFunc instead of a goroutine for context cancellation handling, which should reduce overhead.
Without this optimization, the test would initiate 4000 new goroutines.
I'm trying to understand what you mean by this.
Are you referring to starting a goroutine for each context created here (in the test)?
// Create 1000 contexts of each type.
for i := 0; i < 1000; i++ {
_, _ = g.Create(ctx)
_, _ = g.Create(ctx, WithBlockingCG())
_, _ = g.Create(ctx, WithTimeoutCG())
_, _ = g.Create(ctx, WithBlockingCG(), WithTimeoutCG())
}Additionally, the addition of the test is great. Overall, this looks like a solid improvement.
There was a problem hiding this comment.
Are you referring to starting a goroutine for each context created here (in the test)?
Yes.
|
@ellemouton: review reminder |
| // TestContextGuardCountGoroutines makes sure that ContextGuard doesn't create | ||
| // any goroutines while waiting for contexts. | ||
| func TestContextGuardCountGoroutines(t *testing.T) { | ||
| g := NewContextGuard() |
There was a problem hiding this comment.
t.Parallel() is not invoked in this test because it relies on an accurate count of active goroutines. Running other tests in parallel would introduce additional goroutines, leading to unreliable results.
I added a NOTE to the test.
Make sure WgWait() doesn't block.
Removed 'cancel' argument, because it is called only in case the context has already expired and the only action that cancel function did was cancelling the context.
If ContextGuard lives for some time after Quit method is called, the map won't be collected by GC. Optimization.
Simplifies context cancellation handling by using context.AfterFunc instead of a goroutine to wait for context cancellation. This approach avoids the overhead of a goroutine during the waiting period. For ctxQuitUnsafe, since g.quit is closed only in the Quit method (which also cancels all associated contexts), waiting on context cancellation ensures the same behavior without unnecessary dependency on g.quit. Added a test to ensure that the Create method does not launch any goroutines.
starius
force-pushed
the
optimize-context-guard
branch
from
b22cc4f to
07c4668
Compare
|
@ellemouton I created a draft PR pointing to this version of I updated Waiting for itests. |
|
itests in #9401 are green, except some itests which haven't run:
|
Change Description
Simplifies context cancellation handling by using context.AfterFunc instead of a goroutine to wait for context cancellation. This approach avoids the overhead of a goroutine during the waiting period.
Additional changes:
cancelof ctxBlockingSteps to Test
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.