Skip to content
Snippets Groups Projects

[WIP] ProcessorBase multi-threading

Closed Jeremie Deray requested to merge multi_threading into master

[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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Jeremie Deray Changed title: [WIP] Multi threading[WIP] ProcessorBase multi-threading

    Changed title: [WIP] Multi threading[WIP] ProcessorBase multi-threading

  • Jeremie Deray Added 1 commit:

    Added 1 commit:

    • 9f18b068 - wip ProcessingCapturePolicy & thread priority
  • Author Developer

    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 in process(), then addCapture(...) 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. Maybe addCapture(...) should return a bool.

  • Jeremie Deray Added 1 commit:

    Added 1 commit:

    • 8db6a907 - add test_processor_odom_2d
  • Author Developer

    @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 all Sensors are in the main one. This leads to the classical read/write data-race over the CaptureBasePtr. 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 in CaptureBase 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 CaptureBasewith an inner CaptureBase
    • 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.

  • Hi. Want to merge this into master with the smart pointers in it?

  • Author Developer

    @jsola Not yet as the keyframe creation is not thread-safe yet. I will however rebase it asap.

  • Jeremie Deray Added 391 commits:

    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

    Compare with previous version

  • Author Developer

    Rebased directly on master post-smartptr and working.
    I have to review the need for copying captures, this might have change since we are using smart_ptr.

    Still [WIP] as the keyframe call/creation is not thread-safe yet, but anyone should be able to test it.

  • Jeremie Deray Added 5 commits:

    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 - :lipstick:
    • 1d01de3a - add timer

    Compare with previous version

  • Jeremie Deray Added 1 commit:

    Added 1 commit:

    • 088ac0fa - test timer in test_proc_imu

    Compare with previous version

  • Author Developer

    Added a timer class that use c++11 std::chrono::*.
    Since we are evolving in multi-threads now, the CPU time is not meaningful anymore.
    Running test_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 Deray
  • Author Developer

    multi_threading branch.
    WARNING : This is WIP as the keyframe call/creation is not thread-safe yet.

  • OK. This means I should not merge yet?

  • Author Developer

    Yep do not merge.
    I was actually surprised you could merge !89 (merged)

  • !89 (merged) was not WIP but had quite a few conflicts.

  • Author Developer

    Its title started with [WIP] which should prevent from merging. At least I though so.

  • Author Developer

    @jsola @acoromin I could use some insights of yours regarding how to make both Problem::keyFrameCallback(...) and Processor::keyFrameCallback thread safe.
    For the class Problem :

    • The first approach I can think of is to add a std.:mutex in Problem that is dedicated to the callback.

    For the class ProcessorBase:

    • I most probably need to define the function keyFrameCallback in ProcessorBase which is pure virtual at that time. Adding a pure virtual function keyFrameCallbackImpl in ProcessorBase would then fallback to the current idea that the derived class must define the behavior of this function.

    Any idea/advice/opinion ??

  • Jeremie Deray Added 61 commits:

    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 - :lipstick:
    • be16205f - add timer
    • fedeaed4 - test timer in test_proc_imu
    • 785df76a - mv addCapture/executeImpl to cpp & add latest changes

    Compare with previous version

  • Author Developer

    Rebased on current master.

  • ''Adding a pure virtual function keyFrameCallbackImpl in ProcessorBase 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 in ProcessorBase (line 60) :

            virtual bool keyFrameCallback(FrameBasePtr _keyframe_ptr, const Scalar& _time_tolerance) = 0;
  • @joanvallve is more aware of this issues than @acoromin or @jsola .

  • Author Developer

    Impl stands for 'implementation'.
    It basically means that the definition of the function keyFrameCallback in the derived classes would now take place in the newly added keyFrameCallbackImpl.

    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 define keyFrameCallbackImpl rather than keyFrameCallback.

    As for the reason, it is simply that I need to put a mutex in ProcessorBase::keyFrameCallback.

  • I see this MR is very old and will be quite difficult to merge. Do you want to keep it?

  • Author Developer

    You can close it if you want to but please do not delete the associated branch as it could be a starting point for a moving to multi-threading.

Please register or sign in to reply
Loading