Optimizations for ObjectExtensions by kevingosse · Pull Request #759 · DataDog/dd-trace-dotnet

kevingosse · 2020-06-18T13:00:27Z

Make MemberResult a struct
Add a custom key type for the cache to avoid building a string every time
Use the custom key to prevent closure allocation in the ConcurrentDictionary.GetOrAdd callbacks
Make the PropertyFetcher generic to prevent boxing when fetching a value type
Add a Fetch overload to provide the type when it's already known

Benchmark:

return ObjectsExtensions.Emit.ObjectExtensions.GetProperty<int>("source", "Length").Value;

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.329 (2004/?/20H1)
Intel Core i7-9750H CPU 2.60GHz, 1 CPU, 12 logical and 6 physical cores
  [Host]     : .NET Framework 4.8 (4.8.4180.0), X64 RyuJIT
  DefaultJob : .NET Framework 4.8 (4.8.4180.0), X64 RyuJIT

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
OldGetProperty	445.22 ns	3.179 ns	2.818 ns	1.00	0.1721	-	-	1083 B
NewGetProperty	84.56 ns	0.821 ns	0.768 ns	0.19	-	-	-	-

kevingosse · 2020-06-18T13:02:21Z

            var paramType1 = typeof(TArg1);

            object cachedItem = Cache.GetOrAdd(
-                $"{type.AssemblyQualifiedName}.{methodName}.{paramType1.AssemblyQualifiedName}",


I'm assuming that the comparer for System.Type has the same granularity as the assembly qualified name. It would make sense, but I need to double-check

The AssemblyQualified type was also useful because we want the owning Assembly as well. This is something that we can run into with the application using both StackExchange.Redis.StrongName and StackExchange.Redis NuGet packages

I assumed as much yes. That's why I need to make absolutely sure that the Type equality comparer takes that point into account.

Looking good. I created a library and compiled it twice: once in debug without signature, once in release with a strong name (the debug/release distinction is probably irrelevant)

Then ran the following code:

var debug = Assembly.LoadFrom(@"C:\Users\kevin.gosse\source\repos\ConsoleApp1\Library1\bin\Debug\Library1.dll"); var release = Assembly.LoadFrom(@"C:\Users\kevin.gosse\source\repos\ConsoleApp1\Library1\bin\Release\Library1.dll"); var typeDebug = debug.GetType("Library1.Class1"); var typeRelease = release.GetType("Library1.Class1"); Console.WriteLine(debug.FullName); Console.WriteLine(release.FullName); Console.WriteLine($"{typeDebug.FullName} - {typeDebug.AssemblyQualifiedName}"); Console.WriteLine($"{typeRelease.FullName} - {typeRelease.AssemblyQualifiedName}"); Console.WriteLine(typeDebug.Equals(typeRelease));

The output is:

Library1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null Library1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=c28eafe45d72d6c9 Library1.Class1 - Library1.Class1, Library1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null Library1.Class1 - Library1.Class1, Library1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=c28eafe45d72d6c9 False

So it looks like the type equality comparer does the necessary checks.

kevingosse · 2020-06-18T13:03:36Z

            return dynamicMethod.CreateDelegate();
        }
+
+        private readonly struct PropertyFetcherCacheKey : IEquatable<PropertyFetcherCacheKey>


I'm using the same key type for methods and properties. As far as I know, a type can't have a property and a method with the same name, so I don't believe collisions are possible

kevingosse · 2020-06-18T13:04:11Z

+                Name = name;
+            }
+
+            public bool Equals(PropertyFetcherCacheKey other)


Equality members auto-generated by resharper

kevingosse · 2020-06-18T13:05:57Z

+        /// <returns>The value of the property on the specified object.</returns>
+        public T Fetch<T>(object obj, Type objType)
+        {
            if (objType != _expectedType)


I don't really understand why we do that. If we call Fetch with a different type, shouldn't we throw?
If it's done just for initialization, shouldn't we do that in the constructor?

I'm not entirely sure why we did it this way where we reconstruct the fetcher if the type of the object is different (other than the fact that we ported this directly from dotnet/runtime). It seems like the only upside is that this allows us to resolve properties against multiple implementation types of the same abstract type, but I think we could accomplish this by getting the type of the generic parameter T and it would work for all method arguments.

@lucaspimentel do you have any more insight on this?

This file was copied verbatim from the dotnet repo (see url at top of file). Unfortunately, that link is to master and the file has since changed. We also made this one change which I think was to allow getting inherited properties (not declared in the given type).

My guess is that DiagnosticSource needed to handle changing types because they rely heavily on anonymous types. For example, a fetcher for a property called Name could be called once with

new { Name = "Foo" }

and then later with

new { Name = "Foo", Value = 1 }

... and each time the type changed, they generate a new delegate. I don't think we need to support this "feature" for our use cases.

For reference, I think the version we used was from .NET Core 2.1: source

kevingosse · 2020-06-18T13:06:41Z

        private readonly string _propertyName;
        private Type _expectedType;
-        private PropertyFetch _fetchForExpectedType;
+        private object _fetchForExpectedType;


It bothers me a bit. I'll probably come back to it at some point to try and remove the cast

zacharycmontoya · 2020-06-23T20:43:18Z

            object cachedItem = Cache.GetOrAdd(
                GetKey<TResult>(fieldName, type),
-                key => CreateFieldDelegate<TResult>(type, fieldName));
+                key => CreateFieldDelegate<TResult>(key.Type1, key.Name));


kevingosse · 2020-06-24T11:21:49Z

Pushed a new version that uses expressions to fetch the property value.

I thought I was being clever with the where TProperty : T generic constraint in the property fetcher, but I didn't consider that there isn't necessarily an inheritance relationship between TProperty and T. For instance, in the HttpMessageHandler integration, we fetch a HttpStatusCode (of type Enum) as an int. Thankfully, this was caught in the integration tests.

With the latest commit:

I now generate an expression to fetch the property value. It's probably slower the first time, but it's actually slightly faster when invoked. I can't use generics because the compiler won't accept to cast a TProperty into T unless it can demonstrate it's safe. Expressions have more relaxed constraints.
With that change, I saw no value in keeping the TypedFetchProperty implementation, so I collapsed it into the parent PropertyFetch
I added a unit test for the "enum to int" case

New benchmark:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.329 (2004/?/20H1)
Intel Core i7-9750H CPU 2.60GHz, 1 CPU, 12 logical and 6 physical cores
  [Host]     : .NET Framework 4.8 (4.8.4180.0), X64 RyuJIT
  DefaultJob : .NET Framework 4.8 (4.8.4180.0), X64 RyuJIT

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
OldGetProperty	441.11 ns	2.361 ns	2.093 ns	1.00	0.1721	-	-	1083 B
NewGetProperty	83.61 ns	0.311 ns	0.259 ns	0.19	-	-	-	-
ExpressionGetProperty	82.77 ns	0.297 ns	0.232 ns	0.19	-	-	-	-
CollapsedGetProperty	76.10 ns	0.334 ns	0.296 ns	0.17	-	-	-	-

NewGetProperty => Previous version of the code (that didn't work with the enum -> int conversion)
ExpressionGetProperty => Version that uses expressions in the TypedFetchProperty
CollapsedGetProperty => Version that collapses the TypedFetchProperty into the PropertyFetch (the one I commited)

- Make MemberResult a struct - Add a custom key type for the cache to avoid building a string every time - Use the custom key to prevent closure allocation in the Concurrent.GetOrAdd callbacks - Make the PropertyFetcher generic to prevent boxing when fetching a value type - Add a Fetch overload to provide the type when it's already known

- Messed up the orders of the parameters when creating the key for fields - Didn't consider that somebody could ask for a value of a type different from the property (for instance, GetProperty<object> on an int property)

Enforce that Type1 and Name are not null, and remove the null-check from GetHashCode

zacharycmontoya

LGTM in its current state. I'm not sure if you had planned to make any additional changes

kevingosse requested review from lucaspimentel and zacharycmontoya June 18, 2020 13:00

kevingosse requested a review from a team as a code owner June 18, 2020 13:00

kevingosse commented Jun 18, 2020

View reviewed changes

lucaspimentel added the type:performance Performance, speed, latency, resource usage (CPU, memory) label Jun 18, 2020

lucaspimentel reviewed Jun 18, 2020

View reviewed changes

Comment thread src/Datadog.Trace.ClrProfiler.Managed/Emit/ObjectExtensions.cs Outdated

lucaspimentel reviewed Jun 18, 2020

View reviewed changes

Comment thread src/Datadog.Trace/DiagnosticListeners/AspNetCoreDiagnosticObserver.cs

kevingosse force-pushed the kevin/memberresult branch from b86e439 to ab76ce3 Compare June 23, 2020 16:20

kevingosse requested a review from lucaspimentel June 23, 2020 16:22

zacharycmontoya reviewed Jun 23, 2020

View reviewed changes

kevingosse added 5 commits June 24, 2020 18:28

Fix issues

8007bda

- Messed up the orders of the parameters when creating the key for fields - Didn't consider that somebody could ask for a value of a type different from the property (for instance, GetProperty<object> on an int property)

Address PR feedback

ee987af

Enforce that Type1 and Name are not null, and remove the null-check from GetHashCode

Update for the new TryCallVoidMethod method

d36d952

Use expressions to fetch the property value

4f8dbe6

kevingosse force-pushed the kevin/memberresult branch from cb0d8c1 to 4f8dbe6 Compare June 24, 2020 16:28

zacharycmontoya approved these changes Jun 25, 2020

View reviewed changes

kevingosse merged commit a68e324 into master Jun 26, 2020

kevingosse deleted the kevin/memberresult branch June 26, 2020 14:34

zacharycmontoya added this to the 1.18.2 milestone Jul 9, 2020

kevingosse mentioned this pull request Aug 3, 2020

Optimize HttpMessageHandlerIntegration #808

Merged

Uh oh!

Conversation

kevingosse commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevingosse commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zacharycmontoya left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevingosse commented Jun 18, 2020 •

edited

Loading

kevingosse commented Jun 24, 2020 •

edited

Loading