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 |
kj::FunctionParam 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 EventLoop:sync() 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.
2b23e78 to
5c49666
Compare
|
Thanks for the reviews! Forced push 5c49666 addressing suggestion #347 (comment). |
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.