feat: snapshot. Adding RTT computation API. #963

jeffreyrainy · 2021-07-15T21:18:37Z

This adds the API exposed by the Snapshot System for round-trip times. For now, those values are not filled, they will be in a future PR. But it should allow other development to proceed as it sets the API they will use.

… for now

jeffreyrainy · 2021-07-16T02:46:55Z

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

+
+namespace MLAPI
+{
+    internal class ClientRtt


I'll revise naming. Good point

mattwalsh-unity · 2021-07-16T02:49:53Z

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

+        [Test]
+        public void TestRtt()
+        {
+            var snapshot = new SnapshotSystem();


How about

a test that does > k_RingSize stores?

a test that ack's something that wasn't sent?

yeah, I'd rename this test to be something that communicates "send more, receive less".
also vice-versa option would be "a test that ack's something that wasn't sent?" aka "send less, receive more"

I'd also add:

send 2x more than k_RingSize

randomization: out-of-order send/recv variations (if ooo is ever possible, IDK)

randomization: sending/receiving (notify/ack) intentionally with varying artificatial latencies (WaitForSeconds)

at least, these are the test cases comes to my mind in a minute — perhaps more tests if applicable/sensible?

mattwalsh-unity · 2021-07-16T02:51:34Z

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

+            ret.Best = m_MeasuredLatencies[m_LatenciesBegin];
+            ret.Worst = m_MeasuredLatencies[m_LatenciesBegin];
+
+            while (index != m_LatenciesEnd)


I'll bet there is a Google interview question that asks how to do this operation in constant time :)

I'm familiar with keeping a running sum and subtracting the element that are removed at the other end of the ring buffer. Then, you can divide by the number of elements.

However, I don't know of a good way to do it for the best and worst entries. I believe we can only do O(log(n)) but that would require an associative container for a handful of latencies. Seemed Overkill. Is it possible your comment forgot to consider min/max latency?

For min/max just keep track of the min/max and then compare < & > when adding / removing from the ring buffer?

So, let's say stored latencies are [10, 60, 20, 60, 11, 80 ], min is 10, max is 80, a new latency of 15 needs to be stored. We now have [60, 20, 60, 11, 80, 15] (not in those positions, though). I can remove the min: 10, I can keep the max 80, but finding min: 11 requires a traversal.

I guess you could use 2 priority queues or heaps, one for min. But to your point we can save this for benchmarking

mattwalsh-unity

I'm actually good with the exception of the 2 tests I called out

mattwalsh-unity · 2021-07-16T02:53:24Z

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

+            if (m_SendKey[key % k_RingSize] == key)
+            {
+                double latency = when - m_RttSendTimes[key % k_RingSize];
+                Debug.Log(string.Format("Measured latency of {0}", latency));


I assume you don't want this checked in tho

mattwalsh-unity · 2021-07-16T02:56:23Z

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

+        /// <summary>
+        /// Returns the Round-trip-time computation for this client
+        /// </summary>
+        public Rtt GetRtt()


I guess one thing to think about - how many times do you think you might call GetRtt in a given frame? if you call it a bunch, you might have a dirty flag here and usually return a cached value. OR, if you need RTT almost every frame and call this multiple times, maybe the computation of the RTT could be in the NotifyAck? I guess that depends on how often you don't actually need the RTT data.

I lean this being "premature optimization" at the moment. I don't want to be cavalier about the feedback, it's good and very valid. But I'd wait until it shows in a profile. With the irons we already have in the fire, I'd wait before adding complexity here. Would that be workable to you?

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

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

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

0xFA11 · 2021-07-17T17:08:45Z

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

+            }
+        }
+
+        private const int k_RttSize = 5; // number of RTT to keep an average of (plus one)


maybe RttCount vs MaxRttCount vs RttCacheSize? "size" sounds too generic and at the first glance, I thought this was some sort of RTT packet size :P

0xFA11 · 2021-07-17T17:10:27Z

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

+        private const int
+            k_RingSize = 64; // number of slots to use for RTT computations (max number of in-flight packets)
+
+        private double[] m_RttSendTimes; // times at which packet were sent for RTT computations


in seconds? perhaps m_RttSendTimeSecs?

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

0xFA11 · 2021-07-17T17:11:21Z

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

+
+        private double[] m_RttSendTimes; // times at which packet were sent for RTT computations
+        private int[] m_SendKey; // tick (or other key) at which packets were sent (to allow matching)
+        private double[] m_MeasuredLatencies; // measured latencies (ring buffer)


seconds? :)

0xFA11 · 2021-07-17T17:13:53Z

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


        private ushort m_CurrentTick = 0;

+        internal ClientRtt Rtt(ulong clientId)


we don't call NotifySend or NotifyAck from anywhere yet.
are you planning to have RTT & SnapshotSystem integration in this PR too?

sorry, just saw this:

For now, those values are not filled, they will be in a future PR. But it should allow other development to proceed as it sets the API they will use.

AFAIK, we primarily wanted to have RTT for snapshots and NetVars. are there any other systems being unblocked by this PR? If not, personally, I think RTT integration with snapshot should go under one single PR — otherwise, we're not really shipping this as a feature but as a boilerplate. what would you think?

Yes, there is some other systems blocked by this this. Luke's work on Time System will integrate with this.

alright. just double-checking: without having this functioning (yet) but still having its boilerplate in-place would unblock Luke because his work on TimeSystem is not dependant to RTT calculation but merely its existence? I'm confused. @LukeStampfli ? 👀

So based on your feedback on the time system I got, I'm exploring using the existing time sync for the time system until this is ready @MFatihMAR. I'm not sure yet whether that will work well and for release we will want to use Jeff's system. knowing the API with which I will integrate with makes it easier for me to plan ahead.

But yeah just an API which doesnt' work yet does not "unblock" me but it allows me to push my work with the confidence that there is a high probability that I will be able to integrate the time system in the future.

so, having this "agreed in principle" would unblock you right? if so, again, I think this PR should go in with its snapshot integration/implementation in a functioning state, otherwise we're shipping a boilerplate, not a feature. I do not expect public APIs to change at this point, Luke is already unblocked from that standpoint then. what would you think @jeffreyrainy ? 👀

My position is unchanged. I'd still like to PR this. Thanks for raising your concerns.

sure, fair enough.

LukeStampfli · 2021-07-19T11:06:32Z

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

+
+namespace MLAPI
+{
+    internal class ClientRtt


Could be a struct instead of a class?

Agreed this should be a struct

LukeStampfli

Implements what was discussed in the RFC so looks good to me.

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

… constants

mattwalsh-unity · 2021-07-19T17:22:13Z

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

+        {
+            var snapshot = new SnapshotSystem();
+            var client1 = snapshot.Rtt(0);
+            var iterationCount = 200;


Should this be in terms of k_RingSize perhaps? (like k_RingSize * 3 ?)

Suggested change

var iterationCount = 200;

var iterationCount = k_RingSize * 3;

k_RingSize is private, and afaik, c# doesn't have friend. This raise a few possibilities:

make the ring size a publicly available setting with a setter, and getter

only expose the ring size with a readonly getter

leave the test with hard-coded values, as-is.

Do you have a preference?

we have internal in C# and internals are exposed to test asmdefs IIRC

mattwalsh-unity · 2021-07-19T17:22:56Z

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

+            var snapshot = new SnapshotSystem();
+            var client1 = snapshot.Rtt(0);
+            var iterationCount = 200;
+            var extraCount = 100;


Suggested change

var extraCount = 100;

var extraCount = iterationCound / 2;

…ngSize and RttSize

…nsform * develop: feat: snapshot. Adding RTT computation API. (#963) feat: snapshot. Milestone 1b. Testproject "manual test" "scene transitioning" working with snapshot. Disabled by default. (#862) test: multiprocess tests part 6: fixing issues runnings all tests together (#957) docs: Perf tests part 5. Adding documentation and instructions (#952) test: Perf tests part 4. Adding example of performance test with spawning x network objects at once (#925) test: Correctly teardown OnNetworkSpawn/Despawn tests.

jeffreyrainy added 2 commits July 15, 2021 17:10

feat: snapshot. Adding the RTT computation API. Not actually computed…

153c44a

… for now

test: snapshot. RTT computation test.

ee0ce09

jeffreyrainy requested review from LukeStampfli and mattwalsh-unity July 15, 2021 21:18

jeffreyrainy commented Jul 16, 2021

View reviewed changes

mattwalsh-unity reviewed Jul 16, 2021

View reviewed changes

mattwalsh-unity suggested changes Jul 16, 2021

View reviewed changes

mattwalsh-unity reviewed Jul 16, 2021

View reviewed changes

0xFA11 reviewed Jul 17, 2021

View reviewed changes

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

0xFA11 reviewed Jul 17, 2021

View reviewed changes

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

0xFA11 reviewed Jul 17, 2021

View reviewed changes

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

0xFA11 reviewed Jul 17, 2021

View reviewed changes

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

0xFA11 reviewed Jul 17, 2021

View reviewed changes

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

0xFA11 reviewed Jul 17, 2021

View reviewed changes

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

0xFA11 reviewed Jul 17, 2021

View reviewed changes

LukeStampfli reviewed Jul 19, 2021

View reviewed changes

LukeStampfli approved these changes Jul 19, 2021

View reviewed changes

feat: snapshot. RTT computation PR comments

035f084

jeffreyrainy changed the title ~~Experimental/rtt computation snapshot~~ feat: snapshot. Adding RTT computation API. Jul 19, 2021

0xFA11 reviewed Jul 19, 2021

View reviewed changes

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

test: RTT test style improvement, moved constants from local to class…

b1df742

… constants

mattwalsh-unity reviewed Jul 19, 2021

View reviewed changes

feat: snapshot. RTT computation PR comments, internally accessible Ri…

ee36cc3

…ngSize and RttSize

mattwalsh-unity approved these changes Jul 19, 2021

View reviewed changes

jeffreyrainy merged commit fa2be6f into develop Jul 19, 2021

jeffreyrainy deleted the experimental/rtt-computation-snapshot branch July 19, 2021 22:07


		private ushort m_CurrentTick = 0;

		internal ClientRtt Rtt(ulong clientId)

	var iterationCount = 200;
	var iterationCount = k_RingSize * 3;

feat: snapshot. Adding RTT computation API. #963

feat: snapshot. Adding RTT computation API. #963

Uh oh!

Conversation

jeffreyrainy commented Jul 15, 2021

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xFA11 Jul 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jeffreyrainy Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

LukeStampfli left a comment

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!

mattwalsh-unity Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

0xFA11 Jul 17, 2021 •

edited

Loading

jeffreyrainy Jul 19, 2021 •

edited

Loading

mattwalsh-unity Jul 19, 2021 •

edited

Loading