Skip to content

MessagePack Formatter: Cache Span tag keys in UTF8#1482

Merged
tonyredondo merged 1 commit into
masterfrom
tony/messagepack-improvements
May 24, 2021
Merged

MessagePack Formatter: Cache Span tag keys in UTF8#1482
tonyredondo merged 1 commit into
masterfrom
tony/messagepack-improvements

Conversation

@tonyredondo

@tonyredondo tonyredondo commented May 23, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Old:

// * Summary *

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Xeon CPU E5-2673 v3 2.40GHz, 1 CPU, 4 logical and 2 physical cores
.NET Core SDK=5.0.201
[Host] : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT
Job-WEIHTL : .NET Framework 4.8 (4.8.4341.0), X64 RyuJIT
Job-VTKECX : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT

IterationTime=1.5000 s

Method Job Runtime Toolchain Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
WriteAndFlushTraces Job-WEIHTL .NET 4.7.2 net472 748.0 us 10.84 us 9.61 us 1.00 0.00 - - - 3.03 KB
WriteAndFlushTraces Job-VTKECX .NET Core 3.1 netcoreapp3.1 604.5 us 11.94 us 12.78 us 0.81 0.02 - - - 2.45 KB
WriteAndFlushEnrichedTraces Job-WEIHTL .NET 4.7.2 net472 937.3 us 13.01 us 11.54 us 1.00 0.00 - - - 3.04 KB
WriteAndFlushEnrichedTraces Job-VTKECX .NET Core 3.1 netcoreapp3.1 743.7 us 14.43 us 16.04 us 0.79 0.02 - - - 2.45 KB

New:

// * Summary *

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Xeon CPU E5-2673 v3 2.40GHz, 1 CPU, 4 logical and 2 physical cores
.NET Core SDK=5.0.201
[Host] : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT
Job-GAZNKL : .NET Framework 4.8 (4.8.4341.0), X64 RyuJIT
Job-NQGFHJ : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT

IterationTime=1.5000 s

Method Job Runtime Toolchain Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
WriteAndFlushTraces Job-GAZNKL .NET 4.7.2 net472 604.9 us 6.84 us 6.06 us 1.00 0.00 - - - 3.03 KB
WriteAndFlushTraces Job-NQGFHJ .NET Core 3.1 netcoreapp3.1 467.7 us 6.88 us 5.74 us 0.77 0.01 - - - 2.45 KB
WriteAndFlushEnrichedTraces Job-GAZNKL .NET 4.7.2 net472 819.3 us 15.33 us 14.34 us 1.00 0.00 - - - 3.03 KB
WriteAndFlushEnrichedTraces Job-NQGFHJ .NET Core 3.1 netcoreapp3.1 624.5 us 7.92 us 6.18 us 0.76 0.02 - - - 2.45 KB

Diff:

  1. WriteAndFlushTraces/net472: -143.1 us
  2. WriteAndFlushTraces/netcoreapp3.1: -136,8 us
  3. WriteAndFlushEnrichedTraces/net472: -118 us
  4. WriteAndFlushEnrichedTraces/netcoreapp3.1: -119,2 us

image

@DataDog/apm-dotnet

@tonyredondo tonyredondo self-assigned this May 23, 2021
@tonyredondo tonyredondo changed the title Cache Span tag keys in UTF8 MessagePack Formatter: Cache Span tag keys in UTF8 May 23, 2021
@tonyredondo
tonyredondo marked this pull request as ready for review May 23, 2021 20:02
@tonyredondo
tonyredondo requested a review from a team as a code owner May 23, 2021 20:02
@tonyredondo
tonyredondo force-pushed the tony/messagepack-improvements branch from 78e3c99 to 2871d9d Compare May 24, 2021 08:39
andrewlock
andrewlock approved these changes May 24, 2021
View reviewed changes

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍 I'm kind of surprised there's no change in the allocations though, I guess MessagePackBinary.WriteString is caching the byte[] behind the scene?

@tonyredondo

tonyredondo commented May 24, 2021

Copy link
Copy Markdown
Member Author

Nice 👍 I'm kind of surprised there's no change in the allocations though, I guess MessagePackBinary.WriteString is caching the byte[] behind the scene?

No, is not caching, is writing directly to the actual buffer... https://github.com/DataDog/dd-trace-dotnet/blob/master/src/Datadog.Trace/Vendors/MessagePack/MessagePackBinary.cs#L1715

Also, I do think there is a change on allocations, can be seen in WriteAndFlushEnrichedTraces/net472 from 3.04 KB to 3.03 KB. But is a little change compared to all allocations.

@tonyredondo
tonyredondo requested a review from a team May 24, 2021 12:10
@tonyredondo
tonyredondo merged commit e19c7f4 into master May 24, 2021
@tonyredondo
tonyredondo deleted the tony/messagepack-improvements branch May 24, 2021 14:08
lucaspimentel
lucaspimentel reviewed May 25, 2021
View reviewed changes
Comment thread src/Datadog.Trace/Agent/MessagePack/SpanMessagePackFormatter.cs
Comment on lines +10 to +19
private static byte[] _traceIdBytes = StringEncoding.UTF8.GetBytes("trace_id");
private static byte[] _spanIdBytes = StringEncoding.UTF8.GetBytes("span_id");
private static byte[] _nameBytes = StringEncoding.UTF8.GetBytes("name");
private static byte[] _resourceBytes = StringEncoding.UTF8.GetBytes("resource");
private static byte[] _serviceBytes = StringEncoding.UTF8.GetBytes("service");
private static byte[] _typeBytes = StringEncoding.UTF8.GetBytes("type");
private static byte[] _startBytes = StringEncoding.UTF8.GetBytes("start");
private static byte[] _durationBytes = StringEncoding.UTF8.GetBytes("duration");
private static byte[] _parentIdBytes = StringEncoding.UTF8.GetBytes("parent_id");
private static byte[] _errorBytes = StringEncoding.UTF8.GetBytes("error");

@lucaspimentel lucaspimentel May 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make these not static? Did we benchmark in .NET Framework loading Datadog.Trace.dll domain-neutral?

Access to static data and methods is slower for domain-neutral assemblies because of the need to isolate assemblies. Each application domain that accesses the assembly must have a separate copy of the static data, to prevent references to objects in static fields from crossing domain boundaries. As a result, the runtime contains additional logic to direct a caller to the appropriate copy of the static data or method. This extra logic slows down the call.

source: https://docs.microsoft.com/en-us/dotnet/framework/app-domains/application-domains

@tonyredondo tonyredondo May 28, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, as we talked, I'll run some benchmarks for curiosity about the performance impact on domain neutral assemblies. But it doesn't make any harm to change it to instance fields in the SpanMessagePackFormatter file if we double check we use a single instance. I will do a PR for that.

@tonyredondo tonyredondo mentioned this pull request May 31, 2021
Merged
@pellared pellared mentioned this pull request May 31, 2021
Merged
@andrewlock andrewlock added this to the 1.27.0 milestone Jun 1, 2021
@andrewlock andrewlock added area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) type:performance Performance, speed, latency, resource usage (CPU, memory) labels Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lucaspimentel lucaspimentel lucaspimentel left review comments
@colin-higgins colin-higgins colin-higgins approved these changes
@andrewlock andrewlock andrewlock approved these changes

Assignees

@tonyredondo tonyredondo

Labels

area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) type:performance Performance, speed, latency, resource usage (CPU, memory)

Projects

None yet

Milestone

1.27.0

Development

Successfully merging this pull request may close these issues.

4 participants

@tonyredondo @colin-higgins @lucaspimentel @andrewlock