Skip to content

Commit

Permalink
StreetAddressTest: Significantly improve performance
Browse files Browse the repository at this point in the history 
This reduces CPU cycles by ~99% and memory allocations by ~74%.

Signed-off-by: Taylor Smock <[email protected]>
  • Loading branch information
@tsmock
tsmock committed Dec 15, 2022
1 parent c453368 commit 84682ba
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 33 deletions.
100 changes: 79 additions & 21 deletions ...ava/org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,31 +4,36 @@
import static org.openstreetmap.josm.tools.I18n.marktr;
import static org.openstreetmap.josm.tools.I18n.tr;

import java.awt.geom.Point2D;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.openstreetmap.josm.data.coor.EastNorth;
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.INode;
import org.openstreetmap.josm.data.osm.IPrimitive;
import org.openstreetmap.josm.data.osm.IWay;
import org.openstreetmap.josm.data.osm.Node;
import org.openstreetmap.josm.data.osm.OsmPrimitive;
import org.openstreetmap.josm.data.osm.Relation;
import org.openstreetmap.josm.data.osm.Way;
import org.openstreetmap.josm.data.projection.ProjectionRegistry;
import org.openstreetmap.josm.data.validation.OsmValidator;
import org.openstreetmap.josm.data.validation.Severity;
import org.openstreetmap.josm.data.validation.Test;
import org.openstreetmap.josm.data.validation.TestError;
import org.openstreetmap.josm.data.validation.util.ValUtil;
import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin;
import org.openstreetmap.josm.spi.preferences.Config;
import org.openstreetmap.josm.tools.CheckParameterUtil;
import org.openstreetmap.josm.tools.Pair;

Expand All @@ -40,6 +45,8 @@ public class StreetAddressTest extends Test {
public static final double BBOX_EXPANSION = 0.002;
private static final String ADDR_STREET = "addr:street";
private final Set<OsmPrimitive> namePrimitiveMap = new HashSet<>();
private final Map<Point2D, Set<String>> nameMap = new HashMap<>();
private final Map<Point2D, List<OsmPrimitive>> primitiveCellMap = new HashMap<>();
/**
* Classified highways. This uses a {@link Set} instead of a {@link List} since
* the MapWithAI code doesn't care about order.
Expand Down Expand Up @@ -76,10 +83,29 @@ public void visit(Node node) {

@Override
public void endTest() {
for (Map.Entry<Point2D, List<OsmPrimitive>> entry : this.primitiveCellMap.entrySet()) {
Collection<String> names = getSurroundingHighwayNames(entry.getKey());
for (OsmPrimitive primitive : entry.getValue()) {
if (!primitive.isOutsideDownloadArea()) {
if (!names.contains(primitive.get(ADDR_STREET))) {
namePrimitiveMap.add(primitive);
}
if (this.isCanceled()) {
break;
}
}
}
if (this.isCanceled()) {
break;
}
}

Map<String, List<OsmPrimitive>> values = namePrimitiveMap.stream()
.collect(Collectors.groupingBy(p -> p.get(ADDR_STREET)));
values.forEach(this::createError);
namePrimitiveMap.clear();
this.nameMap.clear();
this.primitiveCellMap.clear();
}

/**
Expand All @@ -96,34 +122,66 @@ public void createError(String addrStreet, List<OsmPrimitive> primitives) {
}

private void realVisit(OsmPrimitive primitive) {
if (primitive.isUsable() && hasStreetAddressTags(primitive) && !primitive.isOutsideDownloadArea()) {
Collection<Way> surroundingWays = getSurroundingHighways(primitive);
Collection<String> names = getWayNames(surroundingWays);
if (!names.contains(primitive.get(ADDR_STREET))) {
namePrimitiveMap.add(primitive);
if (primitive.isUsable()) {
final double gridDetail = OsmValidator.getGridDetail() / 100;
if (isHighway(primitive)) {
Collection<String> names = getWayNames((Way) primitive);
if (names.isEmpty()) {
return;
}
List<Node> nodes = ((Way) primitive).getNodes();
for (int i = 0; i < nodes.size() - 1; i++) {
// Populate the name map
INode n1 = nodes.get(i);
INode n2 = nodes.get(i + 1);
for (Point2D cell : ValUtil.getSegmentCells(n1, n2, gridDetail)) {
this.nameMap.computeIfAbsent(cell, k -> new HashSet<>()).addAll(names);
}
}
} else if (hasStreetAddressTags(primitive) && !primitive.isOutsideDownloadArea()) {
final EastNorth en;
if (primitive instanceof Node) {
en = ((Node) primitive).getEastNorth();
} else {
en = primitive.getBBox().getCenter().getEastNorth(ProjectionRegistry.getProjection());
}
long x = (long) Math.floor(en.getX() * gridDetail);
long y = (long) Math.floor(en.getY() * gridDetail);
Point2D point = new Point2D.Double(x, y);
primitiveCellMap.computeIfAbsent(point, p -> new ArrayList<>()).add(primitive);
}
}
}

private static Collection<String> getWayNames(Collection<Way> ways) {
return ways.stream().flatMap(w -> w.getInterestingTags().entrySet().stream())
private static Collection<String> getWayNames(Way way) {
return way.getInterestingTags().entrySet().stream()
.filter(e -> (e.getKey().contains("name") || e.getKey().contains("ref"))
&& !e.getKey().contains("tiger"))
.map(Map.Entry::getValue).flatMap(s -> Stream.of(s.split(";", -1))).map(String::trim)
.filter(s -> !s.isEmpty()).collect(Collectors.toSet());
}

private static Collection<Way> getSurroundingHighways(OsmPrimitive address) {
Objects.requireNonNull(address.getDataSet(), "Node must be part of a dataset");
DataSet ds = address.getDataSet();
BBox addrBox = expandBBox(new BBox(address.getBBox()), BBOX_EXPANSION);
int expansions = 0;
int maxExpansions = Config.getPref().getInt("mapwithai.validator.streetaddresstest.maxexpansions", 20);
while (ds.searchWays(addrBox).stream().noneMatch(StreetAddressTest::isHighway) && expansions < maxExpansions) {
expandBBox(addrBox, BBOX_EXPANSION);
expansions++;
private Collection<String> getSurroundingHighwayNames(Point2D point2D) {
if (this.nameMap.isEmpty()) {
return Collections.emptySet();
}
Set<String> surroundingWays = new HashSet<>();
int surrounding = 1;
while (surroundingWays.isEmpty()) {
for (int x = -surrounding; x <= surrounding; x++) {
for (int y = -surrounding; y <= surrounding; y++) {
Point2D key = new Point2D.Double((long) Math.floor(point2D.getX() + x),
(long) Math.floor(point2D.getY() + y));
if (this.nameMap.containsKey(key)) {
surroundingWays.addAll(this.nameMap.get(key));
}
}
}
if (surroundingWays.isEmpty()) {
surrounding++;
}
}
return ds.searchWays(addrBox).stream().filter(StreetAddressTest::isHighway).collect(Collectors.toSet());
return surroundingWays;
}

/**
Expand Down Expand Up @@ -154,7 +212,7 @@ static Pair<Way, Double> distanceToWay(Way way, OsmPrimitive prim) {
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)) {
if (Double.isNaN(dist) || (!Double.isNaN(tDist) && tDist < dist)) {
dist = tDist;
}
}
Expand Down
33 changes: 21 additions & 12 deletions ...org/openstreetmap/josm/plugins/mapwithai/data/validation/tests/StreetAddressTestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import java.lang.reflect.Method;

import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.openstreetmap.josm.TestUtils;
Expand All @@ -17,52 +18,59 @@
import org.openstreetmap.josm.data.osm.DataSet;
import org.openstreetmap.josm.data.osm.Node;
import org.openstreetmap.josm.data.osm.Way;
import org.openstreetmap.josm.data.validation.OsmValidator;
import org.openstreetmap.josm.testutils.JOSMTestRules;
import org.openstreetmap.josm.tools.Geometry;
import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
import org.openstreetmap.josm.tools.Pair;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

@BasicPreferences // Needed due to OsmValidator static initialization initializing tests which in
// turn need preferences
class StreetAddressTestTest {
private final static String ADDR_STREET = "addr:street";
@RegisterExtension
@SuppressFBWarnings("URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
JOSMTestRules test = new JOSMTestRules().projection();
static JOSMTestRules test = new JOSMTestRules().projection();

@BeforeAll
static void setup() {
OsmValidator.initializeGridDetail();
}

@Test
void testVisitWay() throws ReflectiveOperationException {
StreetAddressTest test = new StreetAddressTest();
Way way1 = TestUtils.newWay("", new Node(new LatLon(0, 0)), new Node(new LatLon(1, 1)));
DataSet ds = new DataSet();
way1.getNodes().forEach(ds::addPrimitive);
ds.addPrimitive(way1);
ds.addPrimitiveRecursive(way1);
Node node1 = new Node(new LatLon(1, 1.00001));
node1.put(ADDR_STREET, "Test");
ds.addPrimitive(node1);
test.visit(way1);
test.visit(ds.allPrimitives());
test.endTest();
assertTrue(test.getErrors().isEmpty());
assertFalse(test.getErrors().isEmpty(), "There should be an error, as the way has no name or highway tag");
way1.put("highway", "residential");

test.visit(node1);
test.visit(ds.allPrimitives());
test.endTest();
assertFalse(test.getErrors().isEmpty());

way1.put("name", "Test1");
test.clear();
test.visit(node1);
test.visit(ds.allPrimitives());
test.endTest();
assertFalse(test.getErrors().isEmpty());

way1.put("name", "Test");
test.clear();
test.visit(node1);
test.visit(ds.allPrimitives());
test.endTest();
assertTrue(test.getErrors().isEmpty());

node1.remove(ADDR_STREET);
test.clear();
test.visit(node1);
test.visit(ds.allPrimitives());
test.endTest();
assertTrue(test.getErrors().isEmpty());

Expand All @@ -73,7 +81,7 @@ void testVisitWay() throws ReflectiveOperationException {
setIncomplete.setAccessible(true);
setIncomplete.invoke(firstNode, true);
assertTrue(way1.firstNode().isIncomplete());
test.visit(node1);
test.visit(ds.allPrimitives());
test.endTest();
assertTrue(test.getErrors().isEmpty());
}
Expand All @@ -89,7 +97,8 @@ void testDistanceToWay() {

distance = StreetAddressTest.distanceToWay(way1, node1);
assertSame(way1, distance.a);
assertEquals(Geometry.getDistance(way1, node1), distance.b, 0.0);
assertEquals(0, distance.b, 0.0,
"node1 should be on the line (lat-lon). It is not quite on the line (East-North)");
}

@Test
Expand Down

0 comments on commit 84682ba

Please sign in to comment.