fibers: Add lifeycle hook to fiber#610
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
An option hook is added. It is a function pointer and an opaque context pointer. If it is set, then it is called on fiber suspend and resume, with user supplied context pointer. Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>
ceacba7 to
5c12ac1
Compare
|
just a heads up and something that popped my mind, we can also dispatch plain functions outside of a fiber context. Not sure if we need a hook for those cases as well (see proactor main loop on how we execute those) |
|
I do not think we need. I'm any case I asked abhijiat to write down how this will help with memory accounting. I do not wish to add this if it does not solve anything as it slows down all context switches |
|
just consider this an experiment for now, Im using it in dragonflydb/dragonfly#7794 but its a swappable part |
| // the fiber context. Returns the active fiber before the context switch. | ||
| FiberInterface* SwitchSetup(); | ||
|
|
||
| void CallSwitchHook(FiberSwitchHookEvent event) noexcept { |
There was a problem hiding this comment.
You do not need these duplicated functions.
Both can be removed if you introduce FiberSwitchHook::Run(FiberSwitchHookEvent)
and just call it on the appropriate objects.
| HIGH = 2, // High priority, activated earlier than other fibers. | ||
| }; | ||
|
|
||
| enum class FiberSwitchHookEvent : uint8_t { |
There was a problem hiding this comment.
Do you need both events? I would like to see memory accounting design before we submit this
There was a problem hiding this comment.
we need both events, now I can answer this because the accounting design has progressed. the use case is like this:
in dragonfly we have the struct with the following fields:
- baseline: memory usage in bytes when the scope began
- delta: increase/decrease in memory usage which will contribute to metric, starts at 0
on suspend we need to do:
- compute delta to latest value before suspension (delta = current_mem_usage - baseline)
on resume we need to do:
- restore baseline to current_mem_usage so that later on delta calculations (at destructor or next suspend) are against current baseline
So we need the callback method to be able to differentiate between the two events.
Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>
c377a62 to
bb061d6
Compare
Signed-off-by: Abhijat Malviya <abhijat@dragonflydb.io>
|
I added a benchmark in a temp. commit, which will be dropped after review, it shows the following numbers the no hook benchmark does not install a hook and keeps yielding between the test and subject fiber. However even here the I commented out the lines which actually run the hooks for better comparison and reran the benchmark: the numbers are still somewhat similar to the first run. perhaps it is best to compare with the base branch (only the no hook version ofc) |
|
same benchmark on the base branch is moderately faster: the numbers on this branch for comparison: After several runs, the base branch yield test is between 85 to 86 ns on each run. So there is around a 2ns/test cost on my machine for the hook setup and calling etc on suspend/resume. |
An option hook is added. It is a function pointer and an opaque context pointer. If it is set, then it is called on fiber suspend and resume, with user supplied context pointer.