Load CH preparations in parallel #2455
Conversation
| executorService.shutdown(); | ||
| try { | ||
| for (int i = 0; i < callables.size(); i++) | ||
| completionService.take().get(); | ||
| } catch (Exception e) { | ||
| executorService.shutdownNow(); | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
| executorService.shutdown(); | |
| try { | |
| for (int i = 0; i < callables.size(); i++) | |
| completionService.take().get(); | |
| } catch (Exception e) { | |
| executorService.shutdownNow(); | |
| throw new RuntimeException(e); | |
| } | |
| try { | |
| for (int i = 0; i < callables.size(); i++) | |
| completionService.take().get(); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| throw new RuntimeException(e); | |
| } catch (ExecutionException e) { | |
| throw new RuntimeException(e); | |
| } finally { | |
| executorService.shutdown(); | |
| } |
There was a problem hiding this comment.
I tried to simply keep it as it was before. There is a long, possibly related discussion about this here: #980. But sure maybe this can be improved.
| @@ -723,6 +727,20 @@ public static long calcMillisWithTurnMillis(Weighting weighting, EdgeIteratorSta | |||
| } | |||
| } | |||
|
|
|||
| public static void runConcurrently(List<Callable<String>> callables, int threads) { | |||
There was a problem hiding this comment.
| public static void runConcurrently(List<Callable<String>> callables, int threads) { | |
| public static void runConcurrently(List<Callable<String>> callables, int threads, String threadPrefix) { |
Would be nice if we could remove the Thread renaming from the Callables and let the ThreadFactory of the Executor handle this.
There was a problem hiding this comment.
Yes, maybe, no idea tbh. There is also this comment in CHPreparationHandler:
// toString is not taken into account so we need to cheat, see http://stackoverflow.com/q/6113746/194609 for other options
| }, name); | ||
| } | ||
|
|
||
| threadPool.shutdown(); | ||
|
|
||
| try { | ||
| for (int i = 0; i < preparations.size(); i++) { | ||
| completionService.take().get(); | ||
| } | ||
| } catch (Exception e) { | ||
| threadPool.shutdownNow(); | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
I moved this code to a helper method in GHUtility.
| return c.chConfig.getName(); | ||
| }) | ||
| .collect(Collectors.toList()); | ||
| int numThreads = Math.max(1, Math.min(4, callables.size())); |
There was a problem hiding this comment.
We used to only use one thread here. The prepare.ch.threads parameter is not available here, so I hard-coded the default to 4 (max). Should be ok, because the memory usage won't be increased because of this.
There was a problem hiding this comment.
If memory consumption doesn't scale with the threadcount here, why don't we use all available cores?
int numThreads = Math.max(1, Math.min(Runtime.availableProcessors(), callables.size()));There was a problem hiding this comment.
Yes, this would also be possible, I guess. I was just trying to be conservative. If we did that we could also just use .parallelStream().
There was a problem hiding this comment.
Using parallelStream() has some sideeffects as it's ForkJoinPool is shared by the whole JVM. Probably not that bad for standalone GraphHopper but a pitfall for people which integrate it as a library.
|
@karussell any comments here? |
|
LGTM - thanks! |
|
I'll merge this so we can move forward. @otbutz if you still think something should be improved please don't hesitate to open a separate PR. |
Fixes #2454