[SERVER-52948] Investigate removing MONGO_COMPILER_COLD_FUNCTION from appropriate StatusWith constructors Created: 19/Nov/20 Updated: 02/Feb/23 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | Internal Code |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Matthew Saltz (Inactive) | Assignee: | Backlog - Service Architecture |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | servicearch-wfbf-day | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Assigned Teams: |
Service Arch
|
| Participants: |
| Description |
|
I'm skeptical about whether these add any perf benefit, and am suspicious that they might actually incur a loss. We should perf test removing these in turn and seeing how that affects performance. In particular the implicit conversion constructor from Status -> StatusWith could be called on accident by passing Status to a function that takes StatusWith as a parameter and user would not know they were doing something "cold". |
| Comments |
| Comment by Lauren Lewis (Inactive) [ 18/Mar/22 ] |
|
We haven’t heard back from you for some time, so we're going to close this ticket. If this is still an issue for you, please provide additional information and we will reopen the ticket. |
| Comment by Billy Donahue [ 19/Nov/20 ] |
|
I'll just paste in my thoughts on the nonexplicit StatusWith(Status) ctor from the #server-servicearch thread. There's a dassert in there that the status is !isOK(). A few things: That's an inappropriate dassert and should be upgraded to invariant immediately. The check is crucial to StatusWith's internal consistency, and extremely cheap to compute (amounts to checking pointer nullity). As an error handler, it's likely that a significant portion of the callgraph edges leading to it are untested, so debug builds are insufficient enforcement. Should a conversion that's potentially so catastrophic really be non explicit? It's supremely useful for a function with a StatusWith<LongBoringName> return type to be able to return status; instead of return StatusWith<LongBoringName>(status); But a helper function like makeStatusWith(status) can take care of that pain. All told I think implicit conversion is probably going to end up being too convenient to axe. We treat ALL non-OK Status handling as exceptional and therefore cold. This is a case where we KNOW the Status is exceptional or we're breaking an invariant. So it's as justified a MONGO_COMPILER_COLD_FUNCTION as any other function or Status/StatusWith ctor designed for exceptional Status. If the compiler back-propagates that coldness back up to a conditional, that's appropriate and we should thank it for being so smart. I guess I'm unclear what problem you're anticipating. With a f(StatusWith<int>), a caller calls it with f(status) . That status must be nonOK or this is a serious logic error. Relative performance of cold/noncold paths is the least of our worries. That's an exceptional path and should be as cold as any identified exceptional Status path. If the question is about whether we should have COLD paths for exceptional Status in general, that's possibly a discussion to open up. But this particular annotation on this particular function isn't special. It's like all the others. |