[WIP] ProcessorBase multi-threading
[WIP] Depends on !89 (merged) .
Add a ThreadedBaseClass
that ProcessorBase
inherit from.
It allows the computation of the ProcessorBase::process(...)
to be performed in a separated thread. Threads aren't instantiated/killed but instead are put to sleep waiting for new CaptureBase
.
ThreadedBaseClass
API is rather simple :
- Constructor with argument
max_frequency
allowing the user to set guess what, a max frequency at which the processor operates. -
run()
to start the thread and so the processor. -
stop()
allows to stop the thread and so the processor. -
isRunning()
let the user know if the processor is running or not.
Finally it has the pure virtual function :
-
executeImpl()
where the derived class do its job.
Merge request reports
Activity
One of the point raised during our last discussion was should we allow to skip new Captures if the Processor isn't done processing the previous one yet.
The
ProcessingCapturePolicy
that define the policy for adding captures :-
ProcessingCapturePolicy::STRICT
do not skip capture, wait for the processor to be done before adding a new capture. -
ProcessingCapturePolicy::SOFT
allow for skipping capture, e.g.addCapture(...)
(!89 (merged)) is called while the processor is still inprocess()
, thenaddCapture(...)
returns and the capture is lost.
Notice that the default policy is set to
ProcessingCapturePolicy::STRICT
.Notice that since we are not using smart pointers yet, a capture that is not added should be destroyed by the user ... And that as for now we can't know if a
Capture
has been added or not. MaybeaddCapture(...)
should return a bool.-
@jsola @acoromin @joanvallve I'm now facing an issue with this multi-threading and need your opinion/advice.
In short: as for now only
Processors
live in their own thread, which means that allSensors
are in the main one. This leads to the classical read/write data-race over theCaptureBasePtr
. I see here four ways to solve this :- copy the data between sensor - processor
- I know you are not fans of that solution, it is the simplest though
- add a
mutex
inCaptureBase
that lock set/get data() functions - add a
mutex
shared between the sensor and the processor - not really a solution since that would block the main thread and annihilate the whole point of going multi-threaded.
- swap the input
CaptureBase
with an innerCaptureBase
- this has the drawback of 'modifying the object that the user passed as input.
- and that would make the scheme 1 sensor -> multi-processor very tedious to manage.
What I will do as for now is to actually copy the data in the
ProcessorBase::addCapture(...)
function until we decide a proper mechanism.Go on with the copy. Mind that for cv::Mat, a proper copy needs a call to clone(). In tests using images, we implemented a simple buffer as a std::vectorcv::Mat to manage some of the asynchronous read/write issues. You can have a look.
@jsola Not yet as the keyframe creation is not thread-safe yet. I will however rebase it asap.
Added 391 commits:
- 8db6a907...f89a44e5 - 386 commits from branch
master
- e7a8f8a5 - add ThreadedBaseClass
- b42c7458 - add a bunch of utils for multi_threading
- 2f93bff9 - wip PocessorBase as a ThreadedBaseClass
- 3b979282 - wip SensorBase enable threaded processor
- 684233bd - wip ProcessingCapturePolicy & thread priority
Toggle commit list- 8db6a907...f89a44e5 - 386 commits from branch
Added 5 commits:
- 9886d182 - Motion class explicit default constructor & full args constuctor
- 841e12b4 - NodeBase & CaptureBase *id_count atomic_uint
- 90e3973f - multi-thread print also prints thread id
- 0dac60a5 -
- 1d01de3a - add timer
Toggle commit listAdded a timer class that use c++11
std::chrono::*
.
Since we are evolving in multi-threads now, the CPU time is not meaningful anymore.
Runningtest_processor_imu
on my (old) machine :t0 : 418.535 s tf : 434.729 s duration : 16.194 s N samples : 16201 frequency : 1000.364 Hz time : 0.234 s s/integr : 14.462 us integr/s : 69145.667 ips CPU time : 0.834 s s/integr : 51.496 us integr/s : 19419.082 ips
Edited by Jeremie Deraymulti_threading branch.
WARNING : This is WIP as the keyframe call/creation is not thread-safe yet.Yep do not merge.
I was actually surprised you could merge !89 (merged)!89 (merged) was not WIP but had quite a few conflicts.
@jsola @acoromin I could use some insights of yours regarding how to make both
Problem::keyFrameCallback(...)
andProcessor::keyFrameCallback
thread safe.
For the classProblem
:- The first approach I can think of is to add a
std.:mutex
inProblem
that is dedicated to the callback.
For the class
ProcessorBase
:- I most probably need to define the function
keyFrameCallback
inProcessorBase
which is pure virtual at that time. Adding a pure virtual functionkeyFrameCallbackImpl
inProcessorBase
would then fallback to the current idea that the derived class must define the behavior of this function.
Any idea/advice/opinion ??
- The first approach I can think of is to add a
Added 61 commits:
- 088ac0fa...7be3f4ec - 49 commits from branch
master
- a74b17ac - add ThreadedBaseClass
- a5510604 - add a bunch of utils for multi_threading
- db354340 - wip PocessorBase as a ThreadedBaseClass
- c94114c6 - wip SensorBase enable threaded processor
- f95badd3 - wip ProcessingCapturePolicy & thread priority
- 63e79c14 - Motion class explicit default constructor & full args constuctor
- 5440f892 - NodeBase & CaptureBase *id_count atomic_uint
- 8ecef777 - multi-thread print also prints thread id
-
c5757dc4 -
- be16205f - add timer
- fedeaed4 - test timer in test_proc_imu
- 785df76a - mv addCapture/executeImpl to cpp & add latest changes
Toggle commit list- 088ac0fa...7be3f4ec - 49 commits from branch
''Adding a pure virtual function
keyFrameCallbackImpl
inProcessorBase
would then fallback to the current idea that the derived class must define the behavior of this function.''what does this mean? What is th '
-Impl
' for?The
keyFrameCallback() = 0
is already inProcessorBase
(line 60) :virtual bool keyFrameCallback(FrameBasePtr _keyframe_ptr, const Scalar& _time_tolerance) = 0;
@joanvallve is more aware of this issues than @acoromin or @jsola .
Impl
stands for 'implementation'.
It basically means that the definition of the functionkeyFrameCallback
in the derived classes would now take place in the newly addedkeyFrameCallbackImpl
.The particular point concerning
ProcessorBase::keyFrameCallback
is more of the design and function naming issue rather than functionality issue.
The functionality would remain the very same, the only change is that derived classes would definekeyFrameCallbackImpl
rather thankeyFrameCallback
.As for the reason, it is simply that I need to put a mutex in
ProcessorBase::keyFrameCallback
.