[SERVER-53623] Use a static variable for CancelationToken::Uncancellable() Created: 06/Jan/21 Updated: 10/Mar/21 Resolved: 10/Mar/21 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Minor - P4 |
| Reporter: | Benjamin Caimano (Inactive) | Assignee: | Tyler Seip (Inactive) |
| Resolution: | Won't Do | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Sprint: | Service Arch 2021-03-22 |
| Participants: |
| Description |
|
We create a new CancelationToken for each call to CancelationToken::uncancelable() here. Unfortunately, this makes each invocation take more time than is necessary. We should shift the function to construct an uncancelable state once and reuse it for each invocation.
|
| Comments |
| Comment by Matthew Saltz (Inactive) [ 10/Mar/21 ] |
|
We can revisit this if it's ever a problem |
| Comment by Billy Donahue [ 10/Mar/21 ] |
|
Yes. It would be interesting if the BM_uncancelable_token_ctor benchmark showed the cost as the # of threads rises. Just to put some handwaving numbers on this, experiments benchmarking Status Ref/Unref (a similar refcounted type) showed that the cost of the intrusive_ptr copy goes up by about 40ns just by adding a second thread. Adding more threads it gets worse from there in a rougly linear way. Speeding up from 172ns to 14ns in a single threaded case is nice, but those gains would be quickly wiped out by adding a handful of competing threads. If contention adds 40ns/thread (which is seems to in the Status case), then just 5 competing threads would make the proposed static variable a pessimization.
|
| Comment by Matthew Saltz (Inactive) [ 10/Mar/21 ] |
|
Yeah I just meant nonstandard as in uncommon, sorry for the confusion there. Okay I say we "Won't do" this and come back to it later if it ever ends up being a bottleneck for something |
| Comment by Billy Donahue [ 10/Mar/21 ] |
|
I just want to clarify that thread_local is standard C++11. If you mean nonstandard more colloquially I agree. It can be unexpected. If it's thread_local, all references to that variable are invalid when the thread that created it ends. If it's going to be passed between threads we'd have problems. I agree with leaving it alone and speeding up the "uncancelable" value's initializer if it's too slow, without trying to share instances of it. |
| Comment by Matthew Saltz (Inactive) [ 10/Mar/21 ] |
|
This is probably a terrible idea but making it a thread_local would mean a given thread would only have to create it once, and then there would also be no contention across threads. So in an executor thread for example it'd allow for a lot of savings in constructing the object, but a one-off thread might have to go ahead and make it for itself. But given that using thread_local is kind of confusing and not standard I'd almost rather just leave it in its current less performant state - it's not like we're creating these in a tight loop. My first implementation by the way allowed for a nullptr state instead of the way it is now - it's doable but just makes the CancelationToken implementation a little messier. |
| Comment by Billy Donahue [ 04/Mar/21 ] |
|
Something to consider. maybe the intrusive_ptr refcount contention involved in using a single sentinel will be expensive, maybe more expensive than the construction costs because the contention affects all threads using any uncancelable token. Maybe a nullptr state intrusive_ptr can be used for the "uncancelable" so there's no contention. |