Changeset 18794 in josm for trunk

Timestamp:

2023-08-07T21:10:16+02:00 (19 months ago)

Author:

taylor.smock

Message:

Fix #23085: Improve speed of selecting large amounts of objects

• JVM CPU usage was down ~50%, EDT CPU usage was down ~25%. EDT memory usage was down
• Avoid Stream allocations in ConflictCollection.hasConflictForMy by only looking for a conflict if conflicts exist
• Avoid many string instantiations in DefaultNameFormatter by using cached properties. This significantly reduces memory allocations CPU usage for DefaultNameFormatter methods.
• Avoid some Stream allocations by using standard for loops in DefaultNameFormatter
• Use "" to get the component in PrimitiveRenderer.getListCellRendererComponent -- this reduced the memory allocations by ~50% and CPU usage by ~70% for getListCellRendererComponent by itself, and appears to have no side-effects. We should ask users on different systems with different UI systems if it works properly.
• Significantly reduce cost of Way.hasOnlyLocatableNodes by using a standard for loop instead of a stream -- this isn't as important for this ticket, but was found while profiling. This makes that method have no effective memory allocations and reduces the CPU usage by ~90%.

Location:

trunk/src/org/openstreetmap/josm

Files:

• data/conflict/ConflictCollection.java (modified) (2 diffs)
• data/osm/DefaultNameFormatter.java (modified) (8 diffs)
• data/osm/Way.java (modified) (1 diff)
• gui/PrimitiveRenderer.java (modified) (1 diff)

trunk/src/org/openstreetmap/josm/data/conflict/ConflictCollection.java

@@ -146 +146 @@
      */
     public Conflict<?> getConflictForMy(OsmPrimitive my) {
+        if (conflicts.isEmpty()) {
+            return null;
+        }
         return conflicts.stream()
                 .filter(c -> c.isMatchingMy(my))
@@ -161 +164 @@
      */
     public Conflict<?> getConflictForTheir(OsmPrimitive their) {
+        if (conflicts.isEmpty()) {
+            return null;
+        }
         return conflicts.stream()
                 .filter(c -> c.isMatchingTheir(their))

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

@@ -30 +30 @@
 import org.openstreetmap.josm.data.osm.history.HistoryRelation;
 import org.openstreetmap.josm.data.osm.history.HistoryWay;
+import org.openstreetmap.josm.data.preferences.BooleanProperty;
+import org.openstreetmap.josm.data.preferences.CachingProperty;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPreset;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetNameTemplateList;
@@ -49 +51 @@
 
     private static final List<NameFormatterHook> formatHooks = new LinkedList<>();
+    private static final CachingProperty<Boolean> PROPERTY_SHOW_ID =
+            new BooleanProperty("osm-primitives.showid", false).cached();
+    private static final CachingProperty<Boolean> PROPERTY_SHOW_ID_NEW_PRIMITIVES =
+            new BooleanProperty("osm-primitives.showid.new-primitives", false).cached();
+    private static final CachingProperty<Boolean> PROPERTY_SHOW_VERSION =
+            new BooleanProperty("osm-primitives.showversion", false).cached();
+    private static final CachingProperty<Boolean> PROPERTY_SHOW_COOR =
+            new BooleanProperty("osm-primitives.showcoor", false).cached();
+    private static final CachingProperty<Boolean> PROPERTY_LOCALIZE_NAME =
+            new BooleanProperty("osm-primitives.localize-name", true).cached();
 
     private static final List<String> HIGHWAY_RAILWAY_WATERWAY_LANDUSE_BUILDING = Arrays.asList(
@@ -143 +155 @@
     protected void decorateNameWithId(StringBuilder name, IPrimitive primitive) {
         int version = primitive.getVersion();
-        if (Config.getPref().getBoolean("osm-primitives.showid")) {
-            long id = Config.getPref().getBoolean("osm-primitives.showid.new-primitives") ?
+        if (Boolean.TRUE.equals(PROPERTY_SHOW_ID.get())) {
+            long id = Boolean.TRUE.equals(PROPERTY_SHOW_ID_NEW_PRIMITIVES.get()) ?
                     primitive.getUniqueId() : primitive.getId();
-            if (Config.getPref().getBoolean("osm-primitives.showversion") && version > 0) {
+            if (version > 0 && Boolean.TRUE.equals(PROPERTY_SHOW_VERSION.get())) {
                 name.append(tr(" [id: {0}, v{1}]", id, version));
             } else {
                 name.append(tr(" [id: {0}]", id));
             }
-        } else if (Config.getPref().getBoolean("osm-primitives.showversion")) {
+        } else if (Boolean.TRUE.equals(PROPERTY_SHOW_VERSION.get())) {
             name.append(tr(" [v{0}]", version));
         }
@@ -188 +200 @@
                 preset.nameTemplate.appendText(name, (TemplateEngineDataProvider) node);
             }
-            if (node.isLatLonKnown() && Config.getPref().getBoolean("osm-primitives.showcoor")) {
+            if (node.isLatLonKnown() && Boolean.TRUE.equals(PROPERTY_SHOW_COOR.get())) {
                 name.append(" \u200E(");
                 name.append(CoordinateFormatManager.getDefaultFormat().toString(node, ", "));
@@ -197 +209 @@
 
         String result = name.toString();
-        return formatHooks.stream().map(hook -> hook.checkFormat(node, result))
-                .filter(Objects::nonNull)
-                .findFirst().orElse(result);
-
+        // This avoids memallocs from Stream map, filter and Optional creation.
+        for (NameFormatterHook hook : formatHooks) {
+            final String checkFormat = hook.checkFormat(node, result);
+            if (checkFormat != null) {
+                return checkFormat;
+            }
+        }
+        return result;
     }
 
@@ -275 +291 @@
 
     private static String formatLocalName(IPrimitive osm) {
-        if (Config.getPref().getBoolean("osm-primitives.localize-name", true)) {
+        if (Boolean.TRUE.equals(PROPERTY_LOCALIZE_NAME.get())) {
             return osm.getLocalName();
         } else {
@@ -283 +299 @@
 
     private static String formatLocalName(HistoryOsmPrimitive osm) {
-        if (Config.getPref().getBoolean("osm-primitives.localize-name", true)) {
+        if (Boolean.TRUE.equals(PROPERTY_LOCALIZE_NAME.get())) {
             return osm.getLocalName();
         } else {
@@ -530 +546 @@
      */
     protected void decorateNameWithId(StringBuilder name, HistoryOsmPrimitive primitive) {
-        if (Config.getPref().getBoolean("osm-primitives.showid")) {
+        if (Boolean.TRUE.equals(PROPERTY_SHOW_ID.get())) {
             name.append(tr(" [id: {0}]", primitive.getId()));
         }

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

@@ -635 +635 @@
      */
     public boolean hasOnlyLocatableNodes() {
-        return Arrays.stream(nodes).allMatch(Node::isLatLonKnown);
+        // This is used in various places, some of which are on the UI thread.
+        // If this is called many times, the memory allocations can become prohibitive, if
+        // we use Java streams.
+        // This can be easily tested by loading a large amount of ways into JOSM, and then
+        // selecting everything.
+        for (Node node : nodes) {
+            if (!node.isLatLonKnown()) {
+                return false;
+            }
+        }
+        return true;
     }
 

trunk/src/org/openstreetmap/josm/gui/PrimitiveRenderer.java

@@ -48 +48 @@
     public Component getListCellRendererComponent(JList<? extends IPrimitive> list, IPrimitive value, int index,
             boolean isSelected, boolean cellHasFocus) {
-        Component def = defaultListCellRenderer.getListCellRendererComponent(list, value, index, isSelected, cellHasFocus);
+        // Use an empty string for the value to avoid expensive Object.toString calls, which will happen again in renderer.
+        Component def = defaultListCellRenderer.getListCellRendererComponent(list, "", index, isSelected, cellHasFocus);
         return renderer(def, value, list.getModel().getSize() > 1000);
     }