-
Notifications
You must be signed in to change notification settings - Fork 5.1k
converting eventsource metadata list to a dict #117031
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.
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
Conversation
Tagging subscribers to this area: @tarekgh, @tommcdon, @steveisok, @pjanotti |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the fixed-size EventMetadata[]
storage with a Dictionary<int, EventMetadata>
to allow sparse and dynamic event ID mappings.
- Swaps all
m_eventData
array uses toDictionary<int, EventMetadata>
and updates access patterns to useCollectionsMarshal
. - Converts loops over array indices into dictionary key/value iterations and adapts initializers.
- Adds
using System.Runtime.InteropServices
forCollectionsMarshal
APIs.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.cs | Switched m_eventData to a dictionary, updated bounds check and payload decoding to use CollectionsMarshal . |
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs | Replaced array-based loops and initializers with dictionary equivalents; updated all indexing to use GetValueRefOrNullRef / GetValueRefOrAddDefault . |
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
…acing/EventSource.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…acing/EventSource.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…acing/EventSource.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Blocked by #117039 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from a little bit of formatting stuff and a couple unneeded null checks.
Perf-wise I'd be a bit surprised if this change didn't incur at least a few % extra CPU cost in WriteEvent for a scenario with an active EventListener but I think it would be an OK tradeoff for the sparse event id memory benefits.
Closes #99816
This pull request implements the request described in #99816 of reducing the memory usage of EventSource in the case of high EventIDs by using a dictionary of EventMetadata instead of an array. This change resulted in no visible slowdown of WriteEvent, and memory usage was brought down to consistent levels across high and low EventIDs that matches the preexisting memory usage levels in the case of low EventIDs.