Board index FlightGear Development New features

Defensive moderation of MultiPlayer position reports

Discussion and requests for new features. Please note that FlightGear developers are volunteers and may or may not be able to consider these requests.

Defensive moderation of MultiPlayer position reports

Postby MariuszXC » Thu Jul 01, 2021 10:54 am

So I noticed that, on my not so new hardware, frame rate was really dropping in multiplayer mode, as compared to flying the same route and scenario in single player.
FlightGear allows to define the frequency of position reports, but most users seem to leave it at a default setting, which is probably fine for users with strong CPUs, but not optimal for less capable hardware.
In addition to that, packets tend to arrive not evenly spaced but in bursts, which concentrates the processing load.

With the above in mind I wrote a patch to address this issue on the receiving end. The idea is simple: remember when the first position report from a given callsign was processed and ignore subsequent reports from that callsign for a configured amount of time. Then the cycle repeats. This is applied to position reports from all callsigns. Only position reports are moderated, chat messages etc. are processed as before.

The default behaviour of this code path is inactive - i.e. FlightGear acts as normal. To activate this defensive moderation of incoming position reports, a property /sim/multiplay/moderate-position-reports must be set to true and moderation interval (in seconds) must be increased from default 0.0 in /sim/multiplay/position-moderation-interval.
I tested intervals of 5 and 10 seconds.

The patch should apply cleanly to recent 'next' branch. If there is interest I can also make a patch for current 'stable'.
I tested this patch during our Wednesday multiplayer event and it worked as expected, that is, frame rate was comparable to flying in single player, with the (expected) side effect of other players' aircraft appear to 'jump' to a new position every now and then.

Code: Select all
diff --git a/src/MultiPlayer/multiplaymgr.cxx b/src/MultiPlayer/multiplaymgr.cxx
index fcaa3c7..657c704 100644
--- a/src/MultiPlayer/multiplaymgr.cxx
+++ b/src/MultiPlayer/multiplaymgr.cxx
@@ -8,6 +8,7 @@
 // Copyright (C) 2003  Airservices Australia
 // Copyright (C) 2005  Oliver Schroeder
 // Copyright (C) 2006  Mathias Froehlich
+// Copyright (C) 2021  Mariusz Matuszek (moderation of position reports)
 //
 // This program is free software; you can redistribute it and/or
 // modify it under the terms of the GNU General Public License as
@@ -35,6 +36,8 @@
 #include <errno.h>
 #include <memory>
 
+#include <chrono>
+
 #include <simgear/debug/logstream.hxx>
 #include <simgear/math/sg_random.hxx>
 #include <simgear/misc/sg_dir.hxx>
@@ -1059,6 +1062,10 @@ FGMultiplayMgr::FGMultiplayMgr()
   pReplayState = fgGetNode("/sim/replay/replay-state", true);
   pLogRawSpeedMultiplayer = fgGetNode("/sim/replay/log-raw-speed-multiplayer", true);
 
+  pMultiPlayModeratePositionReports = fgGetNode("/sim/multiplay/moderate-position-reports", true);
+  pMultiPlayPositionModerationInterval = fgGetNode("/sim/multiplay/position-moderation-interval", true);
+  fgSetBool("/sim/multiplay/moderate-position-reports", false);
+  fgSetDouble("/sim/multiplay/position-moderation-interval", 0.0);
 
 } // FGMultiplayMgr::FGMultiplayMgr()
 //////////////////////////////////////////////////////////////////////
@@ -1960,6 +1967,14 @@ FGMultiplayMgr::update(double dt)
   /// Just for expiry
   long stamp = SGTimeStamp::now().getSeconds();
 
+  // For incoming position report moderation
+  std::chrono::duration<double> time_span;
+  std::chrono::steady_clock::time_point time_now;
+  bool moderatePositionReports = pMultiPlayModeratePositionReports->getBoolValue();
+  double positionReportModerationInterval = pMultiPlayPositionModerationInterval->getDoubleValue();
+
+  if (positionReportModerationInterval < 0.0) positionReportModerationInterval = 0.0;
+
   //////////////////////////////////////////////////
   //  Send if required
   //////////////////////////////////////////////////
@@ -2023,7 +2038,22 @@ FGMultiplayMgr::update(double dt)
       ProcessChatMsg(msgBuf, SenderAddress);
       break;
     case POS_DATA_ID:
-      ProcessPosMsg(msgBuf, SenderAddress, stamp);
+      // cout << "+++++moderation: " << moderatePositionReports << ":" << positionReportModerationInterval << "\n";
+      if (moderatePositionReports) { // moderate frequent position updates
+        time_now  = std::chrono::steady_clock::now();
+        time_span = std::chrono::duration_cast<std::chrono::duration<double>>(
+          time_now - lastPositionReportTime[MsgHdr->Callsign]
+        );
+        if (time_span.count() >= positionReportModerationInterval) {
+          //cout << "\n++" << MsgHdr->Callsign << ":" << time_span.count() << " " ; //position message processed
+          lastPositionReportTime[MsgHdr->Callsign] = time_now;
+          ProcessPosMsg(msgBuf, SenderAddress, stamp);
+        } else {
+          //cout << "--" << MsgHdr->Callsign << ":" << time_span.count() << " " ;  // position message skipped
+        }
+      } else { // do not moderate
+        ProcessPosMsg(msgBuf, SenderAddress, stamp);
+      }
       break;
     case UNUSABLE_POS_DATA_ID:
     case OLD_OLD_POS_DATA_ID:
diff --git a/src/MultiPlayer/multiplaymgr.hxx b/src/MultiPlayer/multiplaymgr.hxx
index e65417a..7505479 100644
--- a/src/MultiPlayer/multiplaymgr.hxx
+++ b/src/MultiPlayer/multiplaymgr.hxx
@@ -7,6 +7,7 @@
 //
 // Copyright (C) 2003  Airservices Australia
 // Copyright (C) 2005  Oliver Schroeder
+// Copyright (C) 2021  Mariusz Matuszek (moderation of position reports)
 //
 // This program is free software; you can redistribute it and/or
 // modify it under the terms of the GNU General Public License as
@@ -39,6 +40,9 @@ const int MAX_MP_PROTOCOL_VERSION = 2;
 #include <vector>
 #include <memory>
 
+#include <chrono>
+#include <unordered_map>
+
 #include <simgear/compiler.h>
 #include <simgear/props/props.hxx>
 #include <simgear/io/raw_socket.hxx>
@@ -145,6 +149,9 @@ private:
     SGPropertyNode *pMultiPlayTransmitPropertyBase;
     SGPropertyNode *pReplayState;
     SGPropertyNode *pLogRawSpeedMultiplayer;
+
+    SGPropertyNode *pMultiPlayModeratePositionReports;
+    SGPropertyNode *pMultiPlayPositionModerationInterval;
   
     typedef std::map<unsigned int, const struct IdPropertyList*> PropertyDefinitionMap;
     PropertyDefinitionMap mPropertyDefinition;
@@ -158,6 +165,10 @@ private:
 
     std::deque<std::shared_ptr<std::vector<char>>>  mRecordMessageQueue;
     std::deque<std::shared_ptr<std::vector<char>>>  mReplayMessageQueue;
+
+    // Map of last seen position reports
+    typedef unordered_map<string, std::chrono::steady_clock::time_point> LastSeenMap;
+    LastSeenMap lastPositionReportTime;
 };
 
 #endif
INOP
MariuszXC
 
Posts: 1061
Joined: Tue May 18, 2021 5:38 pm
Location: Europe
Callsign: SP-MRM
Version: 2020.4
OS: Ubuntu 16.04

Re: Defensive moderation of MultiPlayer position reports

Postby merspieler » Thu Jul 01, 2021 11:22 am

Just to make sure... have you checked that it's not a rendering side issues? ie. by turning the LOD ranges for MP aircraft way down...
Nia (you&, she/her)

Please use gender neutral terms when referring to a group of people!

Be the change you wish to see in the world, be an ally to all!

Join the official matrix space
merspieler
 
Posts: 2241
Joined: Thu Oct 26, 2017 11:43 am
Location: Wish to be in YBCS
Pronouns: you&, she/her
Callsign: you&, she/her
IRC name: merspieler
Version: next
OS: NixOS

Re: Defensive moderation of MultiPlayer position reports

Postby MariuszXC » Thu Jul 01, 2021 11:29 am

I did not check LOD, because it seemed it was not the visibility but presence of other aircraft which caused the issue. But I will test this further then.
INOP
MariuszXC
 
Posts: 1061
Joined: Tue May 18, 2021 5:38 pm
Location: Europe
Callsign: SP-MRM
Version: 2020.4
OS: Ubuntu 16.04

Re: Defensive moderation of MultiPlayer position reports

Postby cgdae » Thu Jul 01, 2021 12:49 pm

I'm surprised that ignoring multiplayer messages makes a significant difference to Flightgear's framerate - all that ProcessPosMsg() does is decode the XDR data in mp packets and set various local properties, which shouldn't take much time.

I guess it's possible that a particular aircraft might define listeners for certain properties that run very slowly?

The only other thing i can think of is that on some systems complicated dialogues can causes Flightgear to run slowly when using the default PUI style. So maybe frequently updating the information in the Pilots List dialogue could be causing the slowdown? Does closing this dialogue avoid the slowdown? [Might also be worth trying changing the style by modifying /sim/gui/current-style to see whether the alternatives (which run faster on some systems, but look pretty terrible IMHO) improve things.]

One other thing - i think that CHAT_MSG_ID is not actually used; instead chat text is included in POS_DATA_ID messages (i think this works by making them set a property). So i suspect this patch might lead to chat messages being delayed.

Thanks,

- Jules
cgdae
 
Posts: 117
Joined: Tue May 31, 2016 8:35 pm

Re: Defensive moderation of MultiPlayer position reports

Postby MariuszXC » Thu Jul 01, 2021 6:15 pm

cgdae wrote in Thu Jul 01, 2021 12:49 pm:.. all that ProcessPosMsg() does is decode the XDR data in mp packets and set various local properties, which shouldn't take much time..


But doesn't change of properties have a potential for triggering listeners acting on those changes?

The only other thing i can think of is that on some systems complicated dialogues can causes Flightgear to run slowly when using the default PUI style. So maybe frequently updating the information in the Pilots List dialogue could be causing the slowdown? Does closing this dialogue avoid the slowdown?[...]


I keep the Pilot List closed at all times, so can not answer the question.

One other thing - i think that CHAT_MSG_ID is not actually used; instead chat text is included in POS_DATA_ID messages (i think this works by making them set a property). So i suspect this patch might lead to chat messages being delayed.


I assumed that CHAT_MSG_ID is actually used. In my Wednesday testing communication was mostly via Mumble, but there were occasional chat exchanges and I received them. Of course these might have been included in those 'lucky' packets which got through the moderation.. In other words, chat appeared to function, but there was no formal testing if it is really so.

Should that be by pure luck.. is there an easy way to determine if there is a chat property attached to a position report, before passing the message to ProcessPosMsg? If yes, an exception could be made for such messages.
INOP
MariuszXC
 
Posts: 1061
Joined: Tue May 18, 2021 5:38 pm
Location: Europe
Callsign: SP-MRM
Version: 2020.4
OS: Ubuntu 16.04

Re: Defensive moderation of MultiPlayer position reports

Postby cgdae » Fri Jul 02, 2021 1:45 pm

MariuszXC wrote in Thu Jul 01, 2021 6:15 pm:
cgdae wrote in Thu Jul 01, 2021 12:49 pm:.. all that ProcessPosMsg() does is decode the XDR data in mp packets and set various local properties, which shouldn't take much time..


But doesn't change of properties have a potential for triggering listeners acting on those changes?


Yes, it does. (i think i mentioned this in my post too).

If these listeners are the cause of the slow-down, then it would be better to fix them than change the core code to ignore some of the incoming MP packets.

I wonder whether these listeners could be in aircraft-specific Nasal code?


I keep the Pilot List closed at all times, so can not answer the question.


Ah, ok, so that's one less thing to worry about here.

One other thing - i think that CHAT_MSG_ID is not actually used; instead chat text is included in POS_DATA_ID messages (i think this works by making them set a property). So i suspect this patch might lead to chat messages being delayed.


I assumed that CHAT_MSG_ID is actually used. In my Wednesday testing communication was mostly via Mumble, but there were occasional chat exchanges and I received them. Of course these might have been included in those 'lucky' packets which got through the moderation.. In other words, chat appeared to function, but there was no formal testing if it is really so.


Interesting; maybe CHAT_MSG_ID is used by older releases, or maybe i'm just confused about it not being used.

Should that be by pure luck.. is there an easy way to determine if there is a chat property attached to a position report, before passing the message to ProcessPosMsg? If yes, an exception could be made for such messages.


When i last looked at this, it wasn't trivial, unfortunately. I think that the way things work is that MP Packets can send lists of integer codes that identify particular predefined property paths, plus values, which are then written to the recipient's property tree. In this case, code 10002 is for sim/multiplay/chat.

Then fgdata:Nasal/multiplayer.nas:check_messages() polls this property and appends text to the chat dialogue. Polling seems dodgy here - iI worry that we could miss messages if two or more arrive in between check_messages() being called.

Thanks

- Jules
cgdae
 
Posts: 117
Joined: Tue May 31, 2016 8:35 pm

Re: Defensive moderation of MultiPlayer position reports

Postby MariuszXC » Fri Jul 02, 2021 2:54 pm

Jules, thank you for taking time to look into this.

In the meantime I came to similar conclusions, (line 345 of multiplaymgr.cxx) propertyId of 10002, introduced in V_1_1_2 of the protocol (if I understand the tag meaning correctly).

So my patch indeed has potential to not deliver some (or indeed most) of chat messages :/ and there is no easy way to make exceptions for such packets by testing for presence of this prop ID, because the message buffer is interpreted by sequential iteration over included properties. That is a setback.

Well, I do care more about being able to control my plane in multiplayer (i.e. having frame rate at least above 10) than about chat messages, so I will keep using this patch (and also because I think that this kind of moderation is a good thing from security point of view), but obviously it is not for everyone.

I am hesitant to write this, but I think this way of sending very infrequent data (chat messages) is one of the most bizzare pseudo-optimisations I have ever encountered. Seems to be a lot of pain for a very little gain. I would love to read a discussion and arguments which led to this situation. Does anybody have a link to the relevant topic(s) in the mailing list archives?

Thanks,
Mariusz
INOP
MariuszXC
 
Posts: 1061
Joined: Tue May 18, 2021 5:38 pm
Location: Europe
Callsign: SP-MRM
Version: 2020.4
OS: Ubuntu 16.04

Re: Defensive moderation of MultiPlayer position reports

Postby MariuszXC » Sat Jul 03, 2021 10:54 pm

MariuszXC wrote in Fri Jul 02, 2021 2:54 pm: Does anybody have a link to the relevant topic(s) in the mailing list archives?


So I think I found the relevant entry in the archives. It seems we have this way of sending chat messages since the chat patch was proposed in 2006.. https://sourceforge.net/p/flightgear/ma ... sg13968945

Oh well.. Guess it is too late to do anything about it now.

So what is the CHAT_MSG_ID doing in the code? Has it ever been used at all?
INOP
MariuszXC
 
Posts: 1061
Joined: Tue May 18, 2021 5:38 pm
Location: Europe
Callsign: SP-MRM
Version: 2020.4
OS: Ubuntu 16.04

Re: Defensive moderation of MultiPlayer position reports

Postby Richard » Mon Jul 05, 2021 12:04 am

MariuszXC wrote in Thu Jul 01, 2021 10:54 am:So I noticed that, on my not so new hardware, frame rate was really dropping in multiplayer mode, as compared to flying the same route and scenario in single player.


Interesting observations. Generally I've never seen a packet burst.

Firstly the MP code should be able to handle this without a performance hit; the amount of CPU to process MP messages should be reasonably trivial; don't forget that you can turn on the overrun detection in the performance monitor property tree to see what's happening.

It could be that one of the models has listeners on a property; the easiest way to find out which one is causing the problem is to turn off all of the pilots in the MP list and then turn one on at a time.

I suspect if we were to fix the packet burst it should probably be done on the server to avoid sending the packets; but this is something for the future.

Firstly we should figure out what is going on here; there are other possible implications of packet bursts.
Richard
 
Posts: 810
Joined: Sun Nov 02, 2014 11:17 pm
Version: Git
OS: Win10

Re: Defensive moderation of MultiPlayer position reports

Postby cgdae » Mon Jul 05, 2021 10:47 am

MariuszXC wrote in Fri Jul 02, 2021 2:54 pm:I am hesitant to write this, but I think this way of sending very infrequent data (chat messages) is one of the most bizzare pseudo-optimisations I have ever encountered. Seems to be a lot of pain for a very little gain. I would love to read a discussion and arguments which led to this situation. Does anybody have a link to the relevant topic(s) in the mailing list archives?


It does seem odd to me also. It was done well before i became involved witih Flightgear so i don't know the details.

Maybe there were concerns about the load on the servers, which have to forward MP packets to many destinations? Not sure i can see how putting chat messages into their own MP packets could cause server problems, but it's not an area i have experience in.

I might have a look at detecting if a received MP packet contains a chat message. As well as allowing your patch to work better, this would mean that when replaying recordings that contain multiplayer information, we could distinguish between chat messages that are live, and those that are from the recording.

Thanks,

- Jules
cgdae
 
Posts: 117
Joined: Tue May 31, 2016 8:35 pm

Re: Defensive moderation of MultiPlayer position reports

Postby Johan G » Mon Jul 05, 2021 1:52 pm

One post were split off to the new topic Lag with P3D and FG via FSD and swift client.
Low-level flying — It's all fun and games till someone looses an engine. (Paraphrased from a YouTube video)
Improving the Dassault Mirage F1 (Wiki, Forum, GitLab. Work in slow progress)
Some YouTube videos
Johan G
Moderator
 
Posts: 6629
Joined: Fri Aug 06, 2010 6:33 pm
Location: Sweden
Callsign: SE-JG
IRC name: Johan_G
Version: 2020.3.4
OS: Windows 10, 64 bit

Re: Defensive moderation of MultiPlayer position reports

Postby MariuszXC » Mon Jul 05, 2021 11:07 pm

Richard wrote in Mon Jul 05, 2021 12:04 am:I suspect if we were to fix the packet burst it should probably be done on the server to avoid sending the packets; but this is something for the future.


I may be totally wrong, but I think packet delivery (and even sequence of delivery in UDP) is outside of our scope of control. It is more of a network provider domain, the hardware they use, traffic shaping, QoS and even temporary link overload, all may play a part in what our FG client sees arriving and when (or what does not arrive). IOW: a server may be timing packets perfectly, but this perfection is more than likely lost in transit.

What we can do is employ countermeasures to mitigate some of these issues.

My very imperfect patch was an attempt to address too frequent updates with associated load increase.

Another area which perhaps might use some attention is detection of out-of-sequence packets. A simple Lamport timestamp (in essence an increasing serial number given to every packet sent) would be enough. By my quick calculations, for a default 10Hz update rate, an unsigned 20bit stamp would allow for ~29 hour session before overflowing. 16bits seems a bit low, overflowing every 109 minutes. OTOH if code deals well with overflow, even 8 bits will suffice, so bandwidth hit could be small.
INOP
MariuszXC
 
Posts: 1061
Joined: Tue May 18, 2021 5:38 pm
Location: Europe
Callsign: SP-MRM
Version: 2020.4
OS: Ubuntu 16.04

Re: Defensive moderation of MultiPlayer position reports

Postby amue » Tue Jul 06, 2021 9:54 am

MariuszXC wrote in Mon Jul 05, 2021 11:07 pm:Another area which perhaps might use some attention is detection of out-of-sequence packets. A simple Lamport timestamp (in essence an increasing serial number given to every packet sent) would be enough. By my quick calculations, for a default 10Hz update rate, an unsigned 20bit stamp would allow for ~29 hour session before overflowing. 16bits seems a bit low, overflowing every 109 minutes. OTOH if code deals well with overflow, even 8 bits will suffice, so bandwidth hit could be small.


(Position) MP packets are already time stamped and old/late packets are filtered albeit late in the pipeline in FGAIMultiplayer::update() after the whole packet has been processed in FGMultiplayMgr::ProcessPosMsg() .
Maybe an earlier filtering can be done in FGMultiplayMgr::ProcessPosMsg() when the timestamp is decoded.
amue
 
Posts: 92
Joined: Tue Apr 03, 2018 10:13 am

Re: Defensive moderation of MultiPlayer position reports

Postby cgdae » Tue Jul 06, 2021 10:52 am

amue wrote in Tue Jul 06, 2021 9:54 am:(Position) MP packets are already time stamped and old/late packets are filtered albeit late in the pipeline in FGAIMultiplayer::update() after the whole packet has been processed in FGMultiplayMgr::ProcessPosMsg() .
Maybe an earlier filtering can be done in FGMultiplayMgr::ProcessPosMsg() when the timestamp is decoded.


We could filter early on i guess, though we would want to always let through chat messages, which always requires some parsing of MP packet contents (as discussed earlier in this thread).

However this is only worth doing if parsing MP packets is actually taking a significant amount of time. So before trying to make the fairly significant code changes required, it would be good if someone could profile Flightgear running with many multiplayer aircraft and look for whether FGMultiplayMgr::ProcessPosMsg() is a hot spot?

[And even if FGMultiplayMgr::ProcessPosMsg() is a hot spot, i think we should try to optimise it first before resorting to dropping packets.]

Thanks,

- Jules
cgdae
 
Posts: 117
Joined: Tue May 31, 2016 8:35 pm

Re: Defensive moderation of MultiPlayer position reports

Postby amue » Tue Jul 06, 2021 1:22 pm

cgdae wrote in Tue Jul 06, 2021 10:52 am:We could filter early on i guess, though we would want to always let through chat messages, which always requires some parsing of MP packet contents (as discussed earlier in this thread).

Regarding the problem of dropping chat messages, it doesn't matter if we filter early on or implicitly in FGAIMultiplayer::update() (except that in the latter case a time offset is taken into acount).

However ...
However this is only worth doing if parsing MP packets is actually taking a significant amount of time. So before trying to make the fairly significant code changes required, it would be good if someone could profile Flightgear running with many multiplayer aircraft and look for whether FGMultiplayMgr::ProcessPosMsg() is a hot spot?

[And even if FGMultiplayMgr::ProcessPosMsg() is a hot spot, i think we should try to optimise it first before resorting to dropping packets.

...I also doubt that FGMultiplayMgr::ProcessPosMsg() is the hot spot. But, as you wrote, we need to profile it to be sure.
amue
 
Posts: 92
Joined: Tue Apr 03, 2018 10:13 am

Next

Return to New features

Who is online

Users browsing this forum: No registered users and 6 guests