From 5cba01696229e6448f483d43422db6645351c9a6 Mon Sep 17 00:00:00 2001 From: Piotr Jastrzebski Date: Mon, 14 May 2018 15:01:46 +0200 Subject: [PATCH 1/2] Remove unnecessary quadratic algorithm from MetricsMBean.register Before this change it was taking JMX Server 270 seconds to start when Scylla had 2000 tables. After the change it takes only 2 seconds. Signed-off-by: Piotr Jastrzebski --- .../scylladb/jmx/metrics/MetricsMBean.java | 61 ++++++++++++++++--- 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/scylladb/jmx/metrics/MetricsMBean.java b/src/main/java/com/scylladb/jmx/metrics/MetricsMBean.java index 90aada6..10821ed 100644 --- a/src/main/java/com/scylladb/jmx/metrics/MetricsMBean.java +++ b/src/main/java/com/scylladb/jmx/metrics/MetricsMBean.java @@ -3,6 +3,11 @@ package com.scylladb.jmx.metrics; import static java.util.Arrays.asList; import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; import java.util.function.Predicate; import java.util.function.Supplier; @@ -25,6 +30,9 @@ import com.sun.jmx.mbeanserver.JmxMBeanServer; * */ public abstract class MetricsMBean extends APIMBean { + private static final Map> registered = new HashMap<>(); + private static final Object registrationLock = new Object(); + private final Collection metrics; public MetricsMBean(APIClient client, Metrics... metrics) { @@ -48,13 +56,50 @@ public abstract class MetricsMBean extends APIMBean { }; } - private void register(MetricsRegistry registry, JmxMBeanServer server) throws MalformedObjectNameException { - // Check if we're the first/last of our type bound/removed. - boolean empty = queryNames(server, getTypePredicate()).isEmpty(); - for (Metrics m : metrics) { - if (empty) { - m.registerGlobals(registry); + // Has to be called with registrationLock hold + private static boolean shouldRegisterGlobals(JmxMBeanServer server, String domainAndType, boolean reversed) { + Map serverMap = registered.get(server); + if (serverMap == null) { + assert !reversed; + serverMap = new HashMap<>(); + serverMap.put(domainAndType, 1); + registered.put(server, serverMap); + return true; + } + Integer count = serverMap.get(domainAndType); + if (count == null) { + assert !reversed; + serverMap.put(domainAndType, 1); + return true; + } + if (reversed) { + --count; + if (count == 0) { + serverMap.remove(domainAndType); + if (serverMap.isEmpty()) { + registered.remove(server); + } + return true; } + serverMap.put(domainAndType, count); + return false; + } else { + serverMap.put(domainAndType, count + 1); + } + return false; + } + + private void register(MetricsRegistry registry, JmxMBeanServer server, boolean reversed) throws MalformedObjectNameException { + // Check if we're the first/last of our type bound/removed. + synchronized (registrationLock) { + boolean registerGlobals = shouldRegisterGlobals(server, name.getDomain() + ":" + name.getKeyProperty("type"), reversed); + if (registerGlobals) { + for (Metrics m : metrics) { + m.registerGlobals(registry); + } + } + } + for (Metrics m : metrics) { m.register(registry); } } @@ -64,7 +109,7 @@ public abstract class MetricsMBean extends APIMBean { // Get name etc. name = super.preRegister(server, name); // Register all metrics in server - register(new MetricsRegistry(client, (JmxMBeanServer) server), (JmxMBeanServer) server); + register(new MetricsRegistry(client, (JmxMBeanServer) server), (JmxMBeanServer) server, false); return name; } @@ -83,7 +128,7 @@ public abstract class MetricsMBean extends APIMBean { } } } - }, server); + }, server, true); } catch (MalformedObjectNameException e) { // TODO : log? } From 862aea4a332718ad293ada7b14adbb1b62e1f59c Mon Sep 17 00:00:00 2001 From: Piotr Jastrzebski Date: Mon, 14 May 2018 16:07:44 +0200 Subject: [PATCH 2/2] Use more efficient MBeans repository Default implementation stores MBeans in the following map: -> ( -> NamedObject) This is problematic because NamedObject contains ObjectName that has both domain and properties inside itself. This means we're storing the same data twice. For domain "" we want to store MBeans in a more compact way using map: ObjectName -> DynamicMBean which is equivalent to NamedObject. Signed-off-by: Piotr Jastrzebski --- .../com/scylladb/jmx/utils/APIBuilder.java | 434 +++++++++++++++++- 1 file changed, 433 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/scylladb/jmx/utils/APIBuilder.java b/src/main/java/com/scylladb/jmx/utils/APIBuilder.java index cc16ac4..ac433ae 100644 --- a/src/main/java/com/scylladb/jmx/utils/APIBuilder.java +++ b/src/main/java/com/scylladb/jmx/utils/APIBuilder.java @@ -4,6 +4,14 @@ package com.scylladb.jmx.utils; */ import static com.scylladb.jmx.main.Main.client; +import static java.util.logging.Level.SEVERE; + +import java.lang.reflect.Field; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.logging.Logger; /* * This file is part of Scylla. @@ -22,18 +30,442 @@ import static com.scylladb.jmx.main.Main.client; * along with Scylla. If not, see . */ +import javax.management.DynamicMBean; +import javax.management.InstanceAlreadyExistsException; +import javax.management.InstanceNotFoundException; import javax.management.MBeanServer; import javax.management.MBeanServerBuilder; import javax.management.MBeanServerDelegate; +import javax.management.MalformedObjectNameException; +import javax.management.ObjectName; +import javax.management.QueryExp; +import javax.management.RuntimeOperationsException; +import com.sun.jmx.interceptor.DefaultMBeanServerInterceptor; import com.sun.jmx.mbeanserver.JmxMBeanServer; +import com.sun.jmx.mbeanserver.NamedObject; +import com.sun.jmx.mbeanserver.Repository; +import com.sun.jmx.mbeanserver.Util; public class APIBuilder extends MBeanServerBuilder { + + private static final Logger logger = Logger.getLogger(APIBuilder.class.getName()); + + private static class TableRepository extends Repository { + private static final Logger logger = Logger.getLogger(TableRepository.class.getName()); + + private final ReentrantReadWriteLock lock= new ReentrantReadWriteLock(); + + private final Map tableMBeans = new HashMap<>(); + + private static boolean isTableMetricName(ObjectName name) { + return isTableMetricDomain(name.getDomain()); + } + + private static boolean isTableMetricDomain(String domain) { + return TableMetricParams.TABLE_METRICS_DOMAIN.equals(domain); + } + + public TableRepository(String defaultDomain) { + super(defaultDomain); + } + + @Override + public String getDefaultDomain() { + return super.getDefaultDomain(); + } + + @Override + public boolean contains(final ObjectName name) { + if (!isTableMetricName(name)) { + return super.contains(name); + } else { + lock.readLock().lock(); + try { + return tableMBeans.containsKey(new TableMetricParams(name)); + } finally { + lock.readLock().unlock(); + } + } + } + + @Override + public String[] getDomains() { + final String[] domains = super.getDomains(); + if (tableMBeans.isEmpty()) { + return domains; + } + final String[] res = new String[domains.length + 1]; + System.arraycopy(domains, 0, res, 0, domains.length); + res[domains.length] = TableMetricParams.TABLE_METRICS_DOMAIN; + return res; + } + + @Override + public Integer getCount() { + lock.readLock().lock(); + try { + return super.getCount() + tableMBeans.size(); + } finally { + lock.readLock().unlock(); + } + } + + @Override + public void addMBean(final DynamicMBean bean, final ObjectName name, final RegistrationContext ctx) + throws InstanceAlreadyExistsException { + if (!isTableMetricName(name)) { + super.addMBean(bean, name, ctx); + } else { + final TableMetricParams key = new TableMetricParams(name); + lock.writeLock().lock(); + try { + if (tableMBeans.containsKey(key)) { + throw new InstanceAlreadyExistsException(name.toString()); + } + tableMBeans.put(key, bean); + if (ctx == null) return; + try { + ctx.registering(); + } catch (RuntimeOperationsException x) { + throw x; + } catch (RuntimeException x) { + throw new RuntimeOperationsException(x); + } + } finally { + lock.writeLock().unlock(); + } + } + } + + @Override + public void remove(final ObjectName name, final RegistrationContext ctx) throws InstanceNotFoundException { + if (!isTableMetricName(name)) { + super.remove(name, ctx); + } else { + final TableMetricParams key = new TableMetricParams(name); + lock.writeLock().lock(); + try { + if (tableMBeans.remove(key) == null) { + throw new InstanceNotFoundException(name.toString()); + } + + if (ctx == null) { + return; + } + try { + ctx.unregistered(); + } catch (Exception x) { + logger.log(SEVERE, "Unexpected error.", x); + } + } finally { + lock.writeLock().lock(); + } + } + } + + @Override + public DynamicMBean retrieve(final ObjectName name) { + if (!isTableMetricName(name)) { + return super.retrieve(name); + } else { + lock.readLock().lock(); + try { + return tableMBeans.get(new TableMetricParams(name)); + } finally { + lock.readLock().unlock(); + } + } + } + + private void addAll(final Set res) { + for (Map.Entry e : tableMBeans.entrySet()) { + try { + res.add(new NamedObject(e.getKey().toName(), e.getValue())); + } catch (MalformedObjectNameException e1) { + // This should never happen + logger.log(SEVERE, "Unexpected error.", e1); + } + } + } + + private void addAllMatching(final Set res, + final ObjectNamePattern pattern) { + for (Map.Entry e : tableMBeans.entrySet()) { + try { + ObjectName name = e.getKey().toName(); + if (pattern.matchKeys(name)) { + res.add(new NamedObject(name, e.getValue())); + } + } catch (MalformedObjectNameException e1) { + // This should never happen + logger.log(SEVERE, "Unexpected error.", e1); + } + } + } + + @Override + public Set query(final ObjectName pattern, final QueryExp query) { + Set res = super.query(pattern, query); + ObjectName name; + if (pattern == null || + pattern.getCanonicalName().length() == 0 || + pattern.equals(ObjectName.WILDCARD)) { + name = ObjectName.WILDCARD; + } else { + name = pattern; + } + + lock.readLock().lock(); + try { + // If pattern is not a pattern, retrieve this mbean ! + if (!name.isPattern() && isTableMetricName(name)) { + final DynamicMBean bean = tableMBeans.get(new TableMetricParams(name)); + if (bean != null) { + res.add(new NamedObject(name, bean)); + return res; + } + } + + // All names in all domains + if (name == ObjectName.WILDCARD) { + addAll(res); + return res; + } + + final String canonical_key_property_list_string = + name.getCanonicalKeyPropertyListString(); + + final boolean allNames = + (canonical_key_property_list_string.length()==0); + final ObjectNamePattern namePattern = + (allNames?null:new ObjectNamePattern(name)); + + // All names in default domain + if (name.getDomain().length() == 0) { + if (isTableMetricDomain(getDefaultDomain())) { + if (allNames) { + addAll(res); + } else { + addAllMatching(res, namePattern); + } + } + return res; + } + + if (!name.isDomainPattern()) { + if (isTableMetricDomain(getDefaultDomain())) { + if (allNames) { + addAll(res); + } else { + addAllMatching(res, namePattern); + } + } + return res; + } + + // Pattern matching in the domain name (*, ?) + final String dom2Match = name.getDomain(); + if (Util.wildmatch(TableMetricParams.TABLE_METRICS_DOMAIN, dom2Match)) { + if (allNames) { + addAll(res); + } else { + addAllMatching(res, namePattern); + } + } + } finally { + lock.readLock().unlock(); + } + return res; + } + } + + private final static class ObjectNamePattern { + private final String[] keys; + private final String[] values; + private final String properties; + private final boolean isPropertyListPattern; + private final boolean isPropertyValuePattern; + + /** + * The ObjectName pattern against which ObjectNames are matched. + **/ + public final ObjectName pattern; + + /** + * Builds a new ObjectNamePattern object from an ObjectName pattern. + * @param pattern The ObjectName pattern under examination. + **/ + public ObjectNamePattern(ObjectName pattern) { + this(pattern.isPropertyListPattern(), + pattern.isPropertyValuePattern(), + pattern.getCanonicalKeyPropertyListString(), + pattern.getKeyPropertyList(), + pattern); + } + + /** + * Builds a new ObjectNamePattern object from an ObjectName pattern + * constituents. + * @param propertyListPattern pattern.isPropertyListPattern(). + * @param propertyValuePattern pattern.isPropertyValuePattern(). + * @param canonicalProps pattern.getCanonicalKeyPropertyListString(). + * @param keyPropertyList pattern.getKeyPropertyList(). + * @param pattern The ObjectName pattern under examination. + **/ + ObjectNamePattern(boolean propertyListPattern, + boolean propertyValuePattern, + String canonicalProps, + Map keyPropertyList, + ObjectName pattern) { + this.isPropertyListPattern = propertyListPattern; + this.isPropertyValuePattern = propertyValuePattern; + this.properties = canonicalProps; + final int len = keyPropertyList.size(); + this.keys = new String[len]; + this.values = new String[len]; + int i = 0; + for (Map.Entry entry : keyPropertyList.entrySet()) { + keys[i] = entry.getKey(); + values[i] = entry.getValue(); + i++; + } + this.pattern = pattern; + } + + /** + * Return true if the given ObjectName matches the ObjectName pattern + * for which this object has been built. + * WARNING: domain name is not considered here because it is supposed + * not to be wildcard when called. PropertyList is also + * supposed not to be zero-length. + * @param name The ObjectName we want to match against the pattern. + * @return true if name matches the pattern. + **/ + public boolean matchKeys(ObjectName name) { + // If key property value pattern but not key property list + // pattern, then the number of key properties must be equal + // + if (isPropertyValuePattern && + !isPropertyListPattern && + (name.getKeyPropertyList().size() != keys.length)) { + return false; + } + + // If key property value pattern or key property list pattern, + // then every property inside pattern should exist in name + // + if (isPropertyValuePattern || isPropertyListPattern) { + for (int i = keys.length - 1; i >= 0 ; i--) { + // Find value in given object name for key at current + // index in receiver + // + String v = name.getKeyProperty(keys[i]); + // Did we find a value for this key ? + // + if (v == null) { + return false; + } + // If this property is ok (same key, same value), go to next + // + if (isPropertyValuePattern && + pattern.isPropertyValuePattern(keys[i])) { + // wildmatch key property values + // values[i] is the pattern; + // v is the string + if (Util.wildmatch(v,values[i])) { + continue; + } else { + return false; + } + } + if (v.equals(values[i])) { + continue; + } + return false; + } + return true; + } + + // If no pattern, then canonical names must be equal + // + final String p1 = name.getCanonicalKeyPropertyListString(); + final String p2 = properties; + return (p1.equals(p2)); + } + } + + public static class TableMetricParams { + public static final String TABLE_METRICS_DOMAIN = "org.apache.cassandra.metrics"; + + private final ObjectName name; + + public TableMetricParams(ObjectName name) { + this.name = name; + } + + public ObjectName toName() throws MalformedObjectNameException { + return name; + } + + private static boolean equal(Object a, Object b) { + return (a == null) ? b == null : a.equals(b); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof TableMetricParams)) { + return false; + } + TableMetricParams oo = (TableMetricParams) o; + return equal(name.getKeyProperty("keyspace"), oo.name.getKeyProperty("keyspace")) + && equal(name.getKeyProperty("scope"), oo.name.getKeyProperty("scope")) + && equal(name.getKeyProperty("name"), oo.name.getKeyProperty("name")) + && equal(name.getKeyProperty("type"), oo.name.getKeyProperty("type")); + } + + private static int hash(Object o) { + return o == null ? 0 : o.hashCode(); + } + + private static int safeAdd(int ... nums) { + long res = 0; + for (int n : nums) { + res = (res + n) % Integer.MAX_VALUE; + } + return (int)res; + } + + @Override + public int hashCode() { + return safeAdd(hash(name.getKeyProperty("keyspace")), + hash(name.getKeyProperty("scope")), + hash(name.getKeyProperty("name")), + hash(name.getKeyProperty("type"))); + } + } + @Override public MBeanServer newMBeanServer(String defaultDomain, MBeanServer outer, MBeanServerDelegate delegate) { // It is important to set |interceptors| to true while creating the JmxMBeanSearver. // It is required for calls to JmxMBeanServer.getMBeanServerInterceptor() to be allowed. JmxMBeanServer nested = (JmxMBeanServer) JmxMBeanServer.newMBeanServer(defaultDomain, outer, delegate, true); + DefaultMBeanServerInterceptor interceptor = (DefaultMBeanServerInterceptor) nested.getMBeanServerInterceptor(); + Field repoField; + try { + repoField = DefaultMBeanServerInterceptor.class.getDeclaredField("repository"); + } catch (NoSuchFieldException | SecurityException e) { + logger.log(SEVERE, "Unexpected error.", e); + throw new RuntimeException(e); + } + repoField.setAccessible(true); + try { + repoField.set(interceptor, new TableRepository(defaultDomain)); + } catch (IllegalArgumentException | IllegalAccessException e) { + logger.log(SEVERE, "Unexpected error.", e); + new RuntimeException(e); + } return new APIMBeanServer(client, nested); } -} \ No newline at end of file +}