feat: Adding in Performance Tick Data that stores data for external consumption (RFC #7) #491
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| using System.Collections.Generic; | ||
|
|
||
| namespace MLAPI.Profiling | ||
| { | ||
| public interface ITransportProfilerData | ||
| { | ||
| void BeginNewTick(); | ||
| IReadOnlyDictionary<string, int> GetTransportProfilerData(); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
|
|
||
| namespace MLAPI.Profiling | ||
| { | ||
| static class PerformanceDataManager | ||
| { | ||
| static PerformanceTickData s_ProfilerData; | ||
| static int s_TickID; | ||
|
|
||
| internal static void BeginNewTick() | ||
| { | ||
| s_TickID = Math.Max(s_TickID, 0); | ||
| s_ProfilerData = new PerformanceTickData | ||
| { | ||
| tickID = s_TickID++, | ||
|
There was a problem hiding this comment. I know the odds are very low, but there's a risk of overflowing. We should at least use an unsigned. Probably unsigned long? No sense in having a negative tick id. |
||
| }; | ||
| } | ||
|
|
||
| internal static void Increment(string fieldName, int count = 1) | ||
| { | ||
| s_ProfilerData?.Increment(fieldName, count); | ||
| } | ||
|
|
||
| internal static void AddTransportData(IReadOnlyDictionary<string, int> transportProfilerData) | ||
| { | ||
| s_ProfilerData?.AddNonDuplicateData(transportProfilerData); | ||
| } | ||
|
|
||
| internal static PerformanceTickData GetData() | ||
| { | ||
| return s_ProfilerData; | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
|
|
||
| namespace MLAPI.Profiling | ||
| { | ||
| public class PerformanceTickData | ||
| { | ||
| public int tickID; | ||
|
|
||
| readonly ProfilingDataStore m_TickData = new ProfilingDataStore(); | ||
|
|
||
| public void Increment(string fieldName, int count = 1) | ||
| { | ||
| m_TickData.Increment(fieldName, count); | ||
| } | ||
|
|
||
| public void AddNonDuplicateData(IReadOnlyDictionary<string, int> transportProfilerData) | ||
| { | ||
| IEnumerable<KeyValuePair<string, int>> nonDuplicates = transportProfilerData.Where(entry => !m_TickData.HasData(entry.Key)); | ||
|
There was a problem hiding this comment. this linq query is unnecessary here. you could do the HasData check inside the loop and avoid the linq cost There was a problem hiding this comment. https://stackoverflow.com/questions/4044400/linq-performance-faq The allocations and performance AFAIK are the same. There was a problem hiding this comment. Almost all Linq queries, including I won't hold approval for this since it's not a high frequency alloc but I do think it could be optimized |
||
| foreach (KeyValuePair<string, int> entry in nonDuplicates) | ||
| { | ||
| m_TickData.Add(entry.Key, entry.Value); | ||
| } | ||
| } | ||
|
|
||
| public int GetData(string fieldName) | ||
| { | ||
| return m_TickData.GetData(fieldName); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| namespace MLAPI.Profiling | ||
| { | ||
| public static class ProfilerConstants { | ||
| public const string NumberOfConnections = nameof(NumberOfConnections); | ||
| public const string ReceiveTickRate = nameof(ReceiveTickRate); | ||
|
|
||
| public const string NumberOfNamedMessages = nameof(NumberOfNamedMessages); | ||
| public const string NumberOfUnnamedMessages = nameof(NumberOfUnnamedMessages); | ||
| public const string NumberBytesSent = nameof(NumberBytesSent); | ||
| public const string NumberBytesReceived = nameof(NumberBytesReceived); | ||
| public const string NumberNetworkVarsReceived = nameof(NumberNetworkVarsReceived); | ||
| public const string NumberOfRPCsSent = nameof(NumberOfRPCsSent); | ||
| public const string NumberOfRPCsReceived = nameof(NumberOfRPCsReceived); | ||
| public const string NumberOfRPCBatchesSent = nameof(NumberOfRPCBatchesSent); | ||
| public const string NumberOfRPCBatchesReceived = nameof(NumberOfRPCBatchesReceived); | ||
| public const string NumberOfRPCQueueProcessed = nameof(NumberOfRPCQueueProcessed); | ||
| public const string NumberOfRPCsInQueueSize = nameof(NumberOfRPCsInQueueSize); | ||
| public const string NumberOfRPCsOutQueueSize = nameof(NumberOfRPCsOutQueueSize); | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not sure how to solve, but this is going to allocate on the heap for every network tick. I could see re-using problematic since we shared this data out and who knows whether consumers hold a reference to it / process async. But I'm curious if you or any other reviewers have any thoughts on ways to avoid an alloc here
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.
If we absolutely have to do an allocation a pool might help
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.
+1 to Matt's point and also turning things into
structs as much as we can might help a little on GC pressure.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.
also hashing and caching strings might help a little more on string allocation but I'm not sure how applicable these options are here.
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.
Since we went with the generic approach, it needs to exist on the heap (dictionary<,>) since it's an indeterminate size. Although we should definitely make the containers for the heap objects structs when possible to reduce it