From c45336836883a1ec7be4a5b0855da6ecf197a50e Mon Sep 17 00:00:00 2001 From: Taylor Smock Date: Wed, 14 Dec 2022 10:36:24 -0700 Subject: [PATCH] StreetAddressOrder: Improve performance This reduces CPU time in getNearbyAddresses by ~85% and memory allocations by ~97%. Signed-off-by: Taylor Smock --- .github/workflows/ant-release.yml | 2 +- .github/workflows/ant.yml | 2 +- gradle.properties | 4 +- .../validation/tests/StreetAddressOrder.java | 37 +++++++-- .../validation/tests/StreetAddressTest.java | 83 +++++++++++++++++-- .../tests/StreetAddressOrderTest.java | 38 +++++---- 6 files changed, 132 insertions(+), 34 deletions(-) diff --git a/.github/workflows/ant-release.yml b/.github/workflows/ant-release.yml index 981ae490..984d88cb 100644 --- a/.github/workflows/ant-release.yml +++ b/.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: diff --git a/.github/workflows/ant.yml b/.github/workflows/ant.yml index 3176beb8..7cd280f1 100644 --- a/.github/workflows/ant.yml +++ b/.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 }} diff --git a/gradle.properties b/gradle.properties index 72a9ca7b..f2948c2c 100644 --- a/gradle.properties +++ b/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 diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressOrder.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressOrder.java index a3150298..6d4516a4 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressOrder.java +++ b/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 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 competingHighways = way.getDataSet().searchWays(competingHighwaysBbox); List addrNodes = way.getDataSet().searchNodes(bbox).stream() .filter(StreetAddressTest::hasStreetAddressTags).collect(Collectors.toList()); List addrWays = way.getDataSet().searchWays(bbox).stream().filter(StreetAddressTest::hasStreetAddressTags) .collect(Collectors.toList()); List 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 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 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 competingHighways, OsmPrimitive prim) { BBox primBBox = StreetAddressTest.expandBBox(new BBox(prim.getBBox()), StreetAddressTest.BBOX_EXPANSION); - List> 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> sorted = competingHighways.stream().filter(StreetAddressTest::isHighway) + .filter(w -> primBBox.intersects(w.getBBox())).map(iway -> { + Pair 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; diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressTest.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressTest.java index 31927384..bfd56dab 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressTest.java +++ 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 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 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 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 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<Way, Double> of the distance from the primitive to the - * way + * way. */ - public static Pair distanceToWay(Way way, OsmPrimitive prim) { - return new Pair<>(way, Geometry.getDistance(way, prim)); + static Pair 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 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 diff --git a/src/test/unit/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressOrderTest.java b/src/test/unit/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressOrderTest.java index 670a4ae0..89906126 100644 --- a/src/test/unit/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressOrderTest.java +++ b/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 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)); } }