open-simulation-platform / libcosim

OSP C++ co-simulation library
https://open-simulation-platform.github.io/libcosim
Mozilla Public License 2.0
61 stars 10 forks source link

Add threaded execution runner #695

Closed markaren closed 2 years ago

markaren commented 2 years ago

Should resolve #694

Moves "async" and timing related stuff into a separate header and spawns a thread for running the execution. The execution is left with a blocking simulate_until function. If you want async and real-time simulation, you use a execution_runner. Note that the execution runner is non-owning.

kyllingstad commented 2 years ago

I like this idea a lot! It cleanly separates the following two concerns:

Some things are worth discussing before we merge this, though:

restenb commented 2 years ago

Are we making a point of avoiding boost::fibers (or boost libraries in general) entirely now? Otherwise would wrapping simulate_until in a boost::fibers::packaged_task, and running the resulting task on a new thread suffice as a "make it async again" wrapper? Or perhaps just use boost::async, which combines the two steps?

kyllingstad commented 2 years ago

I've done exactly that as a temporary workaround in my own code, except I used std::packaged_task instead. The only difference is that the std:: versions of packaged_task, future, async, etc. only deal with threads, not fibers. But most people don't use fibers anyway, so not much is lost there.

The main problem with my workaround, which led to my bug report, was that cosim::execution itself currently isn't threadsafe. So when I've spun off exec.simulate_until(std::nullopt) in a separate thread, it's not safe to call exec.stop_simulation() from some other thread to stop it.

restenb commented 2 years ago

We need to fix that, to me it looks like declaring stopped_as std::atomic<bool> should suffice. Doesn't look like stopped_ gets interacted with much, unless someone is calling is_running() constantly - which also needs to be trivially changed. I believe all assignment operators will still work.

restenb commented 2 years ago

At least at a glance, the internals of execution_runner::simulate_until is equal to execution::simulate_until, with the exception of the embedded promise and thread. So I'd be in favor of using std::packaged_task or std::async wrapping execution::simulate_until instead, unless there's some added value I'm not seeing with this approach.

restenb commented 2 years ago

Other than that, I feel like a client might be confused that you're supposed to call execution_runner::simulate_until, instead of exeuction::simulate_until, if you want a real time simulation. That may just be a naming and documentation issue, however.

markaren commented 2 years ago

I'm open to make the changes directly in execution, but I do like splitting the logic.

eidekrist commented 2 years ago
  • If it is agreed that real-time simulation is the job of execution_runner, does it also makes sense to merge the timing and time measurement machinery into it? That is, integrate the functionality in cosim/timer.hpp with it in some way. (One example could be to have functions like get_real_time_factor() (or whatever it's called) as members of execution_runner.) @eidekrist, do you remember the motivation for moving these things out of execution and into separate classes in the first place?

I guess the idea was just to have time related stuff in a separate class in order to avoid clutter. At one point in time I did a number of changes to the timer API, and instead of having to rewrite execution I introduced real_time_config and real_time_metrics. TLDR; it's just for convenience.

markaren commented 2 years ago

Forgot to explicitly mentioned it, but I added #696 as an alternate competing PR, which is just a minor change. It boils down to if we want the split in logic or not.

markaren commented 2 years ago

Closing in favor of #696