Subject: [PATCH] Fix #23085: Significantly reduce cost of selecting all primitives in a large dataset
Direct CPU cycles were reduced by ~30% and memory allocations were reduced by ~70%.
---
Index: src/org/openstreetmap/josm/data/conflict/ConflictCollection.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/org/openstreetmap/josm/data/conflict/ConflictCollection.java b/src/org/openstreetmap/josm/data/conflict/ConflictCollection.java
--- a/src/org/openstreetmap/josm/data/conflict/ConflictCollection.java (revision 18788)
+++ b/src/org/openstreetmap/josm/data/conflict/ConflictCollection.java (date 1690820678515)
@@ -173,7 +173,7 @@
* @return true, if this collection includes a conflict for my; false, otherwise
*/
public boolean hasConflictForMy(OsmPrimitive my) {
- return getConflictForMy(my) != null;
+ return !conflicts.isEmpty() && getConflictForMy(my) != null;
}
/**
Index: src/org/openstreetmap/josm/data/osm/DefaultNameFormatter.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/org/openstreetmap/josm/data/osm/DefaultNameFormatter.java b/src/org/openstreetmap/josm/data/osm/DefaultNameFormatter.java
--- a/src/org/openstreetmap/josm/data/osm/DefaultNameFormatter.java (revision 18788)
+++ b/src/org/openstreetmap/josm/data/osm/DefaultNameFormatter.java (date 1690823766317)
@@ -29,6 +29,8 @@
import org.openstreetmap.josm.data.osm.history.HistoryOsmPrimitive;
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;
import org.openstreetmap.josm.spi.preferences.Config;
@@ -48,6 +50,16 @@
private static DefaultNameFormatter instance;
private static final List formatHooks = new LinkedList<>();
+ private static final CachingProperty PROPERTY_SHOW_ID =
+ new BooleanProperty("osm-primitives.showid", false).cached();
+ private static final CachingProperty PROPERTY_SHOW_ID_NEW_PRIMITIVES =
+ new BooleanProperty("osm-primitives.showid.new-primitives", false).cached();
+ private static final CachingProperty PROPERTY_SHOW_VERSION =
+ new BooleanProperty("osm-primitives.showversion", false).cached();
+ private static final CachingProperty PROPERTY_SHOW_COOR =
+ new BooleanProperty("osm-primitives.showcoor", false).cached();
+ private static final CachingProperty PROPERTY_LOCALIZE_NAME =
+ new BooleanProperty("osm-primitives.localize-name", true).cached();
private static final List HIGHWAY_RAILWAY_WATERWAY_LANDUSE_BUILDING = Arrays.asList(
marktr("highway"), marktr("railway"), marktr("waterway"), marktr("landuse"), marktr("building"));
@@ -142,15 +154,15 @@
*/
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));
}
}
@@ -187,7 +199,7 @@
} else {
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, ", "));
name.append(")\u200C");
@@ -196,10 +208,14 @@
decorateNameWithId(name, node);
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;
}
private final Comparator nodeComparator = Comparator.comparing(this::format);
@@ -274,7 +290,7 @@
}
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 {
return osm.getName();
@@ -282,7 +298,7 @@
}
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 {
return osm.getName();
@@ -529,7 +545,7 @@
* @param primitive the primitive
*/
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()));
}
}
Index: src/org/openstreetmap/josm/data/osm/Way.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/org/openstreetmap/josm/data/osm/Way.java b/src/org/openstreetmap/josm/data/osm/Way.java
--- a/src/org/openstreetmap/josm/data/osm/Way.java (revision 18788)
+++ b/src/org/openstreetmap/josm/data/osm/Way.java (date 1690820678500)
@@ -634,7 +634,17 @@
* @since 13033
*/
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;
}
@Override
Index: src/org/openstreetmap/josm/gui/PrimitiveRenderer.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/org/openstreetmap/josm/gui/PrimitiveRenderer.java b/src/org/openstreetmap/josm/gui/PrimitiveRenderer.java
--- a/src/org/openstreetmap/josm/gui/PrimitiveRenderer.java (revision 18788)
+++ b/src/org/openstreetmap/josm/gui/PrimitiveRenderer.java (date 1690818749381)
@@ -47,7 +47,8 @@
@Override
public Component getListCellRendererComponent(JList 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);
}