Optimisation pass

[TW1STaL1CKY]

I figured I'd continue on after #1364 and look at replacing all player.GetAll calls where applicable, which led into replacing all ents.GetAll calls with ents.Iterator. That brings us to now where I figured I'd keep going with every optimisation method I could think of within reason.

Instead of pushing this straight to develop, I've made this PR to give everyone interested a chance to check I haven't missed anything obvious. I've tried usual PAC usage on my own game with these changes and it seems fine. If there's no objections, I'll merge this within a few days.

Commit description:

Had a scanthrough and made the following changes in the name of optimisation (even micro-optimisations) where possible:

• player.Get* -> player.Iterator
• ents.GetAll -> ents.Iterator
• ipairs -> for i=1,#tbl (only main uses of ipairs has been replaced, not every use)
• Vector:Distance -> Vector:DistToSqr
• Vector:Length -> Vector:LengthSqr
• net.Read/WriteEntity -> net.Read/WritePlayer where it is definitely intended to be a player
• Saved some GetConVar calls to local vars
• Localised/reused some Vector and Color objects
• Localised some functions where they are frequently used in a few files
• Misc minor styling cleanup (mostly spacing)
• BONUS: Fixed pac.ColorToNames being incorrect when used by some parts (some now use the new pac.VectorColorToNames func)

[thegrb93]

I feel like for loops that don't need optimization should use ipairs (which is faster in luajit). Each for loop now being 3 lines is pretty messy and uncomfortable to look at.

[TW1STaL1CKY]

I agree it's nicer to look at when it's just ipairs. However when I test the run times between them, for i = 1, #tbl is consistently the fastest, even when luajit is enabled.

Results:

pairs		0.061870000008639
ipairs		0.023996999989322
for k = 1, #tbl	0.0083710000012616

The code I'm using to test the run times:

local tbl = {"a", "b", "c", "a", "b", "c", "a", "b"}

jit.on() -- Make sure its on

local runTimes = 100

local time = 0
for x = 1, runTimes do
	local start = SysTime()
	
	for i = 1, 1000 do
		for k, v in pairs(tbl) do end
	end
	
	local e = SysTime()
	
	time = time + (e - start)
end

print("pairs", (time / runTimes) * 1000)

time = 0
for x = 1, runTimes do
	local start = SysTime()
	
	for i = 1, 1000 do
		for k, v in ipairs(tbl) do end
	end
	
	local e = SysTime()
	
	time = time + (e - start)
end

print("ipairs", (time / runTimes) * 1000)

time = 0
for x = 1, runTimes do
	local start = SysTime()
	
	for i = 1, 1000 do
		for k = 1, #tbl do
			local v = tbl[k]
		end
	end
	
	local e = SysTime()
	
	time = time + (e - start)
end

print("for k = 1, #tbl", (time / runTimes) * 1000)

[thegrb93]

That makes me really sad for glua. Still, compromising clean code in spots that don't need the performance is a bit of a headache. I'm fine with the changes either way.

[CapsAdmin]

I don't wanna dictate too much here as I'm not very active in the project at the moment, but how does this affect startup time, framerate of rendering a complex outfit, and so on?

In other words, if it doesn't improve the performance users care about then all we're really doing is making the code more complex.

[Astralcircle]

I don't wanna dictate too much here as I'm not very active in the project at the moment, but how does this affect startup time, framerate of rendering a complex outfit, and so on?

In other words, if it doesn't improve the performance users care about then all we're really doing is making the code more complex.

Of most significant, these changes can reduce the number of calls to player.GetAll() and ents.GetAll(), for i=1,#tbl can go through the table faster. Given that in some cases ents.GetAll() and player.Iterator() are called in loops, this can greatly increase performance in them

[thegrb93]

That's stating the obvious. He just wants to know if the fps gain is worth the messy code. Imo only the hot loops should be optimized, but it doesn't really matter.

[pingu7867]

Having to resolve the conflicts in that many files changed will be an absolute pain for me who hoards heaps and heaps of changes before publishing them, but I'll manage.

As for the style, yeah it adds some lines and it's a different way but I'll accept it if it makes pac a little less bulky in the crucial areas that need their inevitable loops. Especially Think and OnDraw ones, that's good to optimize. Even if ultimately there's always a bigger source of cost out there. How much of pac's runtime cost are we fighting over? If it turns out to be less than a percent, fighting over a tiny fraction is useless in my opinion. Just don't make the code unreadable. Which these changes don't.

If we're gonna talk about performance, I'm more interested in numbers and quantifying the impact.

Of most significant, these changes can reduce the number of calls to player.GetAll() and ents.GetAll(), for i=1,#tbl can go through the table faster. Given that in some cases ents.GetAll() and player.Iterator() are called in loops, this can greatly increase performance in them

Neither the word "can" nor "greatly increase performance" don't mean anything by themselves. For any theoretical "can", you have to show the real truth. I can trust your intent but it's not enough to establish. For any "greatly increase", you must specify how much. What's the run time in milliseconds (or microseconds) before, and what's the runtime after? What's the FPS before and after? If you have numbers, I'm sure they'll speak for themselves.

[Astralcircle]

That's stating the obvious. He just wants to know if the fps gain is worth the messy code. Imo only the hot loops should be optimized, but it doesn't really matter.

Perhaps I didn’t express myself clearly enough. What I meant was that it could improve performance in certain situations. For example, if used on a larger server with a bunch of complex PAC3 outfits, the difference would definitely be noticeable. However, if you were to scrutinize it under a magnifying glass in single-player, there would hardly be any noticeable change.

About complicating the code, I don’t think that anything can complicate it except for k = 1, #tbl, because the rest of the changes simply consist of using a different function

[thegrb93]

I finished reviewing. Just a couple more suggestions. I think a big potential performance improvement is minimizing __index calls on entities.

[wrefgtzweve]

ive seen people complain that player.Iterator ents.Iterator can rarely return NULL entities on clientside, im not exactly how it works and how to fix it but its enough of a issue that i can recommend not using it clientside when possible

[wrefgtzweve]

I finished reviewing. Just a couple more suggestions. I think a big potential performance improvement is minimizing __index calls on entities.

Yeah ent, weapon, player __index calls are significantly laggy. Running something like fprofiler and a __index counter would probably help narrow down the lag. Small optimization passes like these arent bad but they wont make significiant differences

[pingu7867]

I've had some updates I've been meaning to push for a while and had decided to wait. But it's been a month, which is a far cry from what the "within a few days" initially stated. So I'm doing some of my updates and you can deal with the git conflicts later.

[TW1STaL1CKY]

I've had some updates I've been meaning to push for a while and had decided to wait. But it's been a month, which is a far cry from what the "within a few days" initially stated. So I'm doing some of my updates and you can deal with the git conflicts later.

That's fine go ahead. I may exclude my changes to do with combat related things for the time being anyway.

And sorry, there were a lot of things pointed out that I could try improving, so I kept this back to keep working on it.

[thegrb93]

This PR is nearly unreviewable with the amount of changes. You're going to just have to test it on a server probably.

[thegrb93]

Conflicts need to be resolved too.