Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I improved the implementation of the concurrent optimization loop in
TileUtil
. Instead of scheduling being handled by one thread and the actual work being done bynThreads
worker threads (resulting innThreads + 1
threads being used), scheduling is now offloaded to thenThreads
worker threads (resulting in onlynThreads
threads being used) by means of concurrent collections.Runtime
I ran a few small experiments: a NxN layout of 2D tiles with M exact matches to their spatial neighbors. Each tile was perturbed by a random (with fixed seed) shift. A 2D translation was fitted. The optimization reduced the error to
1e-4
without a plateau. For three differentNxN, M
layouts, I compare the proposed algorithm (PR) and the original concurrent optimization (orig) for 1 to 32 threads on a machine with 40 physical cores, as well as the single-threaded performance (second row).Since the number of iterations varies because of random shuffling, I printed the runtimes per iteration for better comparison. Also, keep in mind that the original code uses one more thread (the scheduling thread) than given here.
Details
The new algorithm is locking-free. It consists of four steps for every tile:
Concurrent access to the collection of pending tiles (
ConcurrentLinkedDeque
) and currently executing tiles (ConcurrentHashMap.newKeySet()
) is handled by the collections themselves. Checking whether a neighboring tile is currently being processed is handled by the individual threads as is subsequent action (processing or deferring the tile). In contrast to the prior implementation, two neighboring tiles can be checked at the same time (but they cannot be processed at the same time; see below).Apart from re-organizing the main fit-apply loop, I also added parallelized versions of
TileCollection::updateErrors
andTileCollection::apply
. In for the cases I've tested, this played a significant role to reduce the overall runtime.Also, I added a boolean flag to indicate whether logging is desired or not (the default being false). The logging output now prints
TileCollection::getError
instead ofErrorStatistics::mean
, which is the correct value here, I believe. This fixes #68.Finally, I fixed some IDE warnings and deleted some commented code that seemed to be used for parallelizing at some point. As a disclaimer, I did not adhere to the prevalent coding style, since my IDE actively deletes spaces when moving things around.
Correctness
The JMM gives a few guarantees about what happens-before something else:
I cannot guarantee that this method is correct (i.e., always terminates and computes the right thing). However, I'm pretty confident for the following reasons based on above principles:
Further steps
While we're at it, should we:
TileUtil
to unify the API?TileCollection::<optimize|concurrently|silently> methods
?nThreads == 1
?ConcurrentLinkedQueue
-based parallelization forTileCollection::updateErrors
andTileCollection::apply
instead of an a-priori partition of the list of tiles? This potentially allows for better load balancing, but could have more overhead.I'm happy for any feedback regarding correctness and efficiency in more practical situations, especially from @axtimwalde, @tpietzsch, @StephanPreibisch, and @bogovicj.