openzipkin / brave

Java distributed tracing implementation compatible with Zipkin backend services.
Apache License 2.0
2.36k stars 713 forks source link

Adds BaggagePropagation benchmarks for decorate #1425

Closed codefromthecrypt closed 8 months ago

codefromthecrypt commented 8 months ago

I tried a couple ways to optimize ExtraFactory with regards to supplying the extraList directly, including some special casing of singleton list and using InternalPropagation.instance.withExtra(context, unmodifiableList(newExtra)); on a pre-sized list. The results of routine case always ended up with more allocation than now.

This adds a benchmark, so people don't think like I did and feel there is a quick change that won't hurt others. It is possible that the bench isn't representative, or more angles need to be looked at, but this should help people understand where to start, and know if something becomes worse as a result!

See #1421

Benchmark                                                               Mode     Cnt     Score     Error   Units
BaggagePropagationBenchmarks.decorate:gc.alloc.rate.norm              sample      15   160.003 ±   0.001    B/op
BaggagePropagationBenchmarks.decorate:p0.99                           sample             0.042             us/op
BaggagePropagationBenchmarks.decorate_withBaggage:gc.alloc.rate.norm  sample      15    80.002 ±   0.001    B/op
BaggagePropagationBenchmarks.decorate_withBaggage:p0.99               sample             0.042             us/op
codefromthecrypt commented 8 months ago

fwiw this change looks neat, but made allocations worse. this is why we use benchmarks!

     // If context.extra() didn't have an unclaimed extra instance, create one for this context.
+    boolean createdExtra = false;
     if (claimed == null) {
+      createdExtra = true;
       claimed = create();
       if (claimed == null) {
         Platform.get().log("BUG: create() returned null", null);
@@ -160,7 +172,14 @@ public abstract class ExtraFactory<E extends Extra<E, F>, F extends ExtraFactory
       claimed.tryToClaim(traceId, spanId);
     }

-    TraceContext.Builder builder = context.toBuilder().clearExtra().addExtra(claimed);
+    // Handle common case to avoid allocating an array
+    if (extraLength == 0) {
+      return InternalPropagation.instance.withExtra(context, singletonList(claimed));
+    }
+
+    // Pre-size the list to avoid allocations via context.clearExtra().addExtra()..
+    List<Object> newExtra = new ArrayList<>(createdExtra ? extraLength + 1 : extraLength);
+    newExtra.add(claimed);

     for (int i = 0; i < extraLength; i++) {
       Object next = context.extra().get(i);
@@ -173,10 +192,10 @@ public abstract class ExtraFactory<E extends Extra<E, F>, F extends ExtraFactory
           claimed.mergeStateKeepingOursOnConflict(existing);
         }
       } else if (!next.equals(claimed)) {
-        builder.addExtra(next);
+        newExtra.add(next);
       }
     }

-    return builder.build();
+    return InternalPropagation.instance.withExtra(context, unmodifiableList(newExtra));