Adds BaggagePropagation benchmarks for decorate#1425
Merged
codefromthecrypt merged 1 commit intomasterfrom Mar 13, 2024
Merged
Conversation
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>
anuraaga
approved these changes
Mar 13, 2024
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)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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