Faster device initialization by multi-threading. #519
Conversation
There was a problem hiding this comment.
I certainly like the idea. One thing that will be essential is a mechanism to order initialization of devices that depend on other devices (that are not from the same adapter).
The most prominent case of this is serial (and other) ports. (Perhaps this could be special-cased, though somewhat ugly.)
But I would like to see a comprehensive check to make sure that there are no other cases -- for example in some of the Utility devices, and any other that "wrap" another device(s). If it can be shown that no existing device depends on another device being initialized before it (as opposed to merely being loaded), then it's okay; otherwise this can only work by adding a mechanism to somehow represent the dependencies.
Beyond that, I cannot help but note that there is no mechanism to protect access to the core from multiple threads. Devices can call Core functions (through CoreCallback) during initialization, and we need to be sure that none of these cause race conditions. I'd be opposed to merging this without addressing this, even if it appears to work in limited testing. (It's possible that this turns out not to be an issue, or that it can be made safe in the cases where it matters.)
Another thought is that it might be good to make the parallel loading optional, even if going forward with this, so that people can disable it if it is later found to cause problems.
|
Ports should initialize first for sure. Great catch! I believe that all Utility devices only start communicating with their "target" devices after initialization (I ran into issue with this in the past). Order has never been guaranteed (depended on the config file order), so any adapter that does depend on it should be fixed. Happy to make this optional, but where should the option live? Would this be a Core Property? I will check when those are set, obviously should happen before initializing the devices or it would not work. |
|
Seems like a good candidate for the new feature flag pattern? |
…lt. Setting it false will use the old code to do serial initialization. Also ensure that ports are initialized first. Needs testing on several systems before merging.
Yes, but loading in the config file order (upon which the user has control) was guaranteed. I think we depended on this at least at some point in the past, but I agree that Utilities now look safe. In any case, I think it's okay if we've missed some extremely obscure case (I couldn't find any in 15 minutes) as long as it can be turned off (which will require a checkbox on the splash screen after this PR is merged). |
MMCore/MMCore.cpp
Outdated
| for (auto& f : futures) { | ||
| // Note: we can do a 'f.wait_for(std::chrono::seconds(20)' to wait up to 20 seconds before giving up | ||
| // which would avoid hanging with devices that hang in their initialize function | ||
| f.get(); | ||
| } |
There was a problem hiding this comment.
The std::future returned by std::async is special and its destructor blocks until the future completes. This is okay if there are 0 or 1 errors total (the successful initializations run to completion and the exception is propagated).
When there are 2 or more errors, however, the second exception would be thrown in the destructor of the future, and throwing anything in a destructor is very bad (might terminate by default).
I think it's best to catch the first exception and then explicitly wait for the remaining futures (catching and ignoring any subsequent exceptions). Then the first exception can be rethrown. (std::current_exception and std::rethrow_exception can be used.)
There was a problem hiding this comment.
Implemented as you described in a52a350. Tested with limited hardware (guess we need multiple demo devices that can throw exceptions in the initialize function to fully test this).
Also tested earlier version on complicated hardware, where it reduced startup times from 32 to 20 seconds, which is an appreciable improvement.
… clear the used vectors at the end of the function. I am not quite sure if this is needed, but after this change, I no longer saw an exception on Micro-Manager shutdown, so it may be helpful.
…alize all other devices, ignoring more exceptions if they occur, then rethrow first exception.
This PR changes the behavior of the Core function initializeAllDevices. It will discover all device modules contained in the list of devices to be initialized, spin up one thread per module and initialize all requested devices in that module. This parallelizes device initialization which can improve startup times of the Micro-Manager application considerably. Tested with a limited number of devices, I will test on a few more microscopes before merging, but requesting review (especially since I am not very experienced writing multi-threaded code in C++) now.
Minor version of Studio was upgraded according to Semver. I will wait for review before merging.