WaySectionProcessor performance improvements (#210)

* tentative simplifications * spotless * temporary integration test fix * spotless * counts work now * spotless * fix tests failures * fix integration test and spotless * WaySectionProcessor speed improvement * revert bareatlas changes in this branch * Keep subatlas code available * spotless * added unit test for shard cutting
osmlab · Aug 31, 2018 · b1a0e34 · b1a0e34
1 parent bee15d4
commit b1a0e34
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 33 deletions.
diff --git a/...grationTest/java/org/openstreetmap/atlas/geography/atlas/raw/RawAtlasIntegrationTest.java b/...grationTest/java/org/openstreetmap/atlas/geography/atlas/raw/RawAtlasIntegrationTest.java
@@ -142,9 +142,9 @@ public void testPbfToSlicedAtlasWithExpansion()
                 .edges(edge -> edge.getOsmIdentifier() == LINE_OSM_IDENTIFIER_CROSSING_3_SHARDS);
 
         // First look at absolute counts. Each shard will have two forward and reverse edges
-        Assert.assertTrue(Iterables.size(firstGroupOfEdges) == 4);
-        Assert.assertTrue(Iterables.size(secondGroupOfEdges) == 4);
-        Assert.assertTrue(Iterables.size(thirdGroupOfEdges) == 4);
+        Assert.assertEquals(4, Iterables.size(firstGroupOfEdges));
+        Assert.assertEquals(4, Iterables.size(secondGroupOfEdges));
+        Assert.assertEquals(4, Iterables.size(thirdGroupOfEdges));
 
         // Next, let's check identifier consistency
         final Set<Long> uniqueIdentifiers = new HashSet<>();
@@ -168,9 +168,9 @@ public void testPbfToSlicedAtlasWithExpansion()
         Assert.assertTrue(piece3from123.asPolyLine().equals(piece3from62.asPolyLine()));
 
         // Let's validate absolute number of edges in each shard
-        Assert.assertTrue(atlasFromz8x123y122.numberOfEdges() == 12);
-        Assert.assertTrue(atlasFromz8x123y123.numberOfEdges() == 16);
-        Assert.assertTrue(atlasFromz7x62y61.numberOfEdges() == 20);
+        Assert.assertEquals(12, atlasFromz8x123y122.numberOfEdges());
+        Assert.assertEquals(16, atlasFromz8x123y123.numberOfEdges());
+        Assert.assertEquals(20, atlasFromz7x62y61.numberOfEdges());
     }
 
     @Test
@@ -282,11 +282,11 @@ public void testSectioningFromShard()
                         .getFile())),
                 rawAtlasFetcher).run();
 
-        Assert.assertEquals(5011, finalAtlas.numberOfNodes());
-        Assert.assertEquals(9764, finalAtlas.numberOfEdges());
-        Assert.assertEquals(5128, finalAtlas.numberOfAreas());
+        Assert.assertEquals(5009, finalAtlas.numberOfNodes());
+        Assert.assertEquals(9760, finalAtlas.numberOfEdges());
+        Assert.assertEquals(5126, finalAtlas.numberOfAreas());
         Assert.assertEquals(184, finalAtlas.numberOfPoints());
-        Assert.assertEquals(326, finalAtlas.numberOfLines());
+        Assert.assertEquals(271, finalAtlas.numberOfLines());
         Assert.assertEquals(14, finalAtlas.numberOfRelations());
     }
 

diff --git a/...main/java/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessor.java b/...main/java/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessor.java
@@ -82,16 +82,15 @@ public class WaySectionProcessor
 
     // Expand the initial shard boundary to capture any edges that are crossing the shard boundary
     private static final Distance SHARD_EXPANSION_DISTANCE = Distance.meters(20);
+    private static final boolean USE_SUB_ATLAS = false;
 
     private final Atlas rawAtlas;
     private final AtlasLoadingOption loadingOption;
 
     private final List<Shard> loadedShards = new ArrayList<>();
 
     // Bring in all points that are part of any line that will become an edge
-    private final Predicate<AtlasEntity> pointPredicate = entity -> entity instanceof Point
-            && Iterables.stream(entity.getAtlas().linesContaining(((Point) entity).getLocation()))
-                    .anyMatch(this::isAtlasEdge);
+    private final Predicate<AtlasEntity> pointPredicate = entity -> entity instanceof Point;
 
     // TODO - we are pulling in all edges and their contained points in the shard. We can optimize
     // this further by only considering the edges crossing the shard boundary and their intersecting
@@ -244,19 +243,26 @@ private Atlas buildExpandedAtlas(final Shard initialShard, final Sharding shardi
                 final Optional<Atlas> possibleAtlas = rawAtlasFetcher.apply(shard);
                 logTaskAsInfo(SHARD_SPECIFIC_COMPLETED_TASK_MESSAGE, getShardOrAtlasName(),
                         ATLAS_FETCHING_TASK, shard.getName(), fetchTime.elapsedSince());
-
-                if (possibleAtlas.isPresent())
+                if (USE_SUB_ATLAS)
+                {
+                    if (possibleAtlas.isPresent())
+                    {
+                        this.loadedShards.add(shard);
+                        final Atlas atlas = possibleAtlas.get();
+                        final Time subAtlasTime = Time.now();
+                        final Optional<Atlas> subAtlas = atlas
+                                .subAtlas(this.dynamicAtlasExpansionFilter);
+                        logTaskAsInfo(SHARD_SPECIFIC_COMPLETED_TASK_MESSAGE, getShardOrAtlasName(),
+                                SUB_ATLAS_CUTTING_TASK, shard.getName(),
+                                subAtlasTime.elapsedSince());
+                        return subAtlas;
+                    }
+                    return Optional.empty();
+                }
+                else
                 {
-                    this.loadedShards.add(shard);
-                    final Atlas atlas = possibleAtlas.get();
-                    final Time subAtlasTime = Time.now();
-                    final Optional<Atlas> subAtlas = atlas
-                            .subAtlas(this.dynamicAtlasExpansionFilter);
-                    logTaskAsInfo(SHARD_SPECIFIC_COMPLETED_TASK_MESSAGE, getShardOrAtlasName(),
-                            SUB_ATLAS_CUTTING_TASK, shard.getName(), subAtlasTime.elapsedSince());
-                    return subAtlas;
+                    return possibleAtlas;
                 }
-                return Optional.empty();
             }
         };
 
@@ -515,22 +521,23 @@ private Atlas cutSubAtlasForOriginalShard(final Atlas atlas)
     {
         try
         {
-            if (this.loadedShards.size() > 1)
+            if (!this.loadedShards.isEmpty())
             {
                 // The first shard is always the initial one. Use its bounds to build the atlas.
                 final Rectangle originalShardBounds = this.loadedShards.get(0).bounds();
-                return atlas.subAtlas(originalShardBounds).get();
+                return atlas.subAtlas(originalShardBounds)
+                        .orElseThrow(() -> new CoreException(
+                                "Cannot have an empty atlas after way sectioning {}",
+                                this.loadedShards.get(0).getName()));
             }
             else
             {
-                // We don't need to cut anything away, since no other shards were loaded
                 return atlas;
             }
         }
         catch (final Exception e)
         {
-            logger.error("Error creating sub-atlas for original shard bounds", e);
-            return null;
+            throw new CoreException("Error creating sub-atlas for original shard bounds", e);
         }
     }
 
@@ -742,9 +749,16 @@ private boolean isAtlasNode(final WaySectionChangeSet changeSet, final Point poi
     private boolean isAtlasPoint(final WaySectionChangeSet changeSet, final Point point)
     {
         final boolean hasExplicitOsmTags = pointHasExplicitOsmTags(point);
+
+        // We use the presence of explicit OSM tagging to determine if it's a point
+        if (hasExplicitOsmTags)
+        {
+            return true;
+        }
+
         final boolean isRelationMember = !point.relations().isEmpty();
 
-        if (isRelationMember && !hasExplicitOsmTags && !isAtlasNode(changeSet, point))
+        if (isRelationMember && !isAtlasNode(changeSet, point))
         {
             // When the OSM node is part of a relation, doesn't have explicit OSM tagging and is not
             // at an intersection (not an atlas node), then we want to create an atlas point so we
@@ -754,16 +768,15 @@ private boolean isAtlasPoint(final WaySectionChangeSet changeSet, final Point po
 
         final boolean isIsolatedNode = Iterables
                 .isEmpty(this.rawAtlas.linesContaining(point.getLocation()));
-        if (!isRelationMember && !hasExplicitOsmTags && isIsolatedNode)
+        if (!isRelationMember && isIsolatedNode)
         {
             // This is a special case - when an OSM node is not part of a relation, doesn't have
             // explicit OSM tagging and is not a part of an OSM way, then we want to bring it in as
             // a stand-alone Atlas point and differentiate this case from a simple shape point.
             return true;
         }
 
-        // All other times, we use the presence of explicit OSM tagging to determine if it's a point
-        return hasExplicitOsmTags;
+        return false;
     }
 
     /**

diff --git a/.../java/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessorTest.java b/.../java/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessorTest.java
@@ -3,6 +3,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 
 import org.junit.Assert;
@@ -15,6 +16,8 @@
 import org.openstreetmap.atlas.geography.atlas.pbf.AtlasLoadingOption;
 import org.openstreetmap.atlas.geography.atlas.raw.slicing.LineAndPointSlicingTest;
 import org.openstreetmap.atlas.geography.boundary.CountryBoundaryMap;
+import org.openstreetmap.atlas.geography.sharding.SlippyTile;
+import org.openstreetmap.atlas.geography.sharding.SlippyTileSharding;
 import org.openstreetmap.atlas.streaming.compression.Decompressor;
 import org.openstreetmap.atlas.streaming.resource.InputStreamResource;
 import org.openstreetmap.atlas.tags.SyntheticInvalidWaySectionTag;
@@ -59,6 +62,30 @@ public void testBidirectionalRing()
         Assert.assertTrue(finalAtlas.edge(-317579533000002L) != null);
     }
 
+    @Test
+    public void testCutAtShardBoundary()
+    {
+        final Atlas slicedRawAtlas = this.setup.getRawAtlasSpanningOutsideBoundary();
+        final Atlas finalAtlas = new WaySectionProcessor(SlippyTile.forName("8-123-123"),
+                AtlasLoadingOption.createOptionWithAllEnabled(COUNTRY_BOUNDARY_MAP),
+                new SlippyTileSharding(8), shard -> Optional.of(slicedRawAtlas)).run();
+
+        // Assert the number to make sure the edges outside the shard have been excluded
+        Assert.assertEquals(4, finalAtlas.numberOfNodes());
+        Assert.assertEquals(3, finalAtlas.numberOfEdges());
+
+        // Nodes
+        Assert.assertNotNull(finalAtlas.node(112428000000L));
+        Assert.assertNotNull(finalAtlas.node(112430000000L));
+        Assert.assertNotNull(finalAtlas.node(112441000000L));
+        Assert.assertNotNull(finalAtlas.node(112427000000L));
+
+        // Edges
+        Assert.assertNotNull(finalAtlas.edge(112429000001L));
+        Assert.assertNotNull(finalAtlas.edge(112429000002L));
+        Assert.assertNotNull(finalAtlas.edge(112440000001L));
+    }
+
     @Test
     public void testLineWithLoopAtEnd()
     {

diff --git a/...a/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessorTestRule.java b/...a/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessorTestRule.java
@@ -59,6 +59,9 @@ public class WaySectionProcessorTestRule extends CoreTestRule
     @TestAtlas(loadFromTextResource = "wayExceedingSectioningLimit.atlas.txt")
     private Atlas wayExceedingSectioningLimit;
 
+    @TestAtlas(loadFromTextResource = "rawAtlasSpanningOutsideBoundary.atlas.txt")
+    private Atlas rawAtlasSpanningOutsideBoundary;
+
     public Atlas getBidirectionalRingAtlas()
     {
         return this.bidirectioalRingAtlas;
@@ -109,6 +112,11 @@ public Atlas getOneWaySimpleLineAtlas()
         return this.oneWaySimpleLine;
     }
 
+    public Atlas getRawAtlasSpanningOutsideBoundary()
+    {
+        return this.rawAtlasSpanningOutsideBoundary;
+    }
+
     public Atlas getReversedOneWayLineAtlas()
     {
         return this.reversedOneWayLine;

diff --git a/...nstreetmap/atlas/geography/atlas/raw/sectioning/rawAtlasSpanningOutsideBoundary.atlas.txt b/...nstreetmap/atlas/geography/atlas/raw/sectioning/rawAtlasSpanningOutsideBoundary.atlas.txt
@@ -0,0 +1,18 @@
+# Nodes
+# Edges
+# Areas
+# Lines
+112429000000 && 7.0133271,-7.0304936:7.0128905,-7.0306545:7.0123688,-7.0304453 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || highway -> motorway || last_edit_version -> 1 || oneway -> yes
+112440000000 && 7.0128905,-7.0306545:7.0131088,-7.0316684:7.012832,-7.0324623:7.0130236,-7.0330041 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || highway -> motorway || last_edit_version -> 1 || oneway -> yes
+112452000000 && 7.0133697,-7.0323443:7.012832,-7.0324623:7.0122357,-7.0323121 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || highway -> motorway || last_edit_version -> 1 || oneway -> yes
+# Points
+112404000000 && 6.9272056,-6.7232943 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || last_edit_version -> 1
+112427000000 && 7.0133271,-7.0304936 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || last_edit_version -> 1
+112428000000 && 7.0128905,-7.0306545 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || last_edit_version -> 1
+112430000000 && 7.0123688,-7.0304453 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || last_edit_version -> 1
+112439000000 && 7.0131088,-7.0316684 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || last_edit_version -> 1
+112441000000 && 7.012832,-7.0324623 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || last_edit_version -> 1
+112449000000 && 7.0130236,-7.0330041 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || last_edit_version -> 1
+112451000000 && 7.0133697,-7.0323443 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || last_edit_version -> 1
+112453000000 && 7.0122357,-7.0323121 && last_edit_user_name -> myself || last_edit_changeset -> 1 || last_edit_time -> 1513719782000 || last_edit_user_id -> 1 || last_edit_version -> 1
+# Relations