Skip to content

Commit

Permalink
Fix #24075: Reduce memory allocations for TaggingPresetItem#matches
Browse files Browse the repository at this point in the history
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%.

git-svn-id: https://josm.openstreetmap.de/svn/trunk@19285 0c6e7542-c601-0410-84e7-c038aed88b3b
  • Loading branch information
taylor.smock committed Jan 16, 2025
1 parent c16507a commit 4e5942c
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -420,12 +420,12 @@ public static void initializePresets() {
List<TaggingPresetItem> minData = new ArrayList<>();
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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.util.EnumSet;
import java.util.List;
import java.util.Map;
import java.util.RandomAccess;
import java.util.Set;

import javax.swing.ImageIcon;
Expand Down Expand Up @@ -170,12 +171,31 @@ public static ImageIcon loadImageIcon(String iconName, File zipIcons, Integer ma
*/
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public abstract class KeyedItem extends TextItem implements RegionSpecific {
* 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}.
*/
public String match = getDefaultMatch().getValue(); // NOSONAR
protected MatchType match = getDefaultMatch(); // NOSONAR

/**
* List of regions the preset is applicable for.
Expand Down Expand Up @@ -181,6 +181,33 @@ public Usage splitValues(String delimiter) {
}
}

/**
* 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();
}

/**
* Computes the tag usage for the given key from the given primitives
* @param sel the primitives
Expand Down Expand Up @@ -221,7 +248,7 @@ protected static Usage determineBooleanUsage(Collection<OsmPrimitive> sel, Strin
* @return whether key or key+value are required
*/
public boolean isKeyRequired() {
final MatchType type = MatchType.ofString(match);
final MatchType type = this.match;
return MatchType.KEY_REQUIRED == type || MatchType.KEY_VALUE_REQUIRED == type;
}

Expand All @@ -243,7 +270,7 @@ protected String getKeyTooltipText() {

@Override
public Boolean matches(Map<String, String> tags) {
switch (MatchType.ofString(match)) {
switch (this.match) {
case NONE:
return null; // NOSONAR
case KEY:
Expand Down

0 comments on commit 4e5942c

Please sign in to comment.