Changeset 18478 in josm for trunk/src/org/openstreetmap/josm/data

Timestamp:

2022-06-08T22:20:53+02:00 (3 years ago)

Author:

taylor.smock

Message:

Really fix failing tests in VectorDataSetTest

The test failed ~50% of the time (it depended upon the HashSet ordering).
We now use an ArrayList instead, and a the optimization that caused the
failure is not as significant anymore (node search is significantly cheaper
since r18465).

Location:

trunk/src/org/openstreetmap/josm/data

Files:

• imagery/vectortile/mapbox/MVTTile.java (modified) (3 diffs)
• vector/VectorDataStore.java (modified) (13 diffs)

trunk/src/org/openstreetmap/josm/data/imagery/vectortile/mapbox/MVTTile.java

@@ -5 +5 @@
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
@@ -53 +53 @@
             ProtobufParser parser = new ProtobufParser(inputStream);
             Collection<ProtobufRecord> protobufRecords = parser.allRecords();
-            this.layers = new HashSet<>(protobufRecords.size());
+            this.layers = new ArrayList<>(protobufRecords.size());
             for (ProtobufRecord protoBufRecord : protobufRecords) {
                 if (protoBufRecord.getField() == Layer.LAYER_FIELD) {
@@ -66 +66 @@
                 }
             }
-            this.layers = new HashSet<>(this.layers);
+            this.layers = new ArrayList<>(this.layers);
 
             this.extent = layers.stream().filter(Objects::nonNull).mapToInt(Layer::getExtent).max().orElse(Layer.DEFAULT_EXTENT);

trunk/src/org/openstreetmap/josm/data/vector/VectorDataStore.java

@@ -13 +13 @@
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -22 +21 @@
 import org.openstreetmap.gui.jmapviewer.interfaces.ICoordinate;
 import org.openstreetmap.josm.data.IQuadBucketType;
-import org.openstreetmap.josm.data.coor.ILatLon;
 import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.imagery.vectortile.VectorTile;
@@ -163 +161 @@
 
     private synchronized <T extends Tile & VectorTile> VectorNode pointToNode(T tile, Layer layer,
-      Collection<VectorPrimitive> featureObjects, int x, int y, final Map<ILatLon, VectorNode> nodeMap) {
+      Collection<VectorPrimitive> featureObjects, int x, int y) {
         final BBox tileBbox;
         if (tile instanceof IQuadBucketType) {
@@ -179 +177 @@
                 tileBbox.getMinLon() + (tileBbox.getMaxLon() - tileBbox.getMinLon()) * x / layerExtent
         );
-        if (nodeMap.containsKey(coords)) {
-            return nodeMap.get(coords);
-        }
         final Collection<VectorNode> nodes = this.store
           .searchNodes(new BBox(coords.lon(), coords.lat(), VectorDataSet.DUPE_NODE_DISTANCE));
@@ -212 +207 @@
         node.setCoor(coords);
         featureObjects.add(node);
-        nodeMap.put(node.getCoor(), node);
         return node;
     }
 
     private <T extends Tile & VectorTile> List<VectorWay> pathToWay(T tile, Layer layer,
-      Collection<VectorPrimitive> featureObjects, Path2D shape, Map<ILatLon, VectorNode> nodeMap) {
+      Collection<VectorPrimitive> featureObjects, Path2D shape) {
         final PathIterator pathIterator = shape.getPathIterator(null);
         final List<VectorWay> ways = new ArrayList<>(
-                Utils.filteredCollection(pathIteratorToObjects(tile, layer, featureObjects, pathIterator, nodeMap), VectorWay.class));
+                Utils.filteredCollection(pathIteratorToObjects(tile, layer, featureObjects, pathIterator), VectorWay.class));
         // These nodes technically do not exist, so we shouldn't show them
         for (VectorWay way : ways) {
@@ -234 +228 @@
 
     private <T extends Tile & VectorTile> List<VectorPrimitive> pathIteratorToObjects(T tile, Layer layer,
-      Collection<VectorPrimitive> featureObjects, PathIterator pathIterator, Map<ILatLon, VectorNode> nodeMap) {
+      Collection<VectorPrimitive> featureObjects, PathIterator pathIterator) {
         final List<VectorNode> nodes = new ArrayList<>();
         final double[] coords = new double[6];
@@ -255 +249 @@
             }
             if (PathIterator.SEG_MOVETO == type || PathIterator.SEG_LINETO == type) {
-                final VectorNode node = pointToNode(tile, layer, featureObjects, (int) coords[0], (int) coords[1], nodeMap);
+                final VectorNode node = pointToNode(tile, layer, featureObjects, (int) coords[0], (int) coords[1]);
                 nodes.add(node);
             } else if (PathIterator.SEG_CLOSE != type) {
@@ -272 +266 @@
 
     private <T extends Tile & VectorTile> VectorRelation areaToRelation(T tile, Layer layer,
-      Collection<VectorPrimitive> featureObjects, Area area, Map<ILatLon, VectorNode> nodeMap) {
+      Collection<VectorPrimitive> featureObjects, Area area) {
         VectorRelation vectorRelation = new VectorRelation(layer.getName());
-        for (VectorPrimitive member : pathIteratorToObjects(tile, layer, featureObjects, area.getPathIterator(null), nodeMap)) {
+        for (VectorPrimitive member : pathIteratorToObjects(tile, layer, featureObjects, area.getPathIterator(null))) {
             final String role;
             if (member instanceof VectorWay && ((VectorWay) member).isClosed()) {
@@ -294 +288 @@
         // Using a map reduces the cost of addFeatureData from 2,715,158,632 bytes to 235,042,184 bytes (-91.3%)
         // This was somewhat variant, with some runs being closer to ~560 MB (still -80%).
-        final Map<ILatLon, VectorNode> nodeMap = new HashMap<>();
         for (Layer layer : tile.getLayers()) {
             Map<GeometryTypes, List<Feature>> grouped = layer.getFeatures().stream().collect(Collectors.groupingBy(Feature::getGeometryType));
@@ -300 +293 @@
             for (GeometryTypes type : GeometryTypes.values()) {
                 if (grouped.containsKey(type)) {
-                    addFeatureData(tile, layer, grouped.get(type), nodeMap);
+                    addFeatureData(tile, layer, grouped.get(type));
                 }
             }
@@ -313 +306 @@
     }
 
-    private <T extends Tile & VectorTile> void addFeatureData(T tile, Layer layer, Collection<Feature> features, Map<ILatLon, VectorNode> nodeMap) {
+    private <T extends Tile & VectorTile> void addFeatureData(T tile, Layer layer, Collection<Feature> features) {
         for (Feature feature : features) {
             try {
-                addFeatureData(tile, layer, feature, nodeMap);
+                addFeatureData(tile, layer, feature);
             } catch (IllegalArgumentException e) {
                 Logging.error("Cannot add vector data for feature {0} of tile {1}: {2}", feature, tile, e.getMessage());
@@ -324 +317 @@
     }
 
-    private <T extends Tile & VectorTile> void addFeatureData(T tile, Layer layer, Feature feature, Map<ILatLon, VectorNode> nodeMap) {
+    private <T extends Tile & VectorTile> void addFeatureData(T tile, Layer layer, Feature feature) {
         // This will typically be larger than primaryFeatureObjects, but this at least avoids quite a few ArrayList#grow calls
         List<VectorPrimitive> featureObjects = new ArrayList<>(feature.getGeometryObject().getShapes().size());
         List<VectorPrimitive> primaryFeatureObjects = new ArrayList<>(feature.getGeometryObject().getShapes().size());
         for (Shape shape : feature.getGeometryObject().getShapes()) {
-            primaryFeatureObjects.add(shapeToPrimaryFeatureObject(tile, layer, shape, featureObjects, nodeMap));
+            primaryFeatureObjects.add(shapeToPrimaryFeatureObject(tile, layer, shape, featureObjects));
         }
         final VectorPrimitive primitive;
@@ -374 +367 @@
 
     private <T extends Tile & VectorTile> VectorPrimitive shapeToPrimaryFeatureObject(
-            T tile, Layer layer, Shape shape, List<VectorPrimitive> featureObjects, Map<ILatLon, VectorNode> nodeMap) {
+            T tile, Layer layer, Shape shape, List<VectorPrimitive> featureObjects) {
         final VectorPrimitive primitive;
         if (shape instanceof Ellipse2D) {
             primitive = pointToNode(tile, layer, featureObjects,
-                    (int) ((Ellipse2D) shape).getCenterX(), (int) ((Ellipse2D) shape).getCenterY(), nodeMap);
+                    (int) ((Ellipse2D) shape).getCenterX(), (int) ((Ellipse2D) shape).getCenterY());
         } else if (shape instanceof Path2D) {
-            primitive = pathToWay(tile, layer, featureObjects, (Path2D) shape, nodeMap).stream().findFirst().orElse(null);
+            primitive = pathToWay(tile, layer, featureObjects, (Path2D) shape).stream().findFirst().orElse(null);
         } else if (shape instanceof Area) {
-            primitive = areaToRelation(tile, layer, featureObjects, (Area) shape, nodeMap);
+            primitive = areaToRelation(tile, layer, featureObjects, (Area) shape);
             primitive.put(RELATION_TYPE, MULTIPOLYGON_TYPE);
         } else {