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.

Sign up for GitHub

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

Regression and Perf Fixes #52956

Merged
merged 1 commit into from
May 19, 2021
Merged

Regression and Perf Fixes #52956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,13 +1058,6 @@ internal static Activity Create(ActivitySource source, string name, ActivityKind
}
}

activity.IsAllDataRequested = request == ActivitySamplingResult.AllData || request == ActivitySamplingResult.AllDataAndRecorded;

if (request == ActivitySamplingResult.AllDataAndRecorded)
{
activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded;
}

if (parentId != null)
{
activity._parentId = parentId;
Expand All @@ -1083,6 +1076,13 @@ internal static Activity Create(ActivitySource source, string name, ActivityKind
activity._traceState = parentContext.TraceState;
}

activity.IsAllDataRequested = request == ActivitySamplingResult.AllData || request == ActivitySamplingResult.AllDataAndRecorded;

if (request == ActivitySamplingResult.AllDataAndRecorded)
{
activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded;
}

if (startTime != default)
{
activity.StartTimeUtc = startTime.UtcDateTime;
Expand Down
30 changes: 15 additions & 15 deletions ...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MeterListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,35 +196,35 @@ internal static void NotifyForPublishedInstrument(Instrument instrument)
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal void NotifyMeasurement<T>(Instrument instrument, T measurement, ReadOnlySpan<KeyValuePair<string, object?>> tags, object? state)
internal void NotifyMeasurement<T>(Instrument instrument, T measurement, ReadOnlySpan<KeyValuePair<string, object?>> tags, object? state) where T : struct
{
if (measurement is byte byteMeasurement)
if (typeof(T) == typeof(byte))
Copy link
Contributor

Choose a reason for hiding this comment

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

@tarekgh Hey I was just noticing this change. Would you indulge me on a couple of questions?

  • Why remove the else if from the checks?

  • I understand the switch from is to typeof(T) is because some frameworks allocate on is. If we added back the old version under a preprocessor directive for >= NET5.0 would it then be allocation free for users on NET5.0 or NET6.0? Right now those users will see at least one boxing from the object cast, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why remove the else if from the checks?

It doesn't matter as the JIT optimize the code anyway and will remove all other code when generating type specific code.

I understand the switch from is to typeof(T) is because some frameworks allocate on is. If we added back the old version under a preprocessor directive for >= NET5.0 would it then be allocation free for users on NET5.0 or NET6.0? Right now those users will see at least one boxing from the object cast, no?

No, boxing will be optimized away too. no boxing will occur. try it yourself writing some simple code like:

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        internal static int NotifyMeasurement<T>(T measurement) where T : struct
        {
            if (typeof(T) == typeof(int))
            {
                return (int)(object)measurement;
            }
            if (typeof(T) == typeof(long))
            {
                return (int)(long)(object)measurement;
            }
            return 0;
        }

        [Benchmark]
        public void TestAllocation()
        {
            int t = 0;
            for (int i = 0; i < 10; i++)
            {
                t += NotifyMeasurement<int>(i);
                t += NotifyMeasurement<long>(i);
            }
        }

and you will see no allocation happening

|         Method |     Mean |     Error |    StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------- |---------:|----------:|----------:|------:|------:|------:|----------:|
| TestAllocation | 6.096 ns | 0.0659 ns | 0.0584 ns |     - |     - |     - |         - |

Copy link
Contributor

Choose a reason for hiding this comment

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

Good stuff thanks @tarekgh! Is this JIT magic documented anywhere? A cheat sheet of things it will optimize away would be great to have.

{
_byteMeasurementCallback(instrument, byteMeasurement, tags, state);
_byteMeasurementCallback(instrument, (byte)(object)measurement, tags, state);
}
else if (measurement is short shortMeasurement)
if (typeof(T) == typeof(short))
{
_shortMeasurementCallback(instrument, shortMeasurement, tags, state);
_shortMeasurementCallback(instrument, (short)(object)measurement, tags, state);
}
else if (measurement is int intMeasurement)
if (typeof(T) == typeof(int))
{
_intMeasurementCallback(instrument, intMeasurement, tags, state);
_intMeasurementCallback(instrument, (int)(object)measurement, tags, state);
}
else if (measurement is long longMeasurement)
if (typeof(T) == typeof(long))
{
_longMeasurementCallback(instrument, longMeasurement, tags, state);
_longMeasurementCallback(instrument, (long)(object)measurement, tags, state);
}
else if (measurement is float floatMeasurement)
if (typeof(T) == typeof(float))
{
_floatMeasurementCallback(instrument, floatMeasurement, tags, state);
_floatMeasurementCallback(instrument, (float)(object)measurement, tags, state);
}
else if (measurement is double doubleMeasurement)
if (typeof(T) == typeof(double))
{
_doubleMeasurementCallback(instrument, doubleMeasurement, tags, state);
_doubleMeasurementCallback(instrument, (double)(object)measurement, tags, state);
}
else if (measurement is decimal decimalMeasurement)
if (typeof(T) == typeof(decimal))
{
_decimalMeasurementCallback(instrument, decimalMeasurement, tags, state);
_decimalMeasurementCallback(instrument, (decimal)(object)measurement, tags, state);
}
}
}
Expand Down
33 changes: 32 additions & 1 deletion src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,37 @@ public void TestListeningToConstructedActivityEvents()
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void EnsureRecordingTest()
{
RemoteExecutor.Invoke(() => {
Activity.ForceDefaultIdFormat = true;
Activity.DefaultIdFormat = ActivityIdFormat.W3C;

ActivitySource aSource = new ActivitySource("EnsureRecordingTest");

ActivityListener listener = new ActivityListener
{
ShouldListenTo = (activitySource) => true,
Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) =>
{
// Access activityOptions.TraceId to ensure generating the non-default value.
ActivityTraceId traceId = activityOptions.TraceId;
Assert.NotEqual(default(ActivityTraceId), traceId);
return ActivitySamplingResult.AllDataAndRecorded;
}
};

ActivitySource.AddActivityListener(listener);

Activity a = aSource.StartActivity("RecordedActivity");
Assert.NotNull(a);

Assert.True(a.Recorded);
Assert.True((a.Context.TraceFlags & ActivityTraceFlags.Recorded) != 0);
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void TestExpectedListenersReturnValues()
{
Expand Down Expand Up @@ -947,7 +978,7 @@ public void TestActivityCreate()
Assert.Null(a3.Parent);
Assert.Equal("ParentId", a3.ParentId);

ActivityContext parentContext = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.None);
ActivityContext parentContext = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.Recorded);
using (Activity a4 = aSource.CreateActivity("a4", ActivityKind.Internal, parentContext, tags, links))
{
Assert.NotNull(a4);
Expand Down