From ad4797ffcab54e88f863d2e2ce676bed6f9f9618 Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Tue, 11 Jul 2023 10:24:00 -0700 Subject: [PATCH] Tweak ping calculations We want to limit fluctuations caused by random noise on a small time scale, when we are reciving many samples in quick succession. This is common because our protocol acks every packet and each ack can be used to measure the ping, so when two peers are sending packets back and forth they basically get a constant stream of ping measurements. We also use the minimum as the aggregation function when many quick samples are combined into a single sample. See the comments for more info on why. P4:8183131 --- .../steamnetworking_statsutils.h | 10 ++++ .../steamnetworkingsockets_stats.cpp | 52 +++++++++++++++++-- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/steamnetworkingsockets/steamnetworking_statsutils.h b/src/steamnetworkingsockets/steamnetworking_statsutils.h index dca2ee4..f10c5aa 100644 --- a/src/steamnetworkingsockets/steamnetworking_statsutils.h +++ b/src/steamnetworkingsockets/steamnetworking_statsutils.h @@ -162,9 +162,19 @@ struct PingTracker /// a simple timestamp, or possibly because it will contain a sequence number, and /// we will be able to look up that sequence number and remember when we sent it.) SteamNetworkingMicroseconds m_usecTimeLastSentPingRequest; + + /// If we are getting a lot of samples in quick succession, we would like + /// our smoothed estimate to be based on data a bit more spaced apart. + static constexpr SteamNetworkingMicroseconds k_usecMinPingSampleSpacing = 100*1000; + protected: void Reset(); + /// If we get another sample before this time, then just update the last + /// sample, rather than adding a new sample. The idea here is that we want + /// our samples to have some minimum spacing in time. + SteamNetworkingMicroseconds m_usecTimeAllowNewSample; + /// Called when we receive a ping measurement void ReceivedPing( int nPingMS, SteamNetworkingMicroseconds usecNow ); }; diff --git a/src/steamnetworkingsockets/steamnetworkingsockets_stats.cpp b/src/steamnetworkingsockets/steamnetworkingsockets_stats.cpp index 012b7e8..7f50a21 100644 --- a/src/steamnetworkingsockets/steamnetworkingsockets_stats.cpp +++ b/src/steamnetworkingsockets/steamnetworkingsockets_stats.cpp @@ -77,6 +77,7 @@ void PingTracker::Reset() m_nValidPings = 0; m_nSmoothedPing = -1; m_usecTimeLastSentPingRequest = 0; + m_usecTimeAllowNewSample = 0; } void PingTracker::ReceivedPing( int nPingMS, SteamNetworkingMicroseconds usecNow ) @@ -84,9 +85,48 @@ void PingTracker::ReceivedPing( int nPingMS, SteamNetworkingMicroseconds usecNow Assert( nPingMS >= 0 ); COMPILE_TIME_ASSERT( V_ARRAYSIZE(m_arPing) == 3 ); - // Discard oldest, insert new sample at head - m_arPing[2] = m_arPing[1]; - m_arPing[1] = m_arPing[0]; + // A note about using the minimum here when combining samples. + // This is based on an idea from BBR that there is some underlying + // network ping which is relatively constant, and then there is + // random noise which adds delay. Note that one of the most important + // sources of random noise is the local delay when the operating system + // decides to wake us up, and we get a timeslice, acquire the necessary + // locks, etc, to process the packet. + // + // What is the "correct" way to combine samples depends on exactly what + // you are trying to estimate or measure. If you really want it + // to be an unbiased estimator of the next effective round trip, + // measured here in application space, then taking the minimum + // is not accurate. But in most cases, we actually don't really + // want this number, and what we really want to measure is just the + // more constant portion of the delay, and we want to ignore the + // extra noise. + + // Check if we're just updating the last bucket + if ( usecNow < m_usecTimeAllowNewSample ) + { + if ( nPingMS >= m_arPing[0].m_nPingMS ) + { + // We want to take the minimum, and the new + // sample is not smaller than a previous sample + // received very recently. Just update the time + // when we received it, and we're done. + m_arPing[0].m_usecTimeRecv = usecNow; + return; + } + } + else + { + + // Discard oldest, insert new sample at head + m_arPing[2] = m_arPing[1]; + m_arPing[1] = m_arPing[0]; + + // Reset time when we'll allow a new sample + m_usecTimeAllowNewSample = usecNow + k_usecMinPingSampleSpacing; + } + + // Record the sample m_arPing[0].m_nPingMS = nPingMS; m_arPing[0].m_usecTimeRecv = usecNow; @@ -97,12 +137,14 @@ void PingTracker::ReceivedPing( int nPingMS, SteamNetworkingMicroseconds usecNow // First sample. Smoothed value is simply the same thing as the sample m_nValidPings = 1; m_nSmoothedPing = nPingMS; + m_usecTimeAllowNewSample = 0; // Immediately allow new sample, no matter how fast it comes in break; case 1: - // Second sample. Smoothed value is the average + // Second sample. Use the minimum m_nValidPings = 2; - m_nSmoothedPing = ( m_arPing[0].m_nPingMS + m_arPing[1].m_nPingMS ) >> 1; + m_nSmoothedPing = std::min( m_arPing[0].m_nPingMS, m_arPing[1].m_nPingMS ); + m_usecTimeAllowNewSample = 0; // Immediately allow new sample, no matter how fast it comes in break; default: