DynamicAtlas improvements: Multi-Atlas loading and hotspots (#208)

* Avoid re-loading Multi-Atlas twice when shard set is unchanged * Trigger * Fixed hotspots in dynamicatlas * BareAtlas cleanup and sub name improvement * test number of times multi atlas is built * generalized fix for AbstractAtlas
osmlab · Aug 31, 2018 · 80d2723 · 80d2723
1 parent b1a0e34
commit 80d2723
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 28 deletions.
diff --git a/src/main/java/org/openstreetmap/atlas/geography/atlas/AbstractAtlas.java b/src/main/java/org/openstreetmap/atlas/geography/atlas/AbstractAtlas.java
@@ -119,7 +119,7 @@ public Iterable<Area> areasCovering(final Location location)
     @Override
     public Iterable<Area> areasCovering(final Location location, final Predicate<Area> matcher)
     {
-        return Iterables.filter(this.getAreaSpatialIndex().get(location.bounds()), matcher);
+        return Iterables.filter(areasCovering(location), matcher);
     }
 
     @Override
@@ -146,14 +146,14 @@ public Iterable<Edge> edgesContaining(final Location location)
         return Iterables.filter(edges, edge ->
         {
             final PolyLine polyline = edge.asPolyLine();
-            return location.bounds().overlaps(polyline);
+            return polyline.contains(location);
         });
     }
 
     @Override
     public Iterable<Edge> edgesContaining(final Location location, final Predicate<Edge> matcher)
     {
-        return Iterables.filter(this.getEdgeSpatialIndex().get(location.bounds()), matcher);
+        return Iterables.filter(edgesContaining(location), matcher);
     }
 
     @Override
@@ -216,14 +216,14 @@ public Iterable<Line> linesContaining(final Location location)
         return Iterables.filter(lines, line ->
         {
             final PolyLine polyline = line.asPolyLine();
-            return location.bounds().overlaps(polyline);
+            return polyline.contains(location);
         });
     }
 
     @Override
     public Iterable<Line> linesContaining(final Location location, final Predicate<Line> matcher)
     {
-        return Iterables.filter(this.getLineSpatialIndex().get(location.bounds()), matcher);
+        return Iterables.filter(linesContaining(location), matcher);
     }
 
     @Override

diff --git a/src/main/java/org/openstreetmap/atlas/geography/atlas/BareAtlas.java b/src/main/java/org/openstreetmap/atlas/geography/atlas/BareAtlas.java
@@ -639,19 +639,19 @@ public Optional<Atlas> subAtlas(final Predicate<AtlasEntity> matcher)
         // would include only edges, but all the nodes would have to be pulled in. In that case, we
         // use the same size as the source Atlas, but we trim it at the end.
         final PackedAtlasBuilder builder = new PackedAtlasBuilder().withSizeEstimates(size())
-                .withMetaData(metaData());
+                .withMetaData(metaData()).withName(this.getName() + "_sub");
 
         // First, index all the nodes contained by relations and all start/stop nodes from edges
         // contained by relations
-        for (final AtlasEntity entity : relations(relation -> matcher.test(relation)))
+        for (final AtlasEntity entity : relations(matcher::test))
         {
             indexAllNodesFromRelation((Relation) entity, builder, 0);
         }
 
         // Next, index all the individual nodes and edge start/stop nodes coming from the predicate
-        final Iterable<AtlasEntity> nodes = Iterables.stream(nodes(item -> matcher.test(item)))
+        final Iterable<AtlasEntity> nodes = Iterables.stream(nodes(matcher::test))
                 .map(item -> (AtlasEntity) item);
-        final Iterable<AtlasEntity> edges = Iterables.stream(edges(item -> matcher.test(item)))
+        final Iterable<AtlasEntity> edges = Iterables.stream(edges(matcher::test))
                 .map(item -> (AtlasEntity) item);
         for (final AtlasEntity entity : new MultiIterable<>(nodes, edges))
         {
@@ -664,19 +664,19 @@ public Optional<Atlas> subAtlas(final Predicate<AtlasEntity> matcher)
         // Similarly, Relations depend on all other entities to have been added, since they make up
         // the member list. For this pass, add all entities, except Relations, that match the given
         // Predicate to the builder.
-        for (final Edge edge : edges(item -> matcher.test(item)))
+        for (final Edge edge : edges(matcher::test))
         {
             indexEdge(edge, builder);
         }
-        for (final Area area : areas(item -> matcher.test(item)))
+        for (final Area area : areas(matcher::test))
         {
             builder.addArea(area.getIdentifier(), area.asPolygon(), area.getTags());
         }
-        for (final Line line : lines(item -> matcher.test(item)))
+        for (final Line line : lines(matcher::test))
         {
             builder.addLine(line.getIdentifier(), line.asPolyLine(), line.getTags());
         }
-        for (final Point point : points(item -> matcher.test(item)))
+        for (final Point point : points(matcher::test))
         {
             builder.addPoint(point.getIdentifier(), point.getLocation(), point.getTags());
         }
@@ -689,7 +689,7 @@ public Optional<Atlas> subAtlas(final Predicate<AtlasEntity> matcher)
         // relation has been processed). This guarantees that anything we add to the index has all
         // of its members indexed already.
         Set<Long> stagedRelationIdentifiers = new HashSet<>();
-        final Iterable<Relation> relations = relations(item -> matcher.test(item));
+        final Iterable<Relation> relations = relations(matcher::test);
         for (final Relation relation : relations)
         {
             checkRelationMembersAndIndexRelation(relation, matcher, stagedRelationIdentifiers,

diff --git a/src/main/java/org/openstreetmap/atlas/geography/atlas/dynamic/DynamicAtlas.java b/src/main/java/org/openstreetmap/atlas/geography/atlas/dynamic/DynamicAtlas.java
@@ -60,6 +60,7 @@ public class DynamicAtlas extends BareAtlas
 
     // The current Atlas that will be swapped during expansion.
     private Atlas current;
+    private Set<Shard> shardsUsedForCurrent;
 
     private final Map<Shard, Atlas> loadedShards;
     private final Function<Shard, Optional<Atlas>> atlasFetcher;
@@ -70,6 +71,9 @@ public class DynamicAtlas extends BareAtlas
     // This is true when, in case of deferred loading, the loading of the shards has been called
     // (unlocking further automatic loading later)
     private boolean isAlreadyLoaded = false;
+    private boolean preemptiveLoadDone = false;
+    // Number of times the udnerlying Multi-Atlas has been built.
+    private int timesMultiAtlasWasBuiltUnderneath;
 
     /**
      * @param dynamicAtlasExpansionPolicy
@@ -79,8 +83,10 @@ public DynamicAtlas(final DynamicAtlasPolicy dynamicAtlasExpansionPolicy)
     {
         this.setName("DynamicAtlas(" + dynamicAtlasExpansionPolicy.getInitialShards().stream()
                 .map(Shard::getName).collect(Collectors.toSet()) + ")");
+        this.timesMultiAtlasWasBuiltUnderneath = 0;
         this.sharding = dynamicAtlasExpansionPolicy.getSharding();
         this.loadedShards = new HashMap<>();
+        this.shardsUsedForCurrent = new HashSet<>();
         this.atlasFetcher = dynamicAtlasExpansionPolicy.getAtlasFetcher();
         // Still keep the policy
         this.policy = dynamicAtlasExpansionPolicy;
@@ -138,23 +144,31 @@ public Rectangle bounds()
     public void buildUnderlyingMultiAtlas()
     {
         final Time buildTime = Time.now();
-        if (logger.isDebugEnabled())
+        final Set<Shard> nonNullShards = nonNullShards();
+        if (this.shardsUsedForCurrent.equals(nonNullShards))
         {
-            logger.debug("{}: Loading MultiAtlas with {}", this.getName(),
-                    nonNullShards().stream().map(Shard::getName).collect(Collectors.toList()));
+            // Same Multi-Atlas, let's not reload.
+            return;
         }
-        this.policy.getShardSetChecker().accept(nonNullShards());
         final List<Atlas> nonNullAtlasShards = getNonNullAtlasShards();
         if (!nonNullAtlasShards.isEmpty())
         {
+            this.policy.getShardSetChecker().accept(nonNullShards());
             if (nonNullAtlasShards.size() == 1)
             {
                 this.current = nonNullAtlasShards.get(0);
             }
             else
             {
+                if (logger.isDebugEnabled())
+                {
+                    logger.debug("{}: Loading MultiAtlas with {}", this.getName(), nonNullShards()
+                            .stream().map(Shard::getName).collect(Collectors.toList()));
+                }
                 this.current = new MultiAtlas(nonNullAtlasShards);
+                this.timesMultiAtlasWasBuiltUnderneath++;
             }
+            this.shardsUsedForCurrent = nonNullShards;
             if (this.initialized)
             {
                 this.isAlreadyLoaded = true;
@@ -210,6 +224,15 @@ public Iterable<Edge> edgesIntersecting(final Polygon polygon, final Predicate<E
                 this::newEdge);
     }
 
+    /**
+     * @return The number of times that {@link DynamicAtlas} has (re-)built its {@link MultiAtlas}
+     *         underneath.
+     */
+    public int getTimesMultiAtlasWasBuiltUnderneath()
+    {
+        return this.timesMultiAtlasWasBuiltUnderneath;
+    }
+
     @Override
     public Line line(final long identifier)
     {
@@ -417,6 +440,7 @@ public void preemptiveLoad()
             // Loop through the entities again to find potential shards to add.
             this.entities();
         }
+        this.preemptiveLoadDone = true;
     }
 
     @Override
@@ -593,7 +617,9 @@ private <V extends AtlasEntity, T> Iterable<T> expand(
     {
         StreamIterable<V> result = Iterables.stream(entitiesSupplier.get())
                 .filter(Objects::nonNull);
-        while (!entitiesCovered(result, entityCoveredPredicate))
+        final boolean shouldStopExploring = this.policy.isDeferLoading()
+                && !this.policy.isExtendIndefinitely() && this.preemptiveLoadDone;
+        while (!shouldStopExploring && !entitiesCovered(result, entityCoveredPredicate))
         {
             result = Iterables.stream(entitiesSupplier.get()).filter(Objects::nonNull);
         }

diff --git a/.../java/org/openstreetmap/atlas/geography/atlas/dynamic/DynamicAtlasPreemptiveLoadTest.java b/.../java/org/openstreetmap/atlas/geography/atlas/dynamic/DynamicAtlasPreemptiveLoadTest.java
@@ -25,8 +25,6 @@ public class DynamicAtlasPreemptiveLoadTest
     @Rule
     public DynamicAtlasPreemptiveLoadTestRule rule = new DynamicAtlasPreemptiveLoadTestRule();
 
-    private DynamicAtlas dynamicAtlas;
-
     private Map<Shard, Atlas> store;
     private final Supplier<DynamicAtlasPolicy> policySupplier = () -> new DynamicAtlasPolicy(
             shard ->
@@ -41,27 +39,48 @@ public class DynamicAtlasPreemptiveLoadTest
                 }
             }, new SlippyTileSharding(9), new SlippyTile(240, 246, 9), Rectangle.MAXIMUM)
                     .withDeferredLoading(true).withExtendIndefinitely(false);
+    private final Supplier<DynamicAtlasPolicy> allInitialShardsPolicySupplier = () -> new DynamicAtlasPolicy(
+            shard ->
+            {
+                if (this.store.containsKey(shard))
+                {
+                    return Optional.of(this.store.get(shard));
+                }
+                else
+                {
+                    return Optional.empty();
+                }
+            }, new SlippyTileSharding(9),
+            Rectangle.forLocated(new SlippyTile(240, 246, 9).bounds().center(),
+                    new SlippyTile(241, 246, 9).bounds().center()),
+            Rectangle.MAXIMUM).withDeferredLoading(true).withExtendIndefinitely(false);
 
     @Test
     public void loadPreemptivelyTest()
     {
-        this.dynamicAtlas.preemptiveLoad();
-        Assert.assertEquals(3, this.dynamicAtlas.numberOfEdges());
+        final DynamicAtlas dynamicAtlas = new DynamicAtlas(this.policySupplier.get());
+        dynamicAtlas.preemptiveLoad();
+        Assert.assertEquals(3, dynamicAtlas.numberOfEdges());
+        Assert.assertEquals(2, dynamicAtlas.getTimesMultiAtlasWasBuiltUnderneath());
     }
 
-    @Before
-    public void prepare()
+    @Test
+    public void loadPreemptivelyWithAllShardsAsInitialTest()
     {
-        prepare(this.policySupplier.get());
+        final DynamicAtlas dynamicAtlas = new DynamicAtlas(
+                this.allInitialShardsPolicySupplier.get());
+        dynamicAtlas.preemptiveLoad();
+        Assert.assertEquals(3, dynamicAtlas.numberOfEdges());
+        Assert.assertEquals(1, dynamicAtlas.getTimesMultiAtlasWasBuiltUnderneath());
     }
 
-    public void prepare(final DynamicAtlasPolicy policy)
+    @Before
+    public void prepare()
     {
         this.store = new HashMap<>();
         this.store.put(new SlippyTile(240, 246, 9), this.rule.getAtlasZ9x240y246());
         this.store.put(new SlippyTile(240, 245, 9), this.rule.getAtlasZ9x240y245());
         this.store.put(new SlippyTile(241, 245, 9), this.rule.getAtlasZ9x241y245());
         this.store.put(new SlippyTile(241, 246, 9), this.rule.getAtlasZ9x241y246());
-        this.dynamicAtlas = new DynamicAtlas(policy);
     }
 }