Changeset 19285 in josm

Timestamp:

2025-01-16T18:42:37+01:00 (5 weeks ago)

Author:

taylor.smock

Message:

Fix #24075: Reduce memory allocations for TaggingPresetItem#matches

This is done by doing the following:

• Converting KeyedItem.match to a MatchType from a String
  • This avoids calling MatchType#ofString repeatedly
  • This does decrease the visibility of the match field and change the type
• Avoiding ArrayList.Itr creation in TaggingPresetItem#matches
  • This does produce some duplicate code, unfortunately.

The KeyedItem.match change reduces memory allocations in KeyedItem#matches by
98% and CPU cycles by 77%.
The TaggingPresetItem#matches change to avoid ArrayList.Itr creation reduces
memory allocations by 100% and CPU cycles by 94% for ArrayList (only looking at
changes between the for loop types).

The net change for TaggingPresetItem#matches is a reduction of memory
allocations by 99% and CPU cycles by 74%. As noted in the ticket, there was a
reduction in GC by ~80%.

Location:

trunk/src/org/openstreetmap/josm

Files:

• data/validation/tests/TagChecker.java (modified) (1 diff)
• gui/tagging/presets/TaggingPresetItem.java (modified) (2 diffs)
• gui/tagging/presets/items/KeyedItem.java (modified) (4 diffs)

trunk/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java

@@ -421 +421 @@
                 for (TaggingPresetItem i : p.data) {
                     if (i instanceof KeyedItem) {
-                        if (!"none".equals(((KeyedItem) i).match))
+                        if (!"none".equals(((KeyedItem) i).match()))
                             minData.add(i);
                         addPresetValue((KeyedItem) i);
                     } else if (i instanceof CheckGroup) {
                         for (Check c : ((CheckGroup) i).checks) {
-                            if (!"none".equals(c.match))
+                            if (!"none".equals(c.match()))
                                 minData.add(c);
                             addPresetValue(c);

trunk/src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetItem.java

@@ -12 +12 @@
 import java.util.List;
 import java.util.Map;
+import java.util.RandomAccess;
 import java.util.Set;
 
@@ -171 +172 @@
     public static boolean matches(Iterable<? extends TaggingPresetItem> data, Map<String, String> tags) {
         boolean atLeastOnePositiveMatch = false;
-        for (TaggingPresetItem item : data) {
-            Boolean m = item.matches(tags);
-            if (m != null && !m)
-                return false;
-            else if (m != null) {
-                atLeastOnePositiveMatch = true;
+        if (data instanceof List && data instanceof RandomAccess) {
+            List<? extends TaggingPresetItem> items = (List<? extends TaggingPresetItem>) data;
+            /* This is a memory allocation optimization, mostly for ArrayList.
+             * In test runs, this reduced the memory cost for this method by 99%.
+             * This appears to have also improved CPU cost for this method by ~10% as well.
+             * The big win for CPU cost is in GC improvements, which was around 80%.
+             * Overall improvement: 7.6 hours to 4.5 hours for validating a Colorado pbf extract (40% improvement).
+             */
+            for (int i = 0; i < items.size(); i++) { // READ ABOVE: DO NOT REPLACE WITH ENHANCED FOR LOOP WITHOUT PROFILING!
+                TaggingPresetItem item = items.get(i);
+                Boolean m = item.matches(tags);
+                if (m != null && !m) {
+                    return false;
+                } else if (m != null) {
+                    atLeastOnePositiveMatch = true;
+                }
             }
+        } else {
+            for (TaggingPresetItem item : data) {
+                Boolean m = item.matches(tags);
+                if (m != null && !m) {
+                    return false;
+                } else if (m != null) {
+                    atLeastOnePositiveMatch = true;
+                }
+            }
         }
         return atLeastOnePositiveMatch;

trunk/src/org/openstreetmap/josm/gui/tagging/presets/items/KeyedItem.java

@@ -52 +52 @@
      * Default is "keyvalue!" for {@link Key} and "none" for {@link Text}, {@link Combo}, {@link MultiSelect} and {@link Check}.
      */
-    public String match = getDefaultMatch().getValue(); // NOSONAR
+    protected MatchType match = getDefaultMatch(); // NOSONAR
 
     /**
@@ -180 +180 @@
             return usage;
         }
+    }
+
+    /**
+     * Allows to change the matching process, i.e., determining whether the tags of an OSM object fit into this preset.
+     * If a preset fits then it is linked in the Tags/Membership dialog.<ul>
+     * <li>none: neutral, i.e., do not consider this item for matching</li>
+     * <li>key: positive if key matches, neutral otherwise</li>
+     * <li>key!: positive if key matches, negative otherwise</li>
+     * <li>keyvalue: positive if key and value matches, neutral otherwise</li>
+     * <li>keyvalue!: positive if key and value matches, negative otherwise</li></ul>
+     * Note that for a match, at least one positive and no negative is required.
+     * Default is "keyvalue!" for {@link Key} and "none" for {@link Text}, {@link Combo}, {@link MultiSelect} and {@link Check}.
+     * @param match The match type. One of <code>none</code>, <code>key</code>, <code>key!</code>, <code>keyvalue</code>,
+     *              or <code>keyvalue!</code>.
+     * @since 19285
+     */
+    public void setMatch(String match) {
+        this.match = MatchType.ofString(match);
+    }
+
+    /**
+     * Get the match type for this item
+     * @return The match type
+     * @since 19285
+     */
+    public String match() {
+        return this.match.getValue();
     }
 
@@ -222 +249 @@
      */
     public boolean isKeyRequired() {
-        final MatchType type = MatchType.ofString(match);
+        final MatchType type = this.match;
         return MatchType.KEY_REQUIRED == type || MatchType.KEY_VALUE_REQUIRED == type;
     }
@@ -244 +271 @@
     @Override
     public Boolean matches(Map<String, String> tags) {
-        switch (MatchType.ofString(match)) {
+        switch (this.match) {
         case NONE:
             return null; // NOSONAR