Changeset 17752 in josm for trunk/src

Timestamp:

2021-04-12T00:16:06+02:00 (4 years ago)

Author:

simon04

Message:

see #20745 - Avoid heap allocations due to BBox in IPrimitive.getBBox

Cache and return an immutable bounding box.

37.75% in MapCSSTagCheckerPerformanceTest#testCity amount to BBox::new via IPrimitive.getBBox

Location:

trunk/src/org/openstreetmap/josm

Files:

• data/osm/BBox.java (modified) (4 diffs)
• data/osm/IPrimitive.java (modified) (1 diff)
• data/osm/Relation.java (modified) (1 diff)
• data/osm/Way.java (modified) (2 diffs)
• gui/io/UploadTextComponentValidator.java (modified) (1 diff)

trunk/src/org/openstreetmap/josm/data/osm/BBox.java

@@ -148 +148 @@
     public final void add(double x, double y) {
         if (!Double.isNaN(x) && !Double.isNaN(y)) {
-            xmin = Math.min(xmin, x);
-            xmax = Math.max(xmax, x);
-            ymin = Math.min(ymin, y);
-            ymax = Math.max(ymax, y);
+            set(Math.min(xmin, x), Math.max(xmax, x), Math.min(ymin, y), Math.max(ymax, y));
         }
     }
@@ -161 +158 @@
     public final void add(BBox other) {
         if (other.isValid()) {
-            xmin = Math.min(xmin, other.xmin);
-            xmax = Math.max(xmax, other.xmax);
-            ymin = Math.min(ymin, other.ymin);
-            ymax = Math.max(ymax, other.ymax);
-        }
+            set(Math.min(xmin, other.xmin), Math.max(xmax, other.xmax), Math.min(ymin, other.ymin), Math.max(ymax, other.ymax));
+        }
+    }
+
+    protected void set(double xmin, double xmax, double ymin, double ymax) {
+        this.xmin = xmin;
+        this.xmax = xmax;
+        this.ymin = ymin;
+        this.ymax = ymax;
     }
 
@@ -371 +372 @@
 
     @Override
-    public int hashCode() {
+    public final int hashCode() {
         return Objects.hash(xmin, xmax, ymin, ymax);
     }
 
     @Override
-    public boolean equals(Object o) {
+    public final boolean equals(Object o) {
         if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
+        if (!(o instanceof BBox)) return false;
         BBox b = (BBox) o;
         return Double.compare(b.xmax, xmax) == 0 && Double.compare(b.ymax, ymax) == 0
@@ -451 +452 @@
                 LatLon.cDdFormatter.format(ymax));
     }
+
+    /**
+     * Returns an immutable version of this bbox, i.e., modifying calls throw an {@link UnsupportedOperationException}.
+     * @return an immutable version of this bbox
+     */
+    BBox toImmutable() {
+        return new Immutable(this);
+    }
+
+    private static class Immutable extends BBox {
+
+        Immutable(BBox copy) {
+            super(copy);
+        }
+
+        @Override
+        protected void set(double xmin, double xmax, double ymin, double ymax) {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        BBox toImmutable() {
+            return this;
+        }
+    }
 }

trunk/src/org/openstreetmap/josm/data/osm/IPrimitive.java

@@ -466 +466 @@
     /**
      * Fetches the bounding box of the primitive.
+     * Since 17752, the returned bounding box might be immutable, i.e., modifying calls throw an {@link UnsupportedOperationException}.
      * @return Bounding box of the object
      * @since 13764

trunk/src/org/openstreetmap/josm/data/osm/Relation.java

@@ -441 +441 @@
     @Override
     public BBox getBBox() {
-        if (getDataSet() != null && bbox != null)
-            return new BBox(bbox); // use cached value
+        if (getDataSet() != null && bbox != null) {
+            return this.bbox; // use cached immutable value
+        }
 
         BBox box = new BBox();
         addToBBox(box, new HashSet<PrimitiveId>());
-        if (getDataSet() != null)
-            setBBox(box); // set cache
-        return new BBox(box);
+        if (getDataSet() == null) {
+            return box;
+        }
+        setBBox(box); // set cached immutable value
+        return this.bbox;
     }
 
     private void setBBox(BBox bbox) {
-        this.bbox = bbox;
+        this.bbox = bbox == null ? null : bbox.toImmutable();
     }
 

trunk/src/org/openstreetmap/josm/data/osm/Way.java

@@ -586 +586 @@
             return new BBox(this);
         if (bbox == null) {
-            bbox = new BBox(this);
-        }
-        return new BBox(bbox);
+            setBBox(new BBox(this));
+        }
+        return bbox;
     }
 
@@ -596 +596 @@
     }
 
+    private void setBBox(BBox bbox) {
+        this.bbox = bbox == null ? null : bbox.toImmutable();
+    }
+
     @Override
     public void updatePosition() {
-        bbox = new BBox(this);
+        setBBox(new BBox(this));
         clearCachedStyle();
     }

trunk/src/org/openstreetmap/josm/gui/io/UploadTextComponentValidator.java

@@ -154 +154 @@
             this.area = primitives.stream()
                     .map(IPrimitive::getBBox)
-                    .reduce((b1, b2) -> {
+                    .reduce(new BBox(), (b1, b2) -> {
                         b1.add(b2);
                         return b1;
-                    }).map(BBox::area)
-                    .orElse(Double.NaN);
+                    }).area();
             validate();
         }