Possible ways to make this code more efficient?

Help and Feedback Scripting Support
#1

Using non-humanoid NPC’s, I tween the rig to the value of ServerCFrame, just wondering if there is anything else I can tweak to make it more efficient as this is a vital part of my game.

RunService Controller:

local mobs = collectionService:GetTagged('mobs')

local FRAME_LIMIT_MOBS = 1/30
local FRAME_LIMIT_PROJ = 1/30

local projectilesTbl = {}
local mobsTbl: {Instance} = {}

local render = nil
local t: number = 0


local function callRender()
	render = runService.PreRender:Connect(function(dt)
		if t > FRAME_LIMIT_MOBS then
			for pos: number = 1, #mobsTbl do 
				mobModule:render(mobsTbl[pos], dt)
			end
			t = 0
		else 
			t += dt
		end
		if #mobsTbl == 0 then
			render:Disconnect()
			render = nil
		end
	end)
end

for _, v in pairs(mobs) do 
	newMob(v)
end
collectionService:GetInstanceAddedSignal('mobs'):Connect(newMob)
collectionService:GetInstanceRemovedSignal('mobs'):Connect(function(mob)
	local find = table.find(mobsTbl, mob)
	if find then
		table.remove(mobsTbl, find)
	end
	
	mobModule:removeCachedMob(mob)
end)

Mob Module:

local FRAME_LIMIT_MOBS = 1/30
local TWEEN_STYLE_MOBS = TweenInfo.new(FRAME_LIMIT_MOBS*3, Enum.EasingStyle.Linear, Enum.EasingDirection.InOut)

local mobs = {}
local mobCache: {Instance} = {}

function mobs:removeCachedMob(mob)
	if mobCache[mob] then
		mobCache[mob] = nil
	end
end

local function isValidMob(mob)
	if not mobCache[mob] then
		mobCache[mob] = {
			HumanoidRootPart = mob:FindFirstChild("HumanoidRootPart"),
			ServerPart = mob:FindFirstChild('ServerPart'),
			Health = mob:FindFirstChild('Health'),
			ServerCFrame = mob:FindFirstChild('ServerCFrame')
		}
	end
	local cache = mobCache[mob]
	return mob and mob.Parent and cache.HumanoidRootPart and 
		cache.ServerPart and cache.Health and cache.Health.Value > 0 and 
		cache.HumanoidRootPart.CFrame ~= cache.ServerCFrame.Value
end


local function updateMobPosition(mob)
	tweenService:Create(
		mobCache[mob].HumanoidRootPart,
		TWEEN_STYLE_MOBS,
		{CFrame = mobCache[mob].ServerCFrame.Value}
	):Play()
end

function mobs:render(mob, dt)
	if isValidMob(mob) then
		updateMobPosition(mob)
	end
end
2 Likes
#2

PLEASE CHECK if these changes I suggested will work because I only skimmed through your code.

There’s no reason why you would do this instead of just iterating the table. In Luau, generalized iteration is faster than numerical loop + indexing.

You could just be doing this instead:

for pos, mob in mobsTbl do
    mobModule:render(mob, dt)
end

Assuming the table is an array with no gaps, you can avoid using the len operator for this specific usecase. You can just index for the first element and see if it’s nil; that’s how you know if the table is empty. Normally, it won’t be a problem in Luau because table length is cached as the table changes, but it’s not a guarantee.

if mobsTbl[1] == nil then
    render:Disconnect()
    render = nil
end

Once again, GI is better. You don’t need pairs(). This is a microoptimization but does make it more efficient like you wanted.

It might be possible to restructure this code here to use hashmaps instead. table.find uses a linear search with O(n) time complexity, and table.remove will have to reorganize the table to close the gap. All of these can affect performance, so if you figure out a way to use hashmaps, you can avoid these.

One idea is to turn mobsTbl itself into a hashmap with the format {[Instance]: true}. If you make this change, you will have to change how the table is used:

Looping the table will now only use the index key:

for mob in mobsTbl do
    mobModule:render(mob, dt)
end

and checking if the table is empty can now be done using the next() iterator:

if next(mobsTbl) == nil then
    render:Disconnect()
    render = nil
end

and finally, removing the mobs in the CollectionService connection can be simplified:

collectionService:GetInstanceRemovedSignal('mobs'):Connect(function(mob)
	mobsTbl[mob] = nil
	mobModule:removeCachedMob(mob)
end)

And, it’s not inside the code you provided, but adding mobs to mobsTbl uses the similar format where you set the key (the mob) to true.

mobsTbl[mob] = true

Inside your module, there’s one thing you can do:

Using the colon operator is completely unnecessary. You are passing an additional parameter to the function call for no reason which can affect the performance. Once again, this is a microoptimization.

#3

This line is really messy. Why are you doing 7 checks in a single line, if you have a function (the one it is in) whose whole purpose is to check if the mob is valid?

This thread is about inefficiency, presumably from a technical perspective, but this line is inefficient from a fundamental perspective, because combined code like this impedes readability and makes it really easy for a typo to slip in, break your game, and be really hard to spot (because it takes significant time to step back and just understand what the line is doing)

If the point is to return false if the mob is not valid (and return the mob if it is valid), I would recommend just splitting up your code and evaluating each condition one at a time.

Example 
if not A then
    return false
end

if (B > C) then
    return false
end

if (D ~= E) then
    return false
end

return mob

This makes it way easier to visualize each check that you are subjecting mob to and also allows you to segment checks (via comments), debug specific checks, etc. versus dealing with one line that ‘does it all’.

#4

Thank you, will be looking into adding these in now

#5

I mean I only ever run this function once it will never change so if there is a typo then I’ll just fix it and then move on, though yea I agree I guess I could change it up for visualization purposes

#6

Don’t know if this is real but with one mob it went from .15% usage to 0% lol, thanks so much

#7

Managed to remove some redundant checks. thanks

#8

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.