StreetAddressOrder: Improve performance

This reduces CPU time in getNearbyAddresses by ~85% and memory
allocations by ~97%.

Signed-off-by: Taylor Smock <[email protected]>

.github/workflows/ant-release.yml

@@ -8,7 +8,7 @@ jobs:
   call-workflow:
     uses: JOSM/JOSMPluginAction/.github/workflows/ant.yml@v1
     with:
-      josm-revision: "r18464"
+      josm-revision: "r18589"
       update-pluginssource: true
       plugin-jar-name: 'mapwithai'
     secrets:

.github/workflows/ant.yml

@@ -18,7 +18,7 @@ jobs:
   call-workflow:
     strategy:
       matrix:
-        josm-revision: ["", "r18464"]
+        josm-revision: ["", "r18589"]
     uses: JOSM/JOSMPluginAction/.github/workflows/ant.yml@v1
     with:
       josm-revision: ${{ matrix.josm-revision }}

gradle.properties

@@ -1,9 +1,9 @@
 # The minimum JOSM version this plugin is compatible with (can be any numeric version
-plugin.main.version = 18464
+plugin.main.version = 18589
 # The JOSM version this plugin is currently compiled against
 # Please make sure this version is available at https://josm.openstreetmap.de/download
 # The special values "latest" and "tested" are also possible here, but not recommended.
-plugin.compile.version = 18468
+plugin.compile.version = 18590
 plugin.canloadatruntime = true
 plugin.author = Taylor Smock
 plugin.class = org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin

src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressOrder.java

@@ -13,7 +13,6 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import org.openstreetmap.josm.data.osm.BBox;
 import org.openstreetmap.josm.data.osm.IPrimitive;
@@ -71,14 +70,35 @@ public void visit(Way way) {
      */
     public static List<IPrimitive> getNearbyAddresses(Way way) {
         BBox bbox = StreetAddressTest.expandBBox(new BBox(way.getBBox()), StreetAddressTest.BBOX_EXPANSION);
+        BBox competingHighwaysBbox = StreetAddressTest.expandBBox(new BBox(way.getBBox()),
+                2 * StreetAddressTest.BBOX_EXPANSION);
+        List<Way> competingHighways = way.getDataSet().searchWays(competingHighwaysBbox);
         List<Node> addrNodes = way.getDataSet().searchNodes(bbox).stream()
                 .filter(StreetAddressTest::hasStreetAddressTags).collect(Collectors.toList());
         List<Way> addrWays = way.getDataSet().searchWays(bbox).stream().filter(StreetAddressTest::hasStreetAddressTags)
                 .collect(Collectors.toList());
         List<Relation> addrRelations = way.getDataSet().searchRelations(bbox).stream()
                 .filter(StreetAddressTest::hasStreetAddressTags).collect(Collectors.toList());
-        return Stream.of(addrNodes, addrWays, addrRelations).flatMap(List::stream)
-                .filter(prim -> isNearestRoad(way, prim)).collect(Collectors.toList());
+        ArrayList<IPrimitive> foundObjects = new ArrayList<>(addrNodes.size() + addrWays.size() + addrRelations.size());
+        // This isn't as pretty as using a stream, but it significantly reduces the call
+        // stack size
+        for (Node node : addrNodes) {
+            if (isNearestRoad(way, competingHighways, node)) {
+                foundObjects.add(node);
+            }
+        }
+        for (Way addrWay : addrWays) {
+            if (isNearestRoad(way, competingHighways, addrWay)) {
+                foundObjects.add(addrWay);
+            }
+        }
+        for (Relation rel : addrRelations) {
+            if (isNearestRoad(way, competingHighways, rel)) {
+                foundObjects.add(rel);
+            }
+        }
+        foundObjects.trimToSize();
+        return foundObjects;
     }
 
     /**
@@ -88,11 +108,14 @@ public static List<IPrimitive> getNearbyAddresses(Way way) {
      * @param prim The primitive to get the distance from
      * @return {@code true} if the primitive is the nearest way
      */
-    public static boolean isNearestRoad(Way way, OsmPrimitive prim) {
+    static boolean isNearestRoad(Way way, Collection<Way> competingHighways, OsmPrimitive prim) {
         BBox primBBox = StreetAddressTest.expandBBox(new BBox(prim.getBBox()), StreetAddressTest.BBOX_EXPANSION);
-        List<Pair<Way, Double>> sorted = way.getDataSet().searchWays(primBBox).stream()
-                .filter(StreetAddressTest::isHighway).map(iway -> StreetAddressTest.distanceToWay(iway, prim))
-                .sorted(Comparator.comparing(p -> p.b)).collect(Collectors.toList());
+        List<Pair<Way, Double>> sorted = competingHighways.stream().filter(StreetAddressTest::isHighway)
+                .filter(w -> primBBox.intersects(w.getBBox())).map(iway -> {
+                    Pair<Way, Double> p = StreetAddressTest.distanceToWay(iway, prim);
+                    p.b = Math.sqrt(p.b);
+                    return p;
+                }).sorted(Comparator.comparingDouble(p -> p.b)).collect(Collectors.toList());
 
         if (!sorted.isEmpty()) {
             double minDistance = sorted.get(0).b;

src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressTest.java

@@ -15,6 +15,7 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import org.openstreetmap.josm.data.coor.ILatLon;
 import org.openstreetmap.josm.data.osm.BBox;
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.IPrimitive;
@@ -28,7 +29,7 @@
 import org.openstreetmap.josm.data.validation.TestError;
 import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
 import org.openstreetmap.josm.spi.preferences.Config;
-import org.openstreetmap.josm.tools.Geometry;
+import org.openstreetmap.josm.tools.CheckParameterUtil;
 import org.openstreetmap.josm.tools.Pair;
 
 /**
@@ -40,13 +41,15 @@ public class StreetAddressTest extends Test {
     private static final String ADDR_STREET = "addr:street";
     private final Set<OsmPrimitive> namePrimitiveMap = new HashSet<>();
     /**
-     * Classified highways in order of importance
+     * Classified highways. This uses a {@link Set} instead of a {@link List} since
+     * the MapWithAI code doesn't care about order.
      *
      * Copied from {@link org.openstreetmap.josm.data.validation.tests.Highways}
      */
-    public static final List<String> CLASSIFIED_HIGHWAYS = Collections.unmodifiableList(Arrays.asList("motorway",
-            "motorway_link", "trunk", "trunk_link", "primary", "primary_link", "secondary", "secondary_link",
-            "tertiary", "tertiary_link", "unclassified", "residential", "living_street", "service", "road"));
+    public static final Set<String> CLASSIFIED_HIGHWAYS = Collections
+            .unmodifiableSet(new HashSet<>(Arrays.asList("motorway", "motorway_link", "trunk", "trunk_link", "primary",
+                    "primary_link", "secondary", "secondary_link", "tertiary", "tertiary_link", "unclassified",
+                    "residential", "living_street", "service", "road")));
 
     /**
      * Create a new test object
@@ -127,13 +130,75 @@ private static Collection<Way> getSurroundingHighways(OsmPrimitive address) {
      * Get the distance to a way
      *
      * @param way  The way to get a distance to
-     * @param prim The primitive to get a distance from
+     * @param prim The primitive to get a distance from (LatLon space)
      * @return A Pair&lt;Way, Double&gt; of the distance from the primitive to the
-     *         way
+     *         way.
      */
-    public static Pair<Way, Double> distanceToWay(Way way, OsmPrimitive prim) {
-        return new Pair<>(way, Geometry.getDistance(way, prim));
+    static Pair<Way, Double> distanceToWay(Way way, OsmPrimitive prim) {
+        final Node[] nodes;
+        if (prim instanceof Node) {
+            nodes = new Node[] { (Node) prim };
+        } else if (prim instanceof Way) {
+            nodes = ((Way) prim).getNodes().toArray(new Node[0]);
+        } else if (prim instanceof Relation) {
+            nodes = ((Relation) prim).getMemberPrimitives().stream().filter(p -> p instanceof Node || p instanceof Way)
+                    .flatMap(p -> p instanceof Node ? Stream.of((Node) p) : ((Way) p).getNodes().stream())
+                    .toArray(Node[]::new);
+        } else {
+            throw new IllegalArgumentException("Unknown primitive type: " + prim.getClass());
+        }
+        double dist = Double.NaN;
+        List<Node> wayNodes = way.getNodes();
+        for (int i = 0; i < wayNodes.size() - 1; i++) {
+            final Node a = wayNodes.get(i);
+            final Node b = wayNodes.get(i + 1);
+            for (Node node : nodes) {
+                double tDist = getSegmentNodeDistSq(a, b, node);
+                if (Double.isNaN(tDist) || (!Double.isNaN(tDist) && tDist < dist)) {
+                    dist = tDist;
+                }
+            }
+        }
+        return new Pair<>(way, dist);
+    }
+
+    // ****** START COPY FROM GEOMETRY (EastNorth -> ILatLon, perf is more important
+    // than accuracy, some modifications for zero memalloc) *******
+    /**
+     * Calculate closest distance between a line segment s1-s2 and a point p
+     *
+     * @param p1    start of segment
+     * @param p2    end of segment
+     * @param point the point
+     * @return the square of the euclidean distance from p to the closest point on
+     *         the segment
+     */
+    private static double getSegmentNodeDistSq(ILatLon p1, ILatLon p2, ILatLon point) {
+        CheckParameterUtil.ensureParameterNotNull(p1, "p1");
+        CheckParameterUtil.ensureParameterNotNull(p2, "p2");
+        CheckParameterUtil.ensureParameterNotNull(point, "point");
+
+        double ldx = p2.lon() - p1.lon();
+        double ldy = p2.lat() - p1.lat();
+
+        // segment zero length
+        if (ldx == 0 && ldy == 0)
+            return p1.distanceSq(point);
+
+        double pdx = point.lon() - p1.lon();
+        double pdy = point.lat() - p1.lat();
+
+        double offset = (pdx * ldx + pdy * ldy) / (ldx * ldx + ldy * ldy);
+
+        if (offset <= 0)
+            return p1.distanceSq(point);
+        else if (offset >= 1)
+            return p2.distanceSq(point);
+        // Math copied from ILatLon#interpolate to avoid memory allocation
+        return point.distanceSq((1 - offset) * p1.lon() + offset * p2.lon(),
+                (1 - offset) * p1.lat() + offset * p2.lat());
     }
+    // ****** END COPY FROM GEOMETRY ********
 
     /**
      * Check if the primitive has an appropriate highway tag

src/test/unit/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressOrderTest.java

@@ -18,6 +18,7 @@
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.data.coor.EastNorth;
 import org.openstreetmap.josm.data.coor.LatLon;
+import org.openstreetmap.josm.data.osm.BBox;
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.IPrimitive;
 import org.openstreetmap.josm.data.osm.Node;
@@ -235,30 +236,39 @@ void testIsNearestRoad() {
                 new Node(new LatLon(boxCorners, -boxCorners)));
         Way way2 = TestUtils.newWay("", new Node(new LatLon(-boxCorners, boxCorners)),
                 new Node(new LatLon(-boxCorners, -boxCorners)));
-        for (Way way : Arrays.asList(way1, way2)) {
-            way.getNodes().forEach(ds::addPrimitive);
-            ds.addPrimitive(way);
-        }
+        ds.addPrimitiveRecursive(way1);
+        ds.addPrimitiveRecursive(way2);
 
-        assertFalse(StreetAddressOrder.isNearestRoad(way1, node1));
-        assertFalse(StreetAddressOrder.isNearestRoad(way2, node1));
+        List<Way> competingWays = ds
+                .searchWays(StreetAddressTest.expandBBox(new BBox(node1.getBBox()), StreetAddressTest.BBOX_EXPANSION));
+        // We shouldn't find anything here (no highway tags)
+        assertFalse(StreetAddressOrder.isNearestRoad(way1, competingWays, node1));
+        assertFalse(StreetAddressOrder.isNearestRoad(way2, competingWays, node1));
 
         way1.put("highway", "residential");
         way2.put("highway", "motorway");
 
-        assertTrue(StreetAddressOrder.isNearestRoad(way1, node1));
-        assertTrue(StreetAddressOrder.isNearestRoad(way2, node1));
+        // Both ways are equidistant, so both are the "nearest" road
+        assertTrue(StreetAddressOrder.isNearestRoad(way1, competingWays, node1));
+        assertTrue(StreetAddressOrder.isNearestRoad(way2, competingWays, node1));
 
         node1.setCoor(new LatLon(boxCorners * 0.9, boxCorners * 0.9));
-        assertTrue(StreetAddressOrder.isNearestRoad(way1, node1));
-        assertFalse(StreetAddressOrder.isNearestRoad(way2, node1));
+        competingWays = ds
+                .searchWays(StreetAddressTest.expandBBox(new BBox(node1.getBBox()), StreetAddressTest.BBOX_EXPANSION));
+        // Nearest road should be way1
+        assertTrue(StreetAddressOrder.isNearestRoad(way1, competingWays, node1));
+        assertFalse(StreetAddressOrder.isNearestRoad(way2, competingWays, node1));
 
         node1.setCoor(new LatLon(-boxCorners * 0.9, -boxCorners * 0.9));
-        assertTrue(StreetAddressOrder.isNearestRoad(way2, node1));
-        assertFalse(StreetAddressOrder.isNearestRoad(way1, node1));
+        competingWays = ds
+                .searchWays(StreetAddressTest.expandBBox(new BBox(node1.getBBox()), StreetAddressTest.BBOX_EXPANSION));
+        assertTrue(StreetAddressOrder.isNearestRoad(way2, competingWays, node1));
+        assertFalse(StreetAddressOrder.isNearestRoad(way1, competingWays, node1));
 
         node1.setCoor(new LatLon(0.00005, 0.00005));
-        assertFalse(StreetAddressOrder.isNearestRoad(way2, node1));
-        assertTrue(StreetAddressOrder.isNearestRoad(way1, node1));
+        competingWays = ds
+                .searchWays(StreetAddressTest.expandBBox(new BBox(node1.getBBox()), StreetAddressTest.BBOX_EXPANSION));
+        assertFalse(StreetAddressOrder.isNearestRoad(way2, competingWays, node1));
+        assertTrue(StreetAddressOrder.isNearestRoad(way1, competingWays, node1));
     }
 }