Added an allocation rate metric #6604 #6710

egor-ryashin · 2018-12-05T17:17:07Z

Addressing #6604

leventov · 2018-12-05T17:52:52Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+    try {
+      // no Java official public API is present for the following method, so using reflection
+      threadMXBean = ManagementFactory.getThreadMXBean();
+      getThreadAllocatedBytes = threadMXBean.getClass().getMethod("getThreadAllocatedBytes", long[].class);


Could you explain this? I see this method: https://docs.oracle.com/javase/8/docs/jre/api/management/extension/com/sun/management/ThreadMXBean.html#getThreadAllocatedBytes-long:A- in official Java SE API.

Maybe you could do an instanceof check for com.sun.management.ThreadMXBean, I think it should be clearer than reflection.

Classes in the sun.* packages are not part of the public/supported Java API and should not be used directly.
https://www.oracle.com/technetwork/java/faq-sun-packages-142232.html

leventov · 2018-12-05T17:56:08Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+      long[] allThreadIds = threadMXBean.getAllThreadIds();
+      long[] bytes = (long[]) getThreadAllocatedBytes.invoke(threadMXBean, (Object) allThreadIds);
+
+      previousResults = new HashMap<>(allThreadIds.length);


Please use Long2LongOpenHashMap from fastutil.

I bet it's a nano-optimization for ~500 entries.

But it also cost nothing in terms of code complexity. In fact, it adds categorization mental load, "if optimization needed in this case, or not"? So HashMap with primitive elements should simply never be used.

leventov · 2018-12-05T17:56:17Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+      for (int i = 0; i < allThreadIds.length; i++) {
+        long threadId = allThreadIds[i];
+        previousResults.put(threadId, bytes[i]);
+


leventov · 2018-12-05T17:57:58Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+      }
+      initialized = true;
+    }
+    catch (Exception e) {


Could you catch only reflection access exceptions to not accidentally mask some programming mistake?

There is a whole lot of runtime exception types which can be thrown by reflection API (ie IllegalArgumentException and NPE)

I don't see how IllegalArgumentException and NPE are possible. NoSuchMethodException should be rethrown, because it's a programming mistake. IllegalAccessException and SecurityException should be reported in logs.

NoSuchMethodException can be due to a JVM private API change.

You are right.

leventov · 2018-12-05T18:05:58Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+class AllocationMetricCollector
+{
+  private static final Logger log = new Logger(AllocationMetricCollector.class);
+  private Method getThreadAllocatedBytes;


Please make this variable and initialize this as a static

Didn't get it, it's already a variable...

It's an instance field, suggested to make it a static field.

Could be final?

leventov · 2018-12-05T18:06:04Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+{
+  private static final Logger log = new Logger(AllocationMetricCollector.class);
+  private Method getThreadAllocatedBytes;
+  private ThreadMXBean threadMXBean;


Could be final?

leventov · 2018-12-05T18:08:15Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+
+  AllocationMetricCollector()
+  {
+    try {


Since this is a delta metric, maybe it makes more sense to start with an empty map. So the first call to calculateDelta() accounts all data emitted since the Druid application start.

leventov · 2018-12-05T18:14:55Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+        this.previousResults.put(threadId, current);
+      }
+      // remove terminated thread ids
+      previousResults.keySet().retainAll(Arrays.stream(allThreadIds).boxed().collect(Collectors.toList()));


This operation has quadratic complexity. It's easier to build a new map as you iterate allThreadIds and reassign previousResults to that new map here.

I'd say linear (iterate and remove if not exists while remove is constant).

no, retainAll(List) is quadratic, because it calls contains() on the List in the loop over the hash table. #5632 was a similar issue, for example.

leventov · 2018-12-05T18:15:17Z

core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java

+    final ServiceMetricEvent.Builder builder = builder();
+    MonitorUtils.addDimensionsToBuilder(builder, dimensions);
+    Optional<Long> delta = collector.calculateDelta();
+    delta.ifPresent((value) -> emitter.emit(builder.build("jvm/heapAlloc/bytes", value)));


Maybe jvm/heapAlloc/bytes/delta

I'd say .../delta/bytes

Here: #6425 "delta" is the last part, what are the statistics across all metrics?

What statistics? Just checked the code and /delta/* matches > 50 and /delta" just half a dozen.

Yes, I meant the statistics that you provided. Let it be delta/bytes then

leventov · 2018-12-05T18:22:00Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+    if (!initialized) {
+      return Optional.empty();
+    }
+    try {


Also it would be useful to add a comment with the expected (i. e. default) emit period of this metric, and the approximate number of threads in a Druid node, so that readers of this code could quantify the GC and computational overhead of computing this metric.

I understand that the overhead is small, but want to make it more obvious for the readers.

According to this guy experiments: http://openjdk.5641.n7.nabble.com/ThreadMXBean-getCurrentThreadAllocatedBytes-td343872.html
The call to getThreadAllocatedBytes for 500 threads in the worst case takes:

perl -e 'print (500*9000/1000/1000);' 4.5

4.5 ms
AllocationMetricCollector takes linear time to caculate delta, for 500 threads it's negligible. One can call it as freqeuntly as once a second, which is far too frequent for any known purpose to me.

Add this to code to make it evident for code readers

leventov · 2018-12-11T19:45:55Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+        this.previousResults.put(threadId, current);
+      }
+      // remove terminated thread ids
+      previousResults.keySet().retainAll(Arrays.stream(allThreadIds).boxed().collect(Collectors.toSet()));


Now you do the same work three times:

put() in the loop

toSet() - set construction

retainAll()

Instead of doing it once in the loop and then replace the map

leventov · 2018-12-11T19:49:04Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+    }
+  }
+
+  public Optional<Long> calculateDelta()


Logically, once AllocationMetricCollector is created, it should always report the metric, because the risk are inaccessible APIs. So suggested to create a static factory that returns an instance of AllocationMetricCollector if successful, or none if not successful. and then calculateDelta() always returns just long.

I guess then you have to check if the factory returns a true collector, still an additional check anyway.

Yes, but it's coherent from type system perspective - because currently there is a method that returns Optional<Long>, it may seem that it may return empty or full from time to time, but in fact it always returns only one type of thing.

leventov · 2019-01-16T11:27:10Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+  private static Method getThreadAllocatedBytes;
+  private static ThreadMXBean threadMXBean;
+
+  private Map<Long, Long> previousResults;


Using this type instead of Long2LongMap imposes boxing on access, halving the value of primitive collections.

P. S. in fact I want to prohibit using such types altogether: #6861.

leventov · 2019-01-16T11:31:01Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+      previousResults = newResults;
+      return Optional.of(sum);
+    }
+    catch (ReflectiveOperationException e) {


Once you initialized the class successfully I don't believe this is possible.

leventov · 2019-01-21T12:37:33Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+{
+  private static final Logger log = new Logger(AllocationMetricCollector.class);
+  private Method getThreadAllocatedBytes;
+  private ThreadMXBean threadMXBean;


Could be final?

leventov · 2019-01-21T12:37:39Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+class AllocationMetricCollector
+{
+  private static final Logger log = new Logger(AllocationMetricCollector.class);
+  private Method getThreadAllocatedBytes;


Could be final?

leventov · 2019-01-21T12:38:54Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollectorFactory.java

+class AllocationMetricCollectorFactory
+{
+  private static final Logger log = new Logger(AllocationMetricCollectorFactory.class);
+  private static Method getThreadAllocatedBytes;


Some of these fields could be final?

leventov · 2019-01-21T12:39:21Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollectorFactory.java

+import java.lang.management.ThreadMXBean;
+import java.lang.reflect.Method;
+
+class AllocationMetricCollectorFactory


Please add private constructor to prohibit accidental instantiation

This class name is confusing for me because if a class is called "XxxFactory" I expect that I could instantiate it and get an "xxx" by calling an instance method of the factory. Static factory method containers are usually called "Xxxs", i. e. AllocationMetricCollectors in this case.

There's the instance method getAllocationMetricCollector. I don't understand.

leventov · 2019-01-21T12:42:24Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollectorFactory.java

+    }
+  }
+
+  AllocationMetricCollector getAllocationMetricCollector()


Please add @Nullable

Ah, when writing my above comments, I didn't notice that this method is not static. But it could be.

leventov · 2019-01-24T10:06:47Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+  }
+
+  /**
+   * Uses getThreadAllocatedBytes internally.


Please make a Javadoc link

leventov · 2019-01-24T10:21:59Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+   * Tests show the call to getThreadAllocatedBytes for a single thread ID out of 500 threads running takes around
+   * 9000 ns (in the worst case), which for 500 IDs should take 500*9000/1000/1000 = 4.5 ms to the max.
+   * AllocationMetricCollector takes linear time to calculate delta, for 500 threads it's negligible. One can call it as
+   * frequently as once a second, which is far too frequent for any known purpose to me.


Instead of the passage "One can call it as frequently as once a second, which is far too frequent for any known purpose to me." please refer to the default emitting period and where it is configured.

leventov · 2019-01-24T12:51:58Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+
+  /**
+   * Uses getThreadAllocatedBytes internally.
+   * @see com.sun.management.ThreadMXBean#getThreadAllocatedBytes


I don't think @see tags could appear in the middle of the method. I meant just Uses {@link com.sun.management.ThreadMXBean#getThreadAllocatedBytes} internally.

leventov · 2019-01-24T12:53:59Z

core/src/main/java/org/apache/druid/java/util/metrics/AllocationMetricCollector.java

+   * Tests show the call to getThreadAllocatedBytes for a single thread ID out of 500 threads running takes around
+   * 9000 ns (in the worst case), which for 500 IDs should take 500*9000/1000/1000 = 4.5 ms to the max.
+   * AllocationMetricCollector takes linear time to calculate delta, for 500 threads it's negligible.
+   * See the default emitting period.


Yes, exactly! :) I also spent good 5 minutes trying to figure out what is the default emitting period, and failed. But making each reader of this class to perform the same task is not humane.

Addressing apache#6604

Added an allocation rate metric apache#6604

5198535

egor-ryashin requested a review from leventov December 5, 2018 17:20

leventov requested changes Dec 5, 2018

View reviewed changes

leventov added Area - Metrics/Event Emitting Area - Operations labels Dec 5, 2018

leventov reviewed Dec 5, 2018

View reviewed changes

refactoring apache#6604

3d3960a

leventov reviewed Dec 11, 2018

View reviewed changes

egor-ryashin added 2 commits January 10, 2019 22:09

additional refactoring

a55d5ce

Merge remote-tracking branch 'upstream/master' into feature-6604

7305aa9

leventov reviewed Jan 16, 2019

View reviewed changes

egor-ryashin added 2 commits January 20, 2019 23:19

removed long boxing/unboxing

621ea8c

added factory

0777a3e

leventov reviewed Jan 21, 2019

View reviewed changes

egor-ryashin added 2 commits January 21, 2019 19:54

added factory

de55461

refactoring

b352e64

leventov reviewed Jan 24, 2019

View reviewed changes

addressed comments

ba2de49

leventov reviewed Jan 24, 2019

View reviewed changes

addressed comments

67ea769

leventov merged commit 2803fda into apache:master Jan 29, 2019

leventov deleted the feature-6604 branch January 29, 2019 13:16

justinborromeo pushed a commit to justinborromeo/incubator-druid that referenced this pull request Feb 2, 2019

Added an allocation rate metric apache#6604 (apache#6710)

9f98db9

Addressing apache#6604

jon-wei added this to the 0.14.0 milestone Feb 20, 2019

jon-wei mentioned this pull request Feb 22, 2019

0.14.0-incubating release notes #7126

Closed

Added an allocation rate metric #6604 #6710

Added an allocation rate metric #6604 #6710

Uh oh!

Conversation

egor-ryashin commented Dec 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egor-ryashin Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egor-ryashin Dec 6, 2018 •

edited

Loading

leventov Dec 6, 2018 •

edited

Loading

egor-ryashin Dec 7, 2018 •

edited

Loading