feat: Adding in Performance Tick Data that stores data for external consumption (RFC #7) by kvassall-unity · Pull Request #491 · Unity-Technologies/com.unity.netcode.gameobjects

kvassall-unity · 2021-02-22T17:15:50Z

Implements RFC #7: Performance Data Propagation (tracked by Track RFC #7: Performance Data Propagation #484)
Adds in Performance Tick Data that stores data for external consumption
Add an interface ITransportProfilerData for transports to relay transport specific information

becksebenius-unity · 2021-02-22T17:22:33Z

                }
            }
+
+            var profileTransport = NetworkConfig.NetworkTransport as ITransportProfilerData;


This code is totally fine but I always like to share newer c# syntax, this could also be
if(NetworkConfig.NetworkTransport is ITransportProfilerData profileTransport)

(tbh I'm still not 100% sold on the concept of inline variable declarations, it's been growing on me)

becksebenius-unity · 2021-02-22T17:23:33Z

+{
+    public interface ITransportProfilerData
+    {
+        Dictionary<string, int> GetTransportGetData();


Confused by this method name. Should it be something like GetTransportProfilerData ?

becksebenius-unity · 2021-02-22T17:29:37Z

+
+namespace MLAPI.Profiling
+{
+    public static class PerformanceDataManager


Should this class be internal? Do we ever expect external (non-MLAPI) code to call these APIs?

If we do, I think we should at least make BeginNewTick, AddTransportData, and GetData to be internal at the member level

becksebenius-unity · 2021-02-22T17:33:32Z

+
+        public static void BeginNewTick()
+        {
+            s_ProfilerData = new PerformanceTickData


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

If we absolutely have to do an allocation a pool might help

+1 to Matt's point and also turning things into structs as much as we can might help a little on GC pressure.

also hashing and caching strings might help a little more on string allocation but I'm not sure how applicable these options are here.

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

becksebenius-unity · 2021-02-22T17:36:03Z

@@ -0,0 +1,21 @@
+namespace MLAPI.Profiling
+{
+    public enum ProfilerConstants


I'm still not convinced about using an enum here instead of string constants - why bother using a Dictionary, if we're restricting the set of values anyways? I think we should go one way or the other, either make it an explicit data set, or make it extensible. Right now we're taking all the cost of a Dictionary (loose abstraction, heap alloc) and none of the benefits (extensibility)

I do agree with what Beck says here. However, this can offer a clear distinction between standard and custom data to profile. Not sure if we're aiming for such support right now.

Rosme · 2021-02-23T20:49:55Z

+        {
+            s_ProfilerData = new PerformanceTickData
+            {
+                tickID = s_TickID++,


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.

Rosme · 2021-02-23T21:12:22Z

@@ -0,0 +1,21 @@
+namespace MLAPI.Profiling
+{
+    public enum ProfilerConstants


I do agree with what Beck says here. However, this can offer a clear distinction between standard and custom data to profile. Not sure if we're aiming for such support right now.

mattwalsh-unity · 2021-02-24T02:27:45Z

+            m_Dictionary.Add(fieldName, value);
+        }
+
+        public void Increment(string fieldName, int count = 1)


Why not just make the type ProfilerConstants and avoid the ToString()

IT cant be typed. Each transport will have its own types of strings that it wants to propagate up.

Oddly enough, we had the same thing in de-string-ifying the channels in MLAPI. They too have per-transport customization, so we still established a set of enums in the SDK with a 'LAST_ENTRY' enum, but then if your transport wants more than that you just do LAST_ENTRY + 1, etc. That let us have our type safety / low overhead but also be extensible. We do this in Transport.cs:8 and then like in TransportTest.cs:42

becksebenius-unity

Close to approving but we need to address the enum.ToString() issue because as Matt calls out this will cause a lot of runtime allocations. I'd suggest changing them to string constants instead of using an enum.

becksebenius-unity · 2021-02-26T01:41:04Z

+
+        public void AddNonDuplicateData(IReadOnlyDictionary<string, int> transportProfilerData)
+        {
+            IEnumerable<KeyValuePair<string, int>> nonDuplicates = transportProfilerData.Where(entry => !m_TickData.HasData(entry.Key));


this linq query is unnecessary here. you could do the HasData check inside the loop and avoid the linq cost

https://stackoverflow.com/questions/4044400/linq-performance-faq

The allocations and performance AFAIK are the same. nonDuplicates isn't an allocation and we probably save speed by iterating over less entries.
That is to say, the line in question doesn't get evaluated until the bottom for loop lazily. And I feel that this reads better.

Almost all Linq queries, including .Where, allocate an IEnumerable on the heap to wrap the iteration of the collection. this line also allocates an anonymous function - it can't be static since it captures the member field m_TickData

I won't hold approval for this since it's not a high frequency alloc but I do think it could be optimized

0xFA11 · 2021-02-26T12:56:23Z

+        public delegate void PerformanceDataEventHandler(PerformanceTickData profilerData);
+
+        public event PerformanceDataEventHandler OnPerformanceDataEvent;


#if UNITY_EDITOR || DEVELOPMENT_BUILD maybe? I wonder how are we planning to strip-out performance profiling stuff for non-Editor, non-Development targets, maybe I'm missing something?

For performance reporting, I don't think we want to. The profiler itself should be stripped out in non-profiler but the data output itself is potentially relevant for end-user tools at runtime (for example, the net stats overlay). That said, users may still want to strip it out for performance reasons, but I think that should be a runtime check

I'm doing 1-to-1 parity right now. so since these weren't previously in an #if then i'm not doing that in this PR.

Ideally we could ifdef all instances of this class if we want.

…al consumption

… tick

Move transport constants to the appropriate location

mattwalsh-unity · 2021-02-26T19:11:21Z

                    long readStartPos = stream.Position;

                    networkedVarList[i].ReadDelta(stream, IsServer);
+                    PerformanceDataManager.Increment(ProfilerConstants.NumberNetworkVarsReceived.ToString());


The ToString() is being called on a string :)

mattwalsh-unity

Mainly there are a bunch of cases where we're calling ToString on the string in the .increment calls

unity-cla-assistant · 2021-02-26T19:16:48Z

All committers have signed the CLA.

mattwalsh-unity

Ok, I went ahead and removed the extra toString calls

kvassall-unity requested review from 0xFA11, NoelStephensUnity, Rosme, becksebenius-unity and mattwalsh-unity February 22, 2021 17:15

becksebenius-unity reviewed Feb 22, 2021

View reviewed changes

becksebenius-unity suggested changes Feb 22, 2021

View reviewed changes

kvassall-unity requested a review from becksebenius-unity February 23, 2021 16:03

kvassall-unity force-pushed the feature/performance_data_propagation branch from 17f3358 to 2bf5b3e Compare February 23, 2021 16:06

kvassall-unity requested a review from wackoisgod February 23, 2021 19:10

Rosme reviewed Feb 23, 2021

View reviewed changes

mattwalsh-unity reviewed Feb 24, 2021

View reviewed changes

Comment thread com.unity.multiplayer.mlapi/Runtime/Core/NetworkedBehaviour.cs Outdated

mattwalsh-unity reviewed Feb 24, 2021

View reviewed changes

kvassall-unity changed the title ~~feat: Performance Data Propagation (RFC #7)~~ feat: Adding in Performance Tick Data that stores data for external consumption (RFC #7) Feb 24, 2021

kvassall-unity force-pushed the feature/performance_data_propagation branch from 83add46 to ed7b4a4 Compare February 24, 2021 20:21

becksebenius-unity suggested changes Feb 26, 2021

View reviewed changes

becksebenius-unity reviewed Feb 26, 2021

View reviewed changes

0xFA11 reviewed Feb 26, 2021

View reviewed changes

kvassall-unity added 8 commits February 26, 2021 10:35

Adding in Performance Tick Data that can be propagated out for extern…

348e0d2

…al consumption

Addressing all the current review feedback

0a95e8b

Handle Null cases

7f79aba

Refactor so transports reset their counters at the beginning of a new…

c3a5e77

… tick

renaming this better

393508d

Simplify the code even more

305b6dc

Don't overflow to negative ticks

9e78479

Move transport constants to the appropriate location

Don't allocate strings

0529ef4

kvassall-unity force-pushed the feature/performance_data_propagation branch from 957151b to 0529ef4 Compare February 26, 2021 16:36

Making a static event so registering to it can take place at any time

59ff762

becksebenius-unity approved these changes Feb 26, 2021

View reviewed changes

mattwalsh-unity reviewed Feb 26, 2021

View reviewed changes

mattwalsh-unity suggested changes Feb 26, 2021

View reviewed changes

removed extra toString()

82c1f26

mattwalsh-unity approved these changes Feb 26, 2021

View reviewed changes

Merge branch 'develop' into feature/performance_data_propagation

91f0533

mattwalsh-unity merged commit 18c1a0a into develop Feb 26, 2021

0xFA11 deleted the feature/performance_data_propagation branch March 5, 2021 17:44

		public delegate void PerformanceDataEventHandler(PerformanceTickData profilerData);

		public event PerformanceDataEventHandler OnPerformanceDataEvent;

Conversation

kvassall-unity commented Feb 22, 2021 • 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!

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!

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!

becksebenius-unity left a comment

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!

mattwalsh-unity left a comment

Choose a reason for hiding this comment

Uh oh!

unity-cla-assistant commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattwalsh-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kvassall-unity commented Feb 22, 2021 •

edited

Loading

unity-cla-assistant commented Feb 26, 2021 •

edited

Loading