plasmodic / ecto

ecto is a dynamically configurable Directed Acyclic processing Graph (DAG) framework.
http://ecto.willowgarage.com/
BSD 3-Clause "New" or "Revised" License
98 stars 37 forks source link

Be free of the GIL! #271

Closed stonier closed 9 years ago

stonier commented 9 years ago

This should only be looked at after #269 and #270. Summary of what it does is below.

I suspect this is the best place for these calls. I originally had them in the execute() function itself, but this is more complete.

The only worry is that it may affect people's code who by default make python calls from their cells...in which case they would need to toss in a ECTO_SCOPED_CALLPYTHON whre they make the call. The official ecto stacks however do not seem to require this anywhere and I only do so in one cell of mine, so it's a rare occurence.

If this is an issue, this could be shifted to a new api so it doesn't effect existing usercode. For instance an execute_threaded() function or similar.

.....................

This lets ecto pipeline processing be free of its evil overlord, the GIL. That is, you can now schedule different plasms in different threads...e.g. some pseudocode:

# construct some plasms
image_scheduler = ecto.Scheduler(image_plasm)
odometry_scheduler = ecto.Scheduler(odometry_plasm)
image_thread = threading.Thread(name="image_thread", target=image_scheduler.execute)
odometry_thread = threading.Thread(name="odometry_thread", target=odometry_scheduler.execute)
image_thread.start()
odometry_thread.start()
image_thread.join()
odometry_thread.join()

Of course, inside a cell's process() call, you can always grab the GIL momentarily if you need to using the opposite macro...ECTO_SCOPED_CALLPYTHON.

stonier commented 9 years ago

I'll add a test and some documentation and lets see how this shapes up.

stonier commented 9 years ago

Ok..so this has meant me digging into details a bit more - this threaded behaviour I'm replicating appears to be something that the scheduler's execute_async function is supposed to support, but ever since I started using this, it doesn't. i.e. from the docs:

execute_async(niter)
These functions are the same as the blocking ones above, except they start an execution thread in the background and return immediately.

running()
Returns true if the execution started by execute_async() is still running.

stop()
Stops the background graph execution at the end of the current process() call and block until it is stopped

and this code snippet from the same documentation has never worked as intended:

s = Sheduler(p)
s.execute_async()
time.sleep(0.5)
s.stop()
s.execute_async()
assert s.running()

So the question is, when and why this stopped working?

And with what I'm doing here - should I just renable threaded execute_async calls inside the c++ instead of doing it from python like I have in c2a24a513?

stonier commented 9 years ago

So the question is, when and why this stopped working?

Looks suspiciously like it disappeared in dd8fef0e where new thread creation in execute_async got removed.

stonier commented 9 years ago

And with what I'm doing here - should I just renable threaded execute_async calls inside the c++ instead of doing it from python like I have in c2a24a5?

My preference would be to manage it from python spawned threads - that is where the user scripts the flow - i.e. graphs, plasms and scheduling. That is the easiest place to let the user design various kinds of scheduling like I have in c2a24a5. Since the threads behind python are just native threads - I don't expect there would be any significant slowdown.

stonier commented 9 years ago

@vrabaud at this point I would rename exec_async to just exec_implementation or something similar, add python handles to give some asynchronous capability and update the documentation to reflect the current state of threading support in ecto (it's currently way out). What are your thoughts?

vrabaud commented 9 years ago

feel free to do so. I know multi-threading got removed at some point,. I am not sure it impacts what you are describing.

stonier commented 9 years ago

Ach, rebasing always brings trouble! :boom:

Anyway, it's done. Ignore the earlier commits up to 'Be free of the GIL', they were brought in by the rebasing.

Scheduler updates

Releasing the gil enables multithreading capability by default and users can always recapture the gil in their cells if they wish (I do so in some of my cells).

The refactored name works around the confusion I had with it to begin with (it doesn't actually execute). This is less misleading I think, and we can reserve execute_async for later when I'd actually like to have it start a background thread and run jobs immediately as the documentation formerly hinted at and similar to what execute does.

Tests

Documentation

Summary

stonier commented 9 years ago

@vrabaud anything you think this needs to help move this along?

vrabaud commented 9 years ago

time is all I needed :) Sorry, it all looks good to me. Thx for the doc, it really helps.

vrabaud commented 9 years ago

and it's finally released

stonier commented 9 years ago

:neckbeard: