Replace ArrayBlockingQueue with jctools queue. #3034

anuraaga · 2021-03-17T05:39:35Z

The minimized trace-shaded-deps jar is ~30K which is pretty small and probably worth it since it's only in the SDK.

Benchmarks at https://gist.github.com/anuraaga/1de9e2526a159b4e932d011c2dfb58e2 for throughput. I didn't check CPU overhead since I think that requires a profiler with our current setup, but there really shouldn't be any real change.

ArrayBlockingQueue caps off at around 3M ops/sec on my Macbook Pro 16, while the JCTools queue gets up to 8.5M. I also tried compound queue (shard queue into # of CPUs chunks) but didn't find a significant improvement so using the simpler one.

…nto jctools

jkwatson · 2021-03-19T04:14:00Z

I'll take a look and put it on the profiler tomorrow.

sbandadd · 2021-03-19T04:39:24Z

...e/src/jmh/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorMultiThreadBenchmark.java

@@ -42,7 +42,7 @@
    private long exportedSpans;
    private long droppedSpans;

-    @Setup(Level.Iteration)
+    @Setup(Level.Trial)


I think this will mess up with the exporter metrics..

Since when collecting metrics it clears the current values I think it might be ok. Either way, either this needs to be Trial or tearDown needs to be Iteration generally we'd only want to start one BSP per trial though I think.

could you please run this benchmark and verify the exportedSpans metric is valid, i.e; current values do get cleared after collecting them at the end of each iteration.

It does keep on going up now - I don't think we should need to restart the BSP for it though. @jkwatson Do you know a nice way to aggregate the metrics into a rate for reporting in the JMH benchmark?

I don't understand the question. Is something not working about the way things are before this change? Also, should we be doing a forceFlush() at the iteration teardown ?

And, honestly, I'm not sure I understand the purpose of this benchmark in the first place. I ran this on the main branch, and for the 20-thread case, we drop almost all of the spans. Is the goal to see if we can make the BSP drop fewer spans if we can improve things?

I think this change still needs to be reverted @anuraaga

Ok I went ahead and changed the shutdown to be per-iteration too then, need one of the changes to make sure threads are closed or JMH complains (at least to me). I don't think we actually wanted to initialize a whole BSP (worker thread, etc) per iteration though.

What complaints did you get from jmh?

Something about threads not all being shut down, waiting XXX seconds for them to shutdown. Which is definitely true since currently we don't call shutdown on every created BSP.

Ah yes. I had been wondering about that. Good find!

sbandadd · 2021-03-19T04:44:58Z

sdk/trace-shaded-deps/src/main/java/io/opentelemetry/sdk/trace/internal/JcTools.java

+   * implementation so callers do not need to use the shaded classes.
+   */
+  public static long capacity(Queue<?> queue) {
+    return ((MessagePassingQueue<?>) queue).capacity();


you can use MpscArrayQueue right? It does have capacity()

We're shading it and using this in a test in a different artifact where we don't want to have to reference the shaded class. I could cast to MpscArrayQueue here too but may as well stick with the iterface.

sbandadd · 2021-03-19T04:46:38Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessor.java

@@ -73,7 +75,7 @@ public static BatchSpanProcessorBuilder builder(SpanExporter spanExporter) {
            scheduleDelayNanos,
            maxExportBatchSize,
            exporterTimeoutNanos,
-            new ArrayBlockingQueue<>(maxQueueSize));
+            JcTools.newMpscArrayQueue(maxQueueSize));


Note that MpscArrayQueue rounds the queue size to power to 2 for various perf reasons. In my opinion it is better to enforce this so users know what the actual memory that is getting allocated.

Do you mean falling back to ArrayBlockingQueue if size isn't power of 2? I don't think we can require this for the BSP setting instead since it's too tricky to use.

I meant enforcing the maxQueueSize to be a power of 2.

That we can't do we don't want to lose usability (adding restrictions that can only be conveyed through documentation or error messages) here. Would like to hear more thoughts on whether we should fallback if it's not power-of-2

Falling back is not great really since it is not an efficient solution. How about calling it out in the documentation that queue size is rounded to the next power of 2?

I added a note to the builder that some more memory may be allocated, without going too much into implementation detail.

sbandadd · 2021-03-19T16:42:34Z

...figure/src/test/java/io/opentelemetry/sdk/autoconfigure/TracerProviderConfigurationTest.java

@@ -97,8 +98,7 @@ void configureSpanProcessor_empty() {
                assertThat(worker)
                    .extracting("queue")
                    .isInstanceOfSatisfying(
-                        ArrayBlockingQueue.class,
-                        queue -> assertThat(queue.remainingCapacity()).isEqualTo(2048));
+                        Queue.class, queue -> assertThat(JcTools.capacity(queue)).isEqualTo(2048));


The existing logic is verifying the remainingCapacity(). Do you want to do the same?

(JcTools.capacity(queue) - JcTools.size(queue)).isEqualTo(2048)

Nah, it only used tha tmethod since the JDK only provides that one, but capacity is what we're checking

sbandadd · 2021-03-19T16:59:36Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessor.java

@@ -17,8 +17,10 @@
 import io.opentelemetry.sdk.trace.ReadableSpan;
 import io.opentelemetry.sdk.trace.SpanProcessor;
 import io.opentelemetry.sdk.trace.data.SpanData;
+import io.opentelemetry.sdk.trace.internal.JcTools;


Now this comment https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessor.java#L40 is not relevant any more !

…nto jctools

anuraaga

@jkwatson Can you take another look at this? Thanks!

anuraaga · 2021-03-31T01:33:50Z

...figure/src/test/java/io/opentelemetry/sdk/autoconfigure/TracerProviderConfigurationTest.java

@@ -97,8 +98,7 @@ void configureSpanProcessor_empty() {
                assertThat(worker)
                    .extracting("queue")
                    .isInstanceOfSatisfying(
-                        ArrayBlockingQueue.class,
-                        queue -> assertThat(queue.remainingCapacity()).isEqualTo(2048));
+                        Queue.class, queue -> assertThat(JcTools.capacity(queue)).isEqualTo(2048));


Nah, it only used tha tmethod since the JDK only provides that one, but capacity is what we're checking

anuraaga · 2021-03-31T01:36:21Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessor.java

@@ -73,7 +75,7 @@ public static BatchSpanProcessorBuilder builder(SpanExporter spanExporter) {
            scheduleDelayNanos,
            maxExportBatchSize,
            exporterTimeoutNanos,
-            new ArrayBlockingQueue<>(maxQueueSize));
+            JcTools.newMpscArrayQueue(maxQueueSize));


I added a note to the builder that some more memory may be allocated, without going too much into implementation detail.

jkwatson

Let's try it.

sbandadd

Looks good!

Anuraag Agrawal added 5 commits March 17, 2021 14:38

Replace ArrayBlockingQueue with jctools queue.

0357414

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java i…

7645a66

…nto jctools

Finish

c545b2f

ArrayQueue

3552fbe

Fix dependency

73aac08

anuraaga marked this pull request as ready for review March 19, 2021 03:56

anuraaga requested review from arminru, bogdandrutu, carlosalberto, jkwatson, Oberon00, pavolloffay, thisthat and tylerbenson as code owners March 19, 2021 03:56

Drift

8573ef7

sbandadd reviewed Mar 19, 2021

View reviewed changes

Anuraag Agrawal added 2 commits March 31, 2021 10:33

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java i…

57e5210

…nto jctools

Memory note

1f22b51

anuraaga commented Mar 31, 2021

View reviewed changes

Iteration

029390f

jkwatson approved these changes Mar 31, 2021

View reviewed changes

sbandadd approved these changes Mar 31, 2021

View reviewed changes

anuraaga merged commit 2f2af19 into open-telemetry:main Mar 31, 2021

This was referenced Dec 19, 2021

Temurin JDK #4011

Merged

use Eclipse Temurin JDK docker image #4012

Merged

mohitmahi mentioned this pull request Jul 3, 2022

Added a static method "drain" under JcTools with a generic consumer #4582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ArrayBlockingQueue with jctools queue. #3034

Replace ArrayBlockingQueue with jctools queue. #3034

anuraaga commented Mar 17, 2021 •

edited

Loading

jkwatson commented Mar 19, 2021

sbandadd Mar 19, 2021

anuraaga Mar 19, 2021

sbandadd Mar 19, 2021

anuraaga Mar 19, 2021

jkwatson Mar 19, 2021

jkwatson Mar 31, 2021

anuraaga Mar 31, 2021

jkwatson Mar 31, 2021

anuraaga Mar 31, 2021

jkwatson Mar 31, 2021

sbandadd Mar 19, 2021

anuraaga Mar 19, 2021

sbandadd Mar 19, 2021

anuraaga Mar 19, 2021

sbandadd Mar 19, 2021

anuraaga Mar 19, 2021

sbandadd Mar 19, 2021

anuraaga Mar 31, 2021

sbandadd Mar 19, 2021

anuraaga Mar 31, 2021

sbandadd Mar 19, 2021

anuraaga left a comment

anuraaga Mar 31, 2021

anuraaga Mar 31, 2021

jkwatson left a comment

sbandadd left a comment

Replace ArrayBlockingQueue with jctools queue. #3034

Replace ArrayBlockingQueue with jctools queue. #3034

Conversation

anuraaga commented Mar 17, 2021 • edited Loading

jkwatson commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson left a comment

Choose a reason for hiding this comment

sbandadd left a comment

Choose a reason for hiding this comment

anuraaga commented Mar 17, 2021 •

edited

Loading