Edited DarkRP.findPlayers to cycle from 1 to # #3250
Conversation
gamemode/modules/base/sh_util.lua
Outdated
| @@ -97,23 +97,25 @@ function DarkRP.findPlayers(info) | |||
| local InfoPlayers = {} | |||
| for A in string.gmatch(info .. ";", "([a-zA-Z0-9:_.]*)[;(,%s)%c]") do | |||
| if A ~= "" then | |||
| table.insert(InfoPlayers, A) | |||
| InfoPlayers[#InfoPlayers+1] = A | |||
There was a problem hiding this comment.
If you really want to optimise this, create an InfoPlayersCount variable and increment is inside the loop after each insertion so that the length doesn't have to be refetched on each iteration.
There was a problem hiding this comment.
Is table.insert really that slow that we should be avoiding it and doing this instead? At some point I prefer just using table.insert over doing manual accounting and accept the tiny cost it brings.
There was a problem hiding this comment.
See https://gitspartv.github.io/LuaJIT-Benchmarks/#test12 - this is a bit different of a case in application from the other non-tinsert examples since the local count will be reused for a later loop, but table.insert is still the slowest when the code is jitted. I don't think the readability is sacrificed by not using table.insert personally so long as the variables are well-named and there are no regressions which can be verified with a simple unit test of a function like this.
There was a problem hiding this comment.
Look at the code though. This code splits the input string on semicolons and iterates over each element. I'm quite sure that this regex based split would already make the cost of table.insert completely invisible. On top of that, use of this semicolons syntax is extremely rare, so for most calls by far this is going to be a single iteration loop.
In DarkRP this function is only called for some chat commands. That's extremely rare performance wise. Let's not go to extreme lengths trying to optimize this function. It can only introduce bugs, make it less readable and for absolutely zero real world visible gain in performance.
gamemode/modules/base/sh_util.lua
Outdated
| end | ||
| end | ||
|
|
||
| for _, PlayerInfo in ipairs(InfoPlayers) do | ||
| for _ = 1, #InfoPlayers do |
There was a problem hiding this comment.
Change _ to i. _ implies it's totally unused.
gamemode/modules/base/sh_util.lua
Outdated
| end | ||
| end | ||
|
|
||
| for _, PlayerInfo in ipairs(InfoPlayers) do | ||
| for _ = 1, #InfoPlayers do | ||
| local PlayerInfo = InfoPlayers[_] | ||
| -- Playerinfo is always to be treated as UserID when it's a number | ||
| -- otherwise people with numbers in their names could get confused with UserID's of other players | ||
| if tonumber(PlayerInfo) then | ||
| if IsValid(Player(PlayerInfo)) and not found[Player(PlayerInfo)] then |
There was a problem hiding this comment.
While you're here, cache the return of Player(PlayerInfo) instead of calling it 4 times per iteration.
|
I would also rename the PR to something that makes it clear this is an optimisation and not functionality changing. |
There was a problem hiding this comment.
Thanks for the optimizations!
Some of those changes might have a tiny tiny improvement in performance, but they also make things less readable: using ipairs and table.insert is nicer to read than manipulating tables manually. Since in this case I prefer readability, I reverted those particular changes.
@Kefta had a good idea about reducing the amount of calls to Player(). I've added some other optimizations as well.
The trick to optimizing functions like this is measurement. Grab your lua_run command, and use SysTime() before and after to measure the time for that function to do its work. Do that a couple of times to get an idea of its variation. Then go out to optimize, and keep measuring to see what the difference makes.
You can even temporarily put SysTime() calls in the code, to see which parts are the most expensive. That way you can hone in on the specific bottleneck.
gamemode/modules/base/sh_util.lua
Outdated
| end | ||
| end | ||
|
|
||
| for _, PlayerInfo in ipairs(InfoPlayers) do |
There was a problem hiding this comment.
Same question with ipairs. The use of ipairs over pairs already is because it's faster
gamemode/modules/base/sh_util.lua
Outdated
| end | ||
| continue | ||
| end | ||
|
|
||
| local stringPlayerInfo = string.lower(tostring(PlayerInfo)) |
There was a problem hiding this comment.
This data was being converted to lowercase twice for every player in the loop below. If you have 128 players (with no found player), it would do this string conversion 128 times! Take it out of the loop, and it's done only once for every player.
| if found[v] then continue end | ||
| local steamId = v:SteamID() |
There was a problem hiding this comment.
SteamID was called twice per iteration, changing it to once and storing in a variable ain't much, but it also looks a bit nicer in the already long if-statement below.
|
This is often unreliable since measuring can actually affect whether jitting happens or not. It's better to measure idioms in a LuaJIT interpreter and see what uses the least instructions/takes the least time in a normal benchmark, or if done in gmod, what the timing is from an external module which won't invoke Lua->C/C->Lua calls during the code execution itself. SysTime benchmarking is a great tool for the glua api where there are a lot of C funcs where stitching won't help, but for pure Lua idioms, it's best to do that outside of glua. The LuaJIT version per-branch is known so its efficient practices can be known without gmod. |
That is better when you're trying to find out which Lua language construct will get you that last drop of performance in a vacuum chamber experiment. That's not what we're doing here, though. Here there is a real function, with C calls, with rather complex behavior, and with some reasoning about its call sites and most common input. Without measuring, your best recourse is eyeballing and meme based optimizing: changing the things in code that you recognize from your tribal knowledge of "don't do this, do this instead". Without measuring, you might miss what the real bottleneck is in the code. I'm happy with the contribution, but I dread the discussions on those micro optimizations. Did anyone even establish that this function causes performance problems? I'm sure you can prove a performance increase when you call it a million times, but it's called once per chat command in DarkRP. What performance increase are real people going to notice? |
|
One tiny more change to remove a This PR is good to go. Thanks for the optimization! |
No description provided.