Skip to content

Commit

Permalink
(PE-38408) Remove expensive Regexes from puppet profiler Java impl
Browse files Browse the repository at this point in the history 
We have triggered a nasty performance regression in the JVM with some of
our Regex usage in the MetricsPuppetProfiler. Regardless of the
performance regression, our Regex usage is unnecessary and unneedlessly
expensive.
  • Loading branch information
@justinstoller
justinstoller committed Sep 23, 2024
1 parent 0475b27 commit ba20c99
Showing 1 changed file with 53 additions and 26 deletions.
79 changes: 53 additions & 26 deletions src/java/com/puppetlabs/puppetserver/MetricsPuppetProfiler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import org.apache.commons.lang.StringUtils;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -21,17 +22,21 @@ public class MetricsPuppetProfiler implements PuppetProfiler {
private final MetricRegistry registry;
private final Set<String> metric_ids;

private static final Pattern FUNCTION_PATTERN = Pattern.compile(".*\\.functions\\.([\\w\\d_]+)$");
private static final Pattern RESOURCE_PATTERN = Pattern.compile(".*\\.compiler\\.evaluate_resource\\.([\\w\\d_]+\\[([\\w\\d_]+::)*[\\w\\d_]+\\])$");
private static final Pattern CATALOG_PATTERN = Pattern.compile(".*\\.compiler\\.(static_compile_postprocessing|static_compile|compile|find_node)$");
private static final Pattern INLINING_PATTERN = Pattern.compile(".*\\.compiler\\.static_compile_inlining\\.(.*)$");
private static final Pattern PUPPETDB_PATTERN = Pattern.compile(".*\\.puppetdb\\.(resource\\.search|facts\\.encode|command\\.submit\\.replace facts|catalog\\.munge|command\\.submit\\.replace catalog|report\\.convert_to_wire_format_hash|command\\.submit\\.store report|query)$");

private final Map<String, Timer> function_timers;
private final Map<String, Timer> resource_timers;
private final Map<String, Timer> catalog_timers;
private final Map<String, Timer> inlining_timers;
private final Map<String, Timer> puppetdb_timers;

public MetricsPuppetProfiler(String hostname, MetricRegistry registry) {
this.hostname = hostname;
this.registry = registry;
this.metric_ids = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
this.function_timers = new ConcurrentHashMap<String, Timer>();
this.resource_timers = new ConcurrentHashMap<String, Timer>();
this.catalog_timers = new ConcurrentHashMap<String, Timer>();
this.inlining_timers = new ConcurrentHashMap<String, Timer>();
this.puppetdb_timers = new ConcurrentHashMap<String, Timer>();
}

@Override
Expand All @@ -43,9 +48,12 @@ public Object start(String message, String[] metric_id) {
public void finish(Object context, String message, String[] metric_id) {
if (shouldTime(metric_id)) {
Long elapsed = System.currentTimeMillis() - (Long)context;
for (Timer t : getTimers(metric_id)) {
Map<String, Timer> metricsByID = getOrCreateTimersByIDs(metric_id);
for (Timer t : metricsByID.values()) {
t.update(elapsed, TimeUnit.MILLISECONDS);
}

updateMetricsTrackers(metric_id, metricsByID);
}
}

Expand All @@ -54,29 +62,59 @@ public Set<String> getAllMetricIds() {
}

public Map<String, Timer> getFunctionTimers() {
return getTimers(FUNCTION_PATTERN);
return this.function_timers;
}

public Map<String, Timer> getResourceTimers() {
return getTimers(RESOURCE_PATTERN);
return this.resource_timers;
}

public Map<String, Timer> getCatalogTimers() {
return getTimers(CATALOG_PATTERN);
return this.catalog_timers;
}

public Map<String, Timer> getInliningTimers() {
return getTimers(INLINING_PATTERN);
return this.inlining_timers;
}

public Map<String, Timer> getPuppetDBTimers() {
return getTimers(PUPPETDB_PATTERN);
return this.puppetdb_timers;
}

@Override
public void shutdown() {
}

private List<String> sliceOfArrayToList(String[] idSegments, int lengthOfID) {
// Callers expect a mutable List returned, but Arrays.asList() returns a
// fix length array, which is why we have to create a List and then add to it.
List<String> idList = new ArrayList<String>();
idList.addAll(Arrays.asList(Arrays.copyOf(idSegments, lengthOfID)));

return idList;
}

private void updateMetricsTrackers(String[] metricId, Map<String, Timer> metricsByID) {
if ("functions".equals(metricId[0])) {
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 2)));
this.function_timers.put(metricId[1], metric);
} else if ("compiler".equals(metricId[0])) {
if ("evaluate_resource".equals(metricId[1])) {
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 3)));
this.resource_timers.put(metricId[2], metric);
} else if ("static_compile_inlining".equals(metricId[1])) {
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 3)));
this.inlining_timers.put(metricId[2], metric);
} else {
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 2)));
this.catalog_timers.put(metricId[1], metric);
}
} else if ("puppetdb".equals(metricId[0])) {
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 2)));
this.puppetdb_timers.put(metricId[1], metric);
}
}

private boolean shouldTime(String[] metric_id) {
if (metric_id == null) {
return false;
Expand All @@ -90,8 +128,8 @@ private boolean shouldTime(String[] metric_id) {
return true;
}

private List<Timer> getTimers(String[] metric_id) {
List<Timer> timers = new ArrayList<Timer>();
private Map<String, Timer> getOrCreateTimersByIDs(String[] metric_id) {
Map<String, Timer> timers = new HashMap<String, Timer>();
// If this is turns out to be a performance hit, we could cache these in a
// map or something.
for (int i = 0; i < metric_id.length; i++) {
Expand All @@ -101,7 +139,7 @@ private List<Timer> getTimers(String[] metric_id) {
}
String metric_name = getMetricName(current_id);
registerMetricName(metric_name);
timers.add(registry.timer(metric_name));
timers.put(metric_name, registry.timer(metric_name));
}
return timers;
}
Expand All @@ -114,15 +152,4 @@ private String getMetricName(List<String> metric_id) {
private void registerMetricName(String metric_name) {
this.metric_ids.add(metric_name);
}

private Map<String, Timer> getTimers(Pattern pattern) {
Map<String, Timer> rv = new HashMap<>();
for (String metric_id : this.metric_ids) {
Matcher matcher = pattern.matcher(metric_id);
if (matcher.matches()) {
rv.put(matcher.group(1), registry.timer(metric_id));
}
}
return rv;
}
}

0 comments on commit ba20c99

Please sign in to comment.