search

TeamUlysses / ulib

ULib: A Lua library for more rapid development on Garry's Mod servers
http://ulyssesmod.net
Other
104 stars 55 forks source link

removed player.GetAll() #89

Closed HavannaFR closed 7 months ago

HavannaFR commented 1 year ago

Saving invisible players to a local table rather than storing in the player's table and then iterating through all players to see which ones are invisible.

SticklyMan commented 11 months ago

@HavannaFR Could you kindly provide the rationale behind this PR? Are you attempting to improve performance? Or potential issues or exploits with the implementation? As it stands from my point of view (and with very little investigation), I don't see any real benefit in switching this over.

FlorianLeChat commented 11 months ago

This is an optimization because the changes remove the persistent player.GetAll calls in the Think hook for a table of cached players which must be made invisible. I don't know if this will make a big difference, but it will avoid making C -> Lua calls with the former method.

brandonsturgeon commented 11 months ago

I think this might be worse?

It does avoid player.GetAll, that's true, but the tradeoff is that now we're running pairs instead of ipairs, and we're calling IsValid on every entry, every tick.

Have you profiled these changes?

brandonsturgeon commented 11 months ago

Another option could be simply updating a local table of all players on an interval timer.

This could maybe(?) add a small delay to invisible changes, but would vastly reduce the number of player.GetAll calls like you intended.

wrefgtzweve commented 11 months ago

Another option could be simply updating a local table of all players on an interval timer.

This could maybe(?) add a small delay to invisible changes, but would vastly reduce the number of player.GetAll calls like you intended.

https://wiki.facepunch.com/gmod/player.Iterator can do it once the new update releases.

SticklyMan commented 7 months ago

I took the time to profile this PR, our current approach, and the player.Iterator approach on my local dedicated server with me + 31 bots, with all players cloaked. I profiled for 60 seconds, and found the average call times for doInvis() were as follows:

0.07246ms player.GetAll() 0.07249ms Changes from this PR 0.07263ms player.Iterator()

... So, a variance of +/- 0.00014ms. It seems pretty negligible in a real world case, unless there's something I missed. So I will be closing this PR.

For code maintainability (and maybe memory usage), I preferred the player.Iterator approach. So I've implemented that here: https://github.com/TeamUlysses/ulib/commit/5da463795d7ec8e63df4220ab29607dfeb8e83da