refactor: Building out a test to get some surface coverage of the mlapi p… by kvassall-unity · Pull Request #646 · Unity-Technologies/com.unity.netcode.gameobjects

kvassall-unity · 2021-03-18T21:59:13Z

…rofiler functionality

becksebenius-unity

I don't see any red flags but waiting for replies before approve

becksebenius-unity · 2021-03-22T16:27:35Z

com.unity.multiplayer.mlapi/Runtime/Profiling/PerformanceDataManager.cs

        {
            s_TickId = Math.Max(s_TickId, 0);
+            s_TickId++;
            s_ProfilerData = new PerformanceTickData


you'll need to update this after the release->dev merge

hey @becksebenius-unity & @kvassall-unity — we had release to develop merge recently, so what's up? :)

Updated and rebased @MFatihMAR

becksebenius-unity · 2021-03-22T16:30:54Z

com.unity.multiplayer.mlapi/Tests/Editor/ProfilerTests.cs

+            testProfiler.Send();
+            TestHasProfilable.NotifyProfilerListeners();
+
+            BreakDownTestProfiler(useNullTransport);


You should use a [Teardown] method instead of the tailed method call here, otherwise an exception in a single test will leave it in a bad state

becksebenius-unity · 2021-03-22T16:32:55Z

com.unity.multiplayer.mlapi/Tests/Editor/ProfilerTests.cs

+        }
+
+        [Test]
+        public void TestNormalRegisterAndNotifyFlow()


About this test, and TestNormalRegisterAndNotifyFlowNull, there's no asserts in the test and the method name doesn't describe how it could fail - if this test were to fail on me due to some work I was doing, it would be hard for me to decipher what the intent of the test was. If at all possible I'd recommend adding explicit asserts for the fail cases you're testing

com.unity.multiplayer.mlapi/Tests/Editor/ProfilerTests.cs

…rofiler functionality

…t MLAPI could use directly

…be testing

mattwalsh-unity · 2021-03-23T22:24:22Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs


            NetworkConfig.NetworkTransport.Init();
+
+            ProfilerNotifier.Initialize(this);


Is this something we always do even if we're not running in the editor? Or put another way, what if I'm shipping the game and don't want the profiler overhead?

In the same vein, in NetworkProfiler.cs circa line 651 (and I know this is from a previous commit) we unconditionally call ProfilerBeginTick

So this was an intentional choice on our end, since there may be runtime tools that need this data. That doesn't mean users might not want to omit the profiling from the build, but we haven't provided that utility yet. Since we're just tracking counters in v1 the overhead is very small, but as we expand the data set this is going to be important. Maybe we could add a #if !MLAPI_DISABLE_STATS so that it's on by default, but users can turn it off with the compiler configuration.

I'd suggest that this belongs in a separate PR though. Kamau isn't introducing profiling into release builds with this PR, he's just refactoring functionality that already exists.

mattwalsh-unity · 2021-03-23T22:32:44Z

com.unity.multiplayer.mlapi/Runtime/Profiling/PerformanceDataManager.cs

        internal static void BeginNewTick()
        {
            s_TickId = Math.Max(s_TickId, 0);
+            s_TickId++;


Looks like a bugfix? Nice.

mattwalsh-unity

Would like to bottom out on when profiling code should be active / disabled

0xFA11 · 2021-03-23T22:36:15Z

com.unity.multiplayer.mlapi/Runtime/Profiling/IHasProfilableTransport.cs

@@ -0,0 +1,7 @@
+namespace MLAPI.Profiling
+{
+    public interface IHasProfilableTransport


what about IProfilableTransport, ITransportProfileProvider or ITransportProfileSource etc.? @kvassall-unity @becksebenius-unity — I'm just trying to nit-pick on the "Has" part :D

I don't mind the Has personally, but it does sound gramatically awkward - what about IProfilableTransportProvider (kind of a combination of your suggestions)

sounds good to me! :)

@kvassall-unity 👀

0xFA11 · 2021-03-23T22:37:47Z

com.unity.multiplayer.mlapi/Runtime/Profiling/ProfilerNotifier.cs

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


so obviously this PR introduces stuff more than just tests right?

I agree, originally the PR was just tests but should probably update the commit message to refactor with the expanded scope

becksebenius-unity

Updates to the tests look good to me, I understand the code isn't well suited to top-level asserts (yet?). Marking request changes, just semantics but I agree with Fatih that this should be a refactor: now instead of test:. Otherwise lgtm

kvassall-unity · 2021-03-25T15:01:05Z

Updates to the tests look good to me, I understand the code isn't well suited to top-level asserts (yet?). Marking request changes, just semantics but I agree with Fatih that this should be a refactor: now instead of test:. Otherwise lgtm

@becksebenius-unity Which changes were you waiting on? the title change or something specific?

becksebenius-unity · 2021-03-25T15:33:03Z

Updates to the tests look good to me, I understand the code isn't well suited to top-level asserts (yet?). Marking request changes, just semantics but I agree with Fatih that this should be a refactor: now instead of test:. Otherwise lgtm

@becksebenius-unity Which changes were you waiting on? the title change or something specific?

Sorry it wasn't clear, my "requested change" was to update the PR description. Marking approve now

com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs

kvassall-unity requested review from Rosme and becksebenius-unity March 18, 2021 21:59

becksebenius-unity requested a review from seanstolberg-unity March 22, 2021 16:37

becksebenius-unity reviewed Mar 22, 2021

View reviewed changes

seanstolberg-unity approved these changes Mar 22, 2021

View reviewed changes

seanstolberg-unity approved these changes Mar 23, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Tests/Editor/ProfilerTests.cs Show resolved Hide resolved

com.unity.multiplayer.mlapi/Tests/Editor/ProfilerTests.cs Outdated Show resolved Hide resolved

com.unity.multiplayer.mlapi/Tests/Editor/ProfilerTests.cs Show resolved Hide resolved

kvassall-unity added 5 commits March 23, 2021 16:53

test: Building out a test to get some surface coverage of the mlapi p…

551c353

…rofiler functionality

Updating to have a more testable interface and have reusable code tha…

d549d4c

…t MLAPI could use directly

Hooking the Network manager up to the ProfilerNotifier

fdeb073

Updating to make tests more explicit on the type of things we should …

749a945

…be testing

Cleaning up the tests some more

537067c

kvassall-unity force-pushed the feature/initial_barebones_proflier_test branch from cf766b0 to 537067c Compare March 23, 2021 22:01

mattwalsh-unity reviewed Mar 23, 2021

View reviewed changes

mattwalsh-unity suggested changes Mar 23, 2021

View reviewed changes

0xFA11 reviewed Mar 23, 2021

View reviewed changes

mattwalsh-unity approved these changes Mar 24, 2021

View reviewed changes

becksebenius-unity suggested changes Mar 24, 2021

View reviewed changes

kvassall-unity changed the title ~~test: Building out a test to get some surface coverage of the mlapi p…~~ refactor: Building out a test to get some surface coverage of the mlapi p… Mar 24, 2021

kvassall-unity and others added 2 commits March 24, 2021 15:57

Merge branch 'develop' into feature/initial_barebones_proflier_test

bcd35b9

More PR feedback

93437ad

More PR feedback

bee8bca

becksebenius-unity approved these changes Mar 25, 2021

View reviewed changes

0xFA11 reviewed Mar 25, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs Show resolved Hide resolved

kvassall-unity merged commit 1e650bb into develop Mar 25, 2021

kvassall-unity deleted the feature/initial_barebones_proflier_test branch March 25, 2021 16:45


		NetworkConfig.NetworkTransport.Init();

		ProfilerNotifier.Initialize(this);

Conversation

kvassall-unity commented Mar 18, 2021

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

mattwalsh-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!

becksebenius-unity left a comment

Choose a reason for hiding this comment

Uh oh!

kvassall-unity commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

becksebenius-unity commented Mar 25, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kvassall-unity commented Mar 25, 2021 •

edited

Loading