Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Edited DarkRP.findPlayers to cycle from 1 to # #3250

Merged
merged 3 commits into from
Mar 16, 2024
Merged

Edited DarkRP.findPlayers to cycle from 1 to # #3250

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a8b064
Edited DarkRP.findPlayers to cycle from 1 to #
IP1A Mar 6, 2024
726ae3a
Revert less readable changes and optimize some other calls instead
FPtje Mar 10, 2024
c77c78d
Remove redundant toString
FPtje Mar 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions gamemode/modules/base/sh_util.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,23 +97,25 @@
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

Check warning on line 100 in gamemode/modules/base/sh_util.lua

View workflow job for this annotation

GitHub Actions / Lint / lint

"Whitespace style" 

Style: Please put some whitespace before the operator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Kefta Kefta Mar 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end

for _, PlayerInfo in ipairs(InfoPlayers) do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question with ipairs. The use of ipairs over pairs already is because it's faster

for _ = 1, #InfoPlayers do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change _ to i. _ implies it's totally unused.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're here, cache the return of Player(PlayerInfo) instead of calling it 4 times per iteration.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea 👍

found[Player(PlayerInfo)] = true
players = players or {}
table.insert(players, Player(PlayerInfo))
players[#players+1] = Player(PlayerInfo)

Check warning on line 112 in gamemode/modules/base/sh_util.lua

View workflow job for this annotation

GitHub Actions / Lint / lint

"Whitespace style" 

Style: Please put some whitespace before the operator
end
continue
end

for _, v in ipairs(pls) do
for _ = 1, #pls do
local v = pls[_]
-- Prevend duplicates
if found[v] then continue end

Expand All @@ -125,7 +127,7 @@
(v.SteamName and string.find(string.lower(v:SteamName()), string.lower(tostring(PlayerInfo)), 1, true) ~= nil) then
found[v] = true
players = players or {}
table.insert(players, v)
players[#players+1]=v

Check warning on line 130 in gamemode/modules/base/sh_util.lua

View workflow job for this annotation

GitHub Actions / Lint / lint

"Whitespace style" 

Style: Please put some whitespace before the operator

Check warning on line 130 in gamemode/modules/base/sh_util.lua

View workflow job for this annotation

GitHub Actions / Lint / lint

"Whitespace style" 

Style: Please put some whitespace before the operator
end
end
end
Expand Down
Loading