reentrancy, r7095

[Tobi]

Code: Select all

Modified: trunk/rts/Game/GameServer.cpp
===================================================================
--- trunk/rts/Game/GameServer.cpp       2008-11-23 19:52:13 UTC (rev 7094)
+++ trunk/rts/Game/GameServer.cpp       2008-11-23 22:18:44 UTC (rev 7095)
@@ -424,14 +424,16 @@
               if (serverframenum > 0) {
                       //send info about the players
                       int curpos=0;
+                       static int ping[MAX_PLAYERS];
+                       static float cpu[MAX_PLAYERS];
                       float refCpu=0.0f;
                       for (unsigned a = 0; a < players.size(); ++a) {
                               if (players[a].myState >= GameParticipant::INGAME) {
                                       Broadcast(CBaseNetProtocol::Get().SendPlayerInfo(a, players[a].cpuUsage, players[a].ping));
                                       if (players[a].cpuUsage > refCpu)
                                               refCpu = players[a].cpuUsage;
-                                       cpuUsages[curpos]=players[a].cpuUsage;
-                                       pings[curpos]=players[a].ping;
+                                       cpu[curpos]=players[a].cpuUsage;
+                                       ping[curpos]=players[a].ping;
                                       ++curpos;
                               }
                       }
@@ -439,15 +441,15 @@
                       medianCpu=0.0f;
                       medianPing=0;
                       if(enforceSpeed && curpos>0) {
-                               std::sort(cpuUsages,cpuUsages+curpos);
-                               std::sort(pings,pings+curpos);
+                               std::sort(cpu,cpu+curpos);
+                               std::sort(ping,ping+curpos);

                               int midpos=curpos/2;
-                               medianCpu=cpuUsages[midpos];
-                               medianPing=pings[midpos];
+                               medianCpu=cpu[midpos];
+                               medianPing=ping[midpos];
                               if(midpos*2==curpos) {
-                                       medianCpu=(medianCpu+cpuUsages[midpos-1])/2.0f;
-                                       medianPing=(medianPing+pings[midpos-1])/2;
+                                       medianCpu=(medianCpu+cpu[midpos-1])/2.0f;
+                                       medianPing=(medianPing+ping[midpos-1])/2;
                               }
                               refCpu=medianCpu;
                       }

Modified: trunk/rts/Game/GameServer.h
===================================================================
--- trunk/rts/Game/GameServer.h 2008-11-23 19:52:13 UTC (rev 7094)
+++ trunk/rts/Game/GameServer.h 2008-11-23 22:18:44 UTC (rev 7095)
@@ -170,9 +170,7 @@
       std::vector<GameParticipant> players;
       boost::scoped_ptr<GameTeam> teams[MAX_TEAMS];

-       float cpuUsages[MAX_PLAYERS];
       float medianCpu;
-       int pings[MAX_PLAYERS];
       int medianPing;
       int enforceSpeed;
       /////////////////// game settings ///////////////////

What about re-entrancy? (Might be nice in multithreaded code, even though it isn't strictly required in this case: but think about possible dedicated server, which hosts multiple CGameServer instances inside it... this code could give the one to make that a HUGE headache)

I've fixed this, but please don't make code pointlessly non re-entrant.

If it was by accident then this can serve as an example for others

[zerver]

I originally put these arrays in the GameServer class. Auswaschbar didn't want them there, so I made them static for maximum speed. I didn't know it was the intention to have multiple GameServers running in the same exe.

Edit: By the way, the initializer isn't needed and just makes it even slower.
int ping[MAX_PLAYERS] = { 0 };
float cpu[MAX_PLAYERS] = { 0.0f };

[Tobi]

I originally put these arrays in the GameServer class. Auswaschbar didn't want them there, so I made them static for maximum speed. I didn't know it was the intention to have multiple GameServers running in the same exe.

Edit: By the way, the initializer isn't needed and just makes it even slower.
int ping[MAX_PLAYERS] = { 0 };
float cpu[MAX_PLAYERS] = { 0.0f };

Whether or not this is the intention, it is bad practice to make code in this non-obvious way non-reentrant. Who knows which refactor / use case will require and assume the function is re-entrant, and then be plagued by weird random bugs because it isn't...

It's a typical case of premature optimization being the root of all evil.

(That code is being run once every 2 seconds. That means it's doing a whopping single instruction to allocate 256 bytes stack space for the 2 arrays per 2 seconds... And by initializing it we are wasting a whopping 128 bytes per second L1 cache bandwidth... Must be the bottleneck of engine... IOW, please make 99% of the code more readable and maintainable, and only make the 1% of code that's really a bottleneck faster.)

You can remove initializer tho if you want, I had already committed it before I noticed it technically wasn't needed.

[zerver]

You are absolutely right, although if all the non-performance-critical code is inefficient, it can become performance-critical

[Columbus]

LOL
spam alert!