-
Notifications
You must be signed in to change notification settings - Fork 151
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
More optimizations for InFlightRequest #362
Conversation
- Move InFlightRequest to its own file - Make InFlightRequest internal
- Remove unnecessary null checks in Dispose
- Enforce avoidance of closure by using a static method
I have no concerns with the original TaskCancellationException coming through for a client cancellation. @ColinSullivan1 @danielwertheim any objections? |
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.
Looks good, just one small question really.
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.
LGTM, thanks!
I'm usually resistant to breaking changes, but we're still pre 1.0, and it's the right thing to do. Expanding the exceptions thrown here shouldn't impact many users, if any at all.
Based on the optimizations done by @danielwertheim I further optimized InFlightRequest for use cases w/o a timeout (i.e. when using a client provided cancellation token).
Notable changes:
NATSTimeoutException
only if cancellation wasn't caused by a client provided CT. This is a potentially breaking change. Before this, aNATSTimeoutException
was always set as the TCS's exception when a timeout > 0 was passed in, even when a client provided CT caused cancellation (which may happen for any reason). Hence, clients that always used a timeout may not expect/handle theTaskCanceledException
that now occurs if the client provided CT cancelled the request. However, I think that's better than throwing an unrelated exception.Benchmark results
Source: https://github.com/jasper-d/nats.net/blob/benchmarks/inflightrequest/src/Benchmarks/MicroBenchmarks/InFlightRequestBenchmark.cs
The Categories column indicates whether
CancellationToken.None
(DefaultToken) or a "hot" CT (ClientToken) was passed toInFlightRequest
. The Timeout column refers to thetimeout
arguments value passed toInFlightRequest
.The suffix
_Upstream
indicates that the current (less optimized) version ofInFlightRequest
was used.Windows
Linux
Coalesced CT results
Not part of this PR but used to validate the rationale. Benchmarks to investigate if coalesced CTs can provide a benefit (depends on the actual scenario). Depending on the use case, this might also help in cases like #299.
Code: https://github.com/jasper-d/nats.net/blob/benchmarks/inflightrequest/src/Benchmarks/MicroBenchmarks/CoalescedTokenBenchmark.cs
InFlightRequest
uses the optimizedInFlightRequest
with a timeout > 0.InFlightRequest_Upstream_Coalesced
uses the current implementation with coalesced CTs.InFlightRequest_Coalesced
uses the optimized version with coalesced CTs.Windows
Linux