refactor: Replace EventLoop::post() with sync() taking kj::FunctionParam - #347
refactor: Replace EventLoop::post() with sync() taking kj::FunctionParam#347ViniciusCestarii wants to merge 2 commits into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste |
|
Concept ACK 2b23e78. Looks like a nice efficiency improvement! Note that we should be able to make this more efficient by dropping the pipe used to wake the event loop and using the cross-thread mechanisms newer versions of capnproto provide ( |
|
lgtm ACK 2b23e78 |
| //! The function is accepted as a kj::FunctionParam, which only stores a | ||
| //! pointer to the caller's callable instead of taking ownership of it. This | ||
| //! avoids a heap allocation per call, and is safe because this call is | ||
| //! synchronous, so the callable is guaranteed not to go out of scope before | ||
| //! the event loop thread is done running it. It also means the callable | ||
| //! does not need to be copyable or movable. |
There was a problem hiding this comment.
In commit "Fix segfault when creating new wallet" (d3800d59d557388b813513ef16f6b840467821d4)
I think this comment would be great to have in the commit message, but would suggest removing it from the code because it is mostly just repeating capnproto API documentation. This seems like a straightforward use of the FunctionParam type as documented. It also seems potentially confusing for callers for the method documentation to be describing why the method definition is safe. It would make more sense to write that in the implementation if necessary.
4 participants
EventLoop::post() and the sync() wrapper around it did the same thing, so this merges them into a single sync() method. And also takes the callback as a kj::FunctionParam instead of a kj::Function: FunctionParam stores only a pointer to the caller's callable rather than taking ownership of it, so the callable no longer needs to be copyable or movable, and each call avoids a heap allocation.
This is safe because sync() does not return until the event loop thread has finished running the callback and cleared m_post_fn(now named m_sync_fn), so the pointer is never used after the callable goes out of scope.