Driver::register() takes too many arguments
#5985
Conversation
| /// driver should activate the task if it hasn't run in this long | ||
| pub period: Duration, | ||
| /// impl of [`BackgroundTask`] that represents the work of the task | ||
| pub task_impl: Box<dyn BackgroundTask>, |
There was a problem hiding this comment.
I wondered about whether it would be nicer to also be generic over a T: BackgroundTask and make boxing it the responsibility of the driver, but that would mean that Driver::register gets monomorphized separately for every background task, which seemed unfortunate (although I don't think binary size is as big a concern for Nexus as, e.g. embedded projects...). So, I think the current design seems the best to me.
There was a problem hiding this comment.
OTTOH then we could spawn the future unboxed, which means we avoid an additional heap pointer dereference every time the future is polled...but I don't know if we actually care about that overhead, either...
There was a problem hiding this comment.
That's interesting -- I hadn't considered that. Within Nexus, I think we've usually gone the other way, seeking to reduce monomorphization to decrease compile times. I had some second thoughts on even using generics here to simplify the strings, but I figured there'd be at most 4 cases and probably more like 2, and the convenience for callers is considerable.
My general approach within the control plane is to not worry about straight-line performance at the level of pointer derefs and allocations. I'll almost always choose constructs that seem clearer, easier to use (or harder to misuse), or more debuggable even if they cost a few instructions. I've seen cases where on-CPU performance of the control plane was a problem (either for latency or resource utilization), but almost always because it was pathological, not just a few percent slower than it could be. (Scalability is another question but I think is less often in tension here.)
Following up from #5962 (comment)