Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds BaggagePropagation benchmarks for decorate #1425

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

codefromthecrypt
Copy link
Member

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

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.

```
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
```

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Member Author

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));

@codefromthecrypt codefromthecrypt merged commit 69e30f3 into master Mar 13, 2024
3 checks passed
@codefromthecrypt codefromthecrypt deleted the benchmark-decorate branch March 13, 2024 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants