diff --git a/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java b/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java index c3ee89aef..e3683fd36 100644 --- a/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java +++ b/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java @@ -88,8 +88,8 @@ public LoaderState preloadData (AnalysisWorkerTask task) { /** * A blocking way to ensure the network and all linkages and precomputed tables are prepared in advance of routing. - * Note that this does not perform any blocking or locking of its own - any synchronization will be that of the - * underlying caches (synchronized methods on TransportNetworkCache or LinkedPointSet). It also bypasses the + * Note that this does not perform any blocking or locking of its own. Any synchronization or turn-taking will be + * that of the underlying caches (TransportNetworkCache or LinkageCache). It also bypasses the * AsyncLoader locking that would usually allow only one buildValue operation at a time. All threads that call with * similar tasks will make interleaved calls to setProgress (with superficial map synchronization). Other than * causing a value to briefly revert from PRESENT to BUILDING this doesn't seem deeply problematic. diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java b/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java index 2add61702..d8e89dfd9 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java +++ b/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java @@ -458,10 +458,15 @@ protected void handleOneRegionalTask (RegionalTask task) throws Throwable { } // Pull all necessary inputs into cache in a blocking fashion, unlike single-point tasks where prep is async. - // Avoids auto-shutdown while preloading. Must be done after loading destination pointsets to establish extents. - // Note we're completely bypassing the async loader here and relying on the older nested LoadingCaches. - // If those are ever removed, the async loader will need a synchronous mode with per-path blocking (kind of - // reinventing the wheel of LoadingCache) or we'll need to make preparation for regional tasks async. + // This is because single point tasks return fast to report progress, while regional tasks currently do not. + // Worker auto-shutdown time should remain very high during this blocking preload step. Destination point sets + // must already be loaded to establish extents before the preloading step begins. Note that we're still using + // the NetworkPreloader which is an AsyncLoader, but calling a method that intentionally skips all the async or + // background proccessing machinery. Usually, N RegionalTasks all try to preload at once, and all block on + // this method. Redundant slow calculation is not prevented by the AsyncLoader class itself, but by the other + // LoadingCaches behind it. Specifically, the TransportNetworkCache and LinkageCache enforce turn-taking and + // prevent redundant work. If those are ever removed, we would need either async regional task preparation, or + // a synchronous mode with per-key blocking on AsyncLoader (kind of reinventing the wheel of LoadingCache). TransportNetwork transportNetwork = networkPreloader.synchronousPreload(task); // If we are generating a static site, there must be a single metadata file for an entire batch of results.