Major optimization to start round and WEPS.ForcePrecache #1168

Merged
merged 2 commits into from May 8, 2016

Projects

None yet

9 participants

@gspetrou
Contributor
gspetrou commented May 1, 2016 edited

Moved WEPS.ForcePrecache to GM.InitPostEntity, switched it's loop from pairs to
ipairs because it is faster and weapons.GetList is sequential, combined two duplicate if statements, and fixed a syntactical issue that really bothered me.

Overall made BeginRound() go from taking about 0.80s to execute to 0.27s. This practically removed the little lag at the start of each round.

@gspetrou gspetrou Major optimization to start round and WEPS.ForcePrecache
Moved WEPS.ForcePrecache to GM.InitPostEntity, switched it's loop to
ipairs, combined an two duplicate if statements, fixed another
syntactical issue that really bothered me.

Overall made BeginRound() go from about 0.80s to execute to 0.27s.
45c445d
@Kefta
Contributor
Kefta commented May 1, 2016

Test with a flat loop (i = 1, #tbl). LuaJIT probably already takes care of this, but I wanna make sure

@bmwalters
Contributor
bmwalters commented May 1, 2016 edited

@Kefta who tf calls that a "flat loop"? (according to the Lua PIL for var = a, b, c syntax is a "numeric for" and for var in blah syntax is a "generic for")

@robotboy655 robotboy655 added the TTT label May 1, 2016
@gspetrou
Contributor
gspetrou commented May 1, 2016 edited

@Kefta Numeric was barely faster, here are the results if you're curious, though they aren't really that special.

Profiler Results

-- ipairs
function WEPS.ForcePrecache()
   for k, w in ipairs(weapons.GetList()) do
      if w then
         if w.WorldModel then
            util.PrecacheModel(w.WorldModel)
         end
         if w.ViewModel then
            util.PrecacheModel(w.ViewModel)
         end
      end
   end
end

-- Numeric
function WEPS.ForcePrecache()
   local weplist = weapons.GetList()
   for i = 1, #weplist do
      local w = weplist[i]
      if w.WorldModel then
         util.PrecacheModel(w.WorldModel)
      end
      if w.ViewModel then
         util.PrecacheModel(w.ViewModel)
      end
   end
end
@Kefta
Contributor
Kefta commented May 2, 2016 edited

Thanks for testing, up to @svdm if he wants to switch or keep the style consistent

@Kefta
Contributor
Kefta commented May 2, 2016

@zerfgog That's what I saw that loop labelled as when I first read it in code -- thanks for the correction

@Bo98
Contributor
Bo98 commented May 2, 2016

from pairs to ipairs because it is faster

A few weeks ago I was told the opposite was true. Is it inconsistent for everyone or something?

@Bo98
Contributor
Bo98 commented May 2, 2016

Also, this was already suggested before and rejected due to an information leak issue: #1031.

@bigdogmat
bigdogmat commented May 2, 2016 edited

A few weeks ago I was told the opposite was true. Is it inconsistent for everyone or something?

I can't do the testing at the moment, but from what I remember pairs is only faster on arrays with hundreds of thousands of elements

EDIT: Correction, that testing was run when we didn't have LuaJIT, and was done with 15k elements. Though even then the different between the 2 was less than a micro second.

The appeal for ipairs comes from the fact that it does a sequential run of the array and it's the preferred(official?) method for iterating over a sequential array.

@gspetrou
Contributor
gspetrou commented May 2, 2016

That doesn't really make sense though. If I recall correctly the weapons cache isn't reset at the start of every round. Once a weapon is cached it is cached till either a map change happens or the server runs the flush command.

@aStonedPenguin
aStonedPenguin commented May 2, 2016 edited

A few weeks ago I was told the opposite was true. Is it inconsistent for everyone or something?

Before lua jit it was with larger loops. There was an old thread on facepunch people continually reference and spread misinformation with. It's been proven many times ipairs is faster then pairs and a for loop is as fast as ipairs and sometimes faster.

@Bo98
Contributor
Bo98 commented May 2, 2016

Yeah that's what I thought. IIRC ipairs is JIT compiled while pairs isn't.

@gspetrou
Contributor
gspetrou commented May 2, 2016 edited

I forgot to localize ipairs in my ipairs test so here it is again
Corrected ipairs test

Verdict (this is only after one test for each btw)
ipairs = 0.0098702137989903 seconds
numeric = 0.0099555470626456 seconds

Though can we get back on track, this would be a major optimization to the gamemode. If this exploitable command is "cl_precacheinfo" moving where this function call doesn't make a difference. Though I'm not sure what console command is being referred to in the comments of the function.

Oh and @Kefta, I made sure to keep the code styling consistent with the rest.

@TheEMP
TheEMP commented May 2, 2016

@gspetrou Read #1031 it outlines how this pull request could create exploitable behavior.

@gspetrou
Contributor
gspetrou commented May 2, 2016

@TheEMP I have tried using "developer 1". This command does not display when a model has been cached/precached. All it seems to do it display the console on the top left of the screen. I can't seem to find any other way than the command I mentioned previously to see what models are being cached.

@robotboy655
Collaborator

Precache all weapon models on first player join clientside = profit?

@bigdogmat bigdogmat commented on an outdated diff May 2, 2016
garrysmod/gamemodes/terrortown/gamemode/weaponry.lua
@@ -486,12 +486,14 @@ end
-- non-cheat developer commands can reveal precaching the first time equipment
-- is bought, so trigger it at the start of a round instead
function WEPS.ForcePrecache()
- for k, w in pairs(weapons.GetList()) do
- if w and w.WorldModel then
- util.PrecacheModel(w.WorldModel)
- end
- if w and w.ViewModel then
- util.PrecacheModel(w.ViewModel)
+ for k, w in ipairs(weapons.GetList()) do
+ if w then
@bigdogmat
bigdogmat May 2, 2016

Why would the state of w even need to be checked in this case, ipairs is gonna return when it hits the first nil value anyway

@gspetrou
Contributor
gspetrou commented May 3, 2016

@robotboy655 Doesn't seem like this would be necessary. The "model_list", "cl_precacheinfo", and "listmodels" commands didn't leak any info when precaching once serverside at InitPostEntity. This is after testing through multiple in-game rounds.

@gspetrou gspetrou Removed unnecessary if statement
ipairs will break out of the loop once it hits the first nil value. There is no reason for an extra if statement to check that. @bigdogmat
fa718bd
@robotboy655
Collaborator

@gspetrou I am going to assume you tested it on local mutliplayer or singleplayer, where the serverside precaching also forced your client to precache the models. Try starting an srcds instance and testing then.

@gspetrou
Contributor
gspetrou commented May 3, 2016

@Robotboy655 this was all tested on a dedicated server.

@svdm
Collaborator
svdm commented May 4, 2016

I'll try and test this myself later this week.

@svdm svdm merged commit 1511fc1 into garrynewman:master May 8, 2016
@svdm
Collaborator
svdm commented May 8, 2016

From my testing on my local dedicated server, this does indeed do the job. I'd guess that a list of models that have been precached so far is sent from server to clients when they join, meaning a single precache serverside is sufficient to cause all clients to precache even if they join later on.

@Kefta
Contributor
Kefta commented May 8, 2016

That's good to know; I'll have to implement this into my weapons as well. Thanks @svdm @gspetrou

@gspetrou
Contributor
gspetrou commented May 8, 2016

Hip hip, hooray!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment