Subject: [PATCH] Fix #23097: Improve CPU usage and memory allocations during startup
With the Name Suggestion Index preset added to JOSM, the following methods are relatively expensive during startup (mem old -> mem new, cpu old -> cpu new):
* `XmlObjectParser$Entry.getField` (124 MB -> 8.1 MB, 501ms -> 99ms)
* `XmlObjectParser$Entry.getMethod` (126 MB -> 452 kB, 292ms -> 45ms)
The gains are almost entirely from getting rid of copy calls to Method and Field (done when calling `Class.getMethods()` and `Class.getFields()`). There are further gains in JVM methods (like GC), but those can be a bit ticklish to profile correctly. It does look like a 20% improvement there though (32,653ms -> 26,075ms).
Note: I'm also including a change in `PluginListParser` to avoid compiling a pattern over and over again. That reduces the cost of `PluginListParser.parse` from 25.5 mb to 12.1 mb and 217ms to 162ms. Most of the remaining cost is stuff we cannot do anything about.
Additional note: The PluginListParser numbers included the cost of interning the strings. I ended up removing that since code analysis indicated that the strings were not kept long-term.
---
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
diff --git a/src/org/openstreetmap/josm/plugins/PluginListParser.java b/src/org/openstreetmap/josm/plugins/PluginListParser.java
a
|
b
|
|
11 | 11 | import java.util.LinkedList; |
12 | 12 | import java.util.List; |
13 | 13 | import java.util.jar.Attributes; |
| 14 | import java.util.regex.Matcher; |
| 15 | import java.util.regex.Pattern; |
14 | 16 | |
15 | 17 | import org.openstreetmap.josm.tools.Logging; |
16 | 18 | |
… |
… |
|
62 | 64 | String name = null; |
63 | 65 | String url = null; |
64 | 66 | Attributes manifest = new Attributes(); |
| 67 | final Pattern spaceColonSpace = Pattern.compile("\\s*:\\s*", Pattern.UNICODE_CHARACTER_CLASS); |
| 68 | final Matcher matcher = spaceColonSpace.matcher(""); |
65 | 69 | for (String line = r.readLine(); line != null; line = r.readLine()) { |
66 | 70 | if (line.startsWith("\t")) { |
67 | | final String[] keyValue = line.split("\\s*:\\s*", 2); |
68 | | if (keyValue.length >= 2) |
69 | | manifest.put(new Attributes.Name(keyValue[0].substring(1)), keyValue[1]); |
| 71 | matcher.reset(line); |
| 72 | if (matcher.find() && matcher.start() > 0 && matcher.end() < line.length()) { |
| 73 | final String key = line.substring(1, matcher.start()); |
| 74 | final String value = line.substring(matcher.end()); |
| 75 | manifest.put(new Attributes.Name(key), value); |
| 76 | } |
70 | 77 | continue; |
71 | 78 | } |
72 | 79 | addPluginInformation(ret, name, url, manifest); |
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
diff --git a/src/org/openstreetmap/josm/tools/XmlObjectParser.java b/src/org/openstreetmap/josm/tools/XmlObjectParser.java
a
|
b
|
|
9 | 9 | import java.lang.reflect.Field; |
10 | 10 | import java.lang.reflect.Method; |
11 | 11 | import java.lang.reflect.Modifier; |
12 | | import java.util.Arrays; |
13 | 12 | import java.util.HashMap; |
14 | 13 | import java.util.Iterator; |
15 | 14 | import java.util.LinkedList; |
… |
… |
|
189 | 188 | private final boolean both; |
190 | 189 | private final Map<String, Field> fields = new HashMap<>(); |
191 | 190 | private final Map<String, Method> methods = new HashMap<>(); |
| 191 | /** This is used to avoid array copies in {@link #getUncachedMethod(String)}. Do not modify. */ |
| 192 | private Method[] cachedKlassMethods; |
| 193 | /** This is used to avoid array copies in {@link #getUncachedField(String)}. Do not modify. */ |
| 194 | private Field[] cachedKlassFields; |
192 | 195 | |
193 | 196 | Entry(Class<?> klass, boolean onStart, boolean both) { |
194 | 197 | this.klass = klass; |
… |
… |
|
197 | 200 | } |
198 | 201 | |
199 | 202 | Field getField(String s) { |
200 | | return fields.computeIfAbsent(s, ignore -> Arrays.stream(klass.getFields()) |
201 | | .filter(f -> f.getName().equals(s)) |
202 | | .findFirst() |
203 | | .orElse(null)); |
| 203 | return fields.computeIfAbsent(s, this::getUncachedField); |
| 204 | } |
| 205 | |
| 206 | /** |
| 207 | * Get a field (uncached in {@link #fields}) |
| 208 | * @implNote Please profile startup when changing |
| 209 | * @param s The field to get |
| 210 | * @return The field, or {@code null}. |
| 211 | */ |
| 212 | private Field getUncachedField(String s) { |
| 213 | if (this.cachedKlassFields == null) { |
| 214 | this.cachedKlassFields = klass.getFields(); |
| 215 | } |
| 216 | for (Field field : this.cachedKlassFields) { |
| 217 | if (field.getName().equals(s)) { |
| 218 | return field; |
| 219 | } |
| 220 | } |
| 221 | return null; |
204 | 222 | } |
205 | 223 | |
206 | 224 | Method getMethod(String s) { |
207 | | return methods.computeIfAbsent(s, ignore -> Arrays.stream(klass.getMethods()) |
208 | | .filter(m -> m.getName().equals(s) && m.getParameterTypes().length == 1) |
209 | | .findFirst() |
210 | | .orElse(null)); |
| 225 | return methods.computeIfAbsent(s, this::getUncachedMethod); |
| 226 | } |
| 227 | |
| 228 | /** |
| 229 | * Get an uncached method (in {@link #methods}) |
| 230 | * @implNote Please profile startup when changing |
| 231 | * @param s The method to find |
| 232 | * @return The method or {@code null}. |
| 233 | */ |
| 234 | private Method getUncachedMethod(String s) { |
| 235 | if (cachedKlassMethods == null) { |
| 236 | cachedKlassMethods = klass.getMethods(); |
| 237 | } |
| 238 | for (Method method : cachedKlassMethods) { |
| 239 | if (method.getParameterCount() == 1 && method.getName().equals(s)) { |
| 240 | return method; |
| 241 | } |
| 242 | } |
| 243 | return null; |
211 | 244 | } |
212 | 245 | } |
213 | 246 | |