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

System.Diagnostics.Activity Perf Improvement Part 2 #40544

Merged
merged 7 commits into from
Aug 13, 2020
Merged

System.Diagnostics.Activity Perf Improvement Part 2 #40544

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d11af05
Use DynamicDependency to prevent struct GetEnumerator from being remo…
CodeBlanch Aug 7, 2020
526817d
Code review.
CodeBlanch Aug 7, 2020
0af8fa9
Removed empty line.
CodeBlanch Aug 7, 2020
5d0262c
Code review.
CodeBlanch Aug 7, 2020
dd4b901
Update comment style.
CodeBlanch Aug 7, 2020
b9a4865
Code review.
CodeBlanch Aug 7, 2020
0fdba3f
Drop support for reset on struct Enumerator.
CodeBlanch Aug 8, 2020
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
15 changes: 11 additions & 4 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 @@ -76,10 +76,20 @@ public partial class Activity : IDisposable

private byte _w3CIdFlags;

/*
* Note:
* DynamicDependency is used here to prevent the linker from removing the struct GetEnumerator on the internal types.
Copy link
Member

Choose a reason for hiding this comment

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

Looking more at this code, you could also change the Enumerator implementations like the following:

            public Enumerator<T> GetEnumerator() => new Enumerator<T>(_first);
            IEnumerator<T> IEnumerable<T>.GetEnumerator() => GetEnumerator();
            IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

And the public Enumerator<T> GetEnumerator() method would be preserved, since now it is being called. This would be more preferrable, since if we were linking an application, preserving ALL public methods on these types is more than necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I updated the code. Should have just done that initially, would have saved myself a lot of time 🤣 Learned a bit about the linker though so not totally wasted 👍

* Some customers use this GetEnumerator dynamically to avoid allocations.
* See: https://github.com/dotnet/runtime/pull/40362
*/
[DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(TagsLinkedList))]
private TagsLinkedList? _tags;
private LinkedList<KeyValuePair<string, string?>>? _baggage;
[DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(LinkedList<ActivityLink>))]
private LinkedList<ActivityLink>? _links;
[DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(LinkedList<ActivityEvent>))]
private LinkedList<ActivityEvent>? _events;

private LinkedList<KeyValuePair<string, string?>>? _baggage;
private ConcurrentDictionary<string, object>? _customProperties;
private string? _displayName;

Expand Down Expand Up @@ -255,9 +265,6 @@ public string? RootId
/// </summary>
public IEnumerable<KeyValuePair<string, object?>> TagObjects
{
#if ALLOW_PARTIALLY_TRUSTED_CALLERS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tarekgh I removed the SecuritySafeCritical. Assuming this is no longer needed because we removed the Unsafe.As on the last PR?

Copy link
Member

Choose a reason for hiding this comment

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

make sense. If you run the tests locally should catch any issue with the security attributes when running against NetFX.

[System.Security.SecuritySafeCriticalAttribute]
#endif
get => _tags ?? s_emptyTagObjects;
}

Expand Down