Module optimization, any feedback to what I should add or change?

The code I will have below is a module library called SLib. It’s supposed to be a small library that helps with small things. Overall I think the Events section needs more optimization.
SLib - Roblox - Whole module

local rem = {}
local rs = game:GetService("RunService")
local mainfold
if rs:IsServer()then
	if not game.ReplicatedStorage:FindFirstChild("SLibFolder") then
		local fold = Instance.new("Folder",game.ReplicatedStorage)
		fold.Name = "SLibFolder"
		mainfold = fold
	else
		mainfold = game.ReplicatedStorage.SLibFolder
	end
else
	mainfold = game.ReplicatedStorage:WaitForChild("SLibFolder")
end
function rem.remote(name)
	local event
	local type
	local connectmethod
	if rs:IsServer()then
		local par
		if not mainfold:FindFirstChild("SLibRemoteEvents") then
			local fold = Instance.new("Folder",mainfold)
			fold.Name = "SLibRemoteEvents"
			par = fold
		else
			par = mainfold:FindFirstChild("SLibRemoteEvents")
		end
		event = Instance.new("RemoteEvent",par)
		event.Name = name
		connectmethod = "OnServerEvent"
		type = "serv"
	elseif rs:IsClient() then
		if not mainfold:FindFirstChild("SLibRemoteEvents") then
			warn("Unable to find SLibRemoteEvents")
		else
			event = mainfold.SLibRemoteEvents:WaitForChild(name)
		end
		connectmethod = "OnClientEvent"
		type = "client"
	end
	local remote = {
		_connections = {};
		Event = event;
		Connect = function(self,func)
			table.insert(self._connections,event[connectmethod]:Connect(func))
		end,
		Fire = function(self,...)
			if type == "client"then
				event:FireServer(...)
			elseif type == "serv"then
				event:FireAllClients(...)
			end
		end;
		FireTo = function(self,...)
			if type == "serv"then
				local args = {...}
				local otherargs = {}
				for i=2,#args do
					table.insert(otherargs,args[i])
				end
				event:FireClient(args[1],unpack(otherargs))
			end
		end,
		Destroy = function(self)
			for _,v in ipairs(self._connections)do
				v:Disconnect()
			end
			event:Destroy()
		end,
	}
	return remote
end
function rem.func(name)
	local event
	local type
	local connectmethod
	if rs:IsServer()then
		local par
		if not mainfold:FindFirstChild("SLibRemoteFunctions") then
			local fold = Instance.new("Folder",mainfold)
			fold.Name = "SLibRemoteFunctions"
			par = fold
		else
			par = mainfold:FindFirstChild("SLibRemoteFunctions")
		end
		event = Instance.new("RemoteFunction",par)
		event.Name = name
		connectmethod = "OnServerInvoke"
		type = "serv"
	elseif rs:IsClient() then
		if not mainfold:FindFirstChild("SLibRemoteFunctions") then
			warn("Unable to find SLibRemoteFunctions")
		else
			event = mainfold.SLibRemoteFunctions:WaitForChild(name)
		end
		connectmethod = "OnClientInvoke"
		type = "client"
	end
	local remote = {
		_connections = {};
		Event = event;
		Connect = function(self,func)
			table.insert(self._connections,func)
		end,
		Fire = function(self,...)
			if type == "client"then
				return event:InvokeServer(...)
			elseif type == "serv"then
				return event:InvokeClient(...)
			end
		end;
		Destroy = function()
			event:Destroy()
		end,
	}
	event[connectmethod] = function()
		for _,v in ipairs(remote._connections)do
			return coroutine.wrap(v)()
		end
	end -- how 2 disconnect?
	return remote
end

return rem

^ The module I mostly want optimized
I just want to know if there is any way I could optimize this module. Thanks.
PS: I have been thinking of making my code more tidy and cleaning it up a bit but I just don’t know where to start.
How the code above works is that it’s sort of a shortcut for events. Example:

local cash = slib.Events.remote("AddCash")

cash:Connect(function(player,amount)
print(player.Name.." added "..amount.." cash!")
end)

Client:

local cash = slib.Events.remote("AddCash") -- gets cash remote

cash:Fire(10000)

This can be pretty useful as it saves up fuss when making remote events like this

why do i need to download your code


maybe remove the first comment

Comments, as far as I know, don’t impact script performance.
And you have to download my code because its multiple modules in one.
Edit: i changed it to a model

@Qin2007
Yep, comments don’t impact performance at all. As far as the engine is concerned, they don’t even exist, they’re omitted completely from the script’s bytecode.

The one thing I would immediately recommend with the code you included in your post is using shared metatables. So, Connect, Fire, Destroy, etc all use the same functions and every remote object uses the same metatable. The connections upvalue becomes self.Connections and is created on the remote object.

This would speed up object creations, and reduce the memory usage per object by a decent bit.

Additionally, you are using a pairs loop for the connections table, however I would recommend using ipairs since its an array table (it’s not a real performance gain, but it is best practice). I would also recommend omitting the i variable as _, and naming the v variable so it is more clear when read.

Additionally, you don’t actually have a way to disconnect connections, and your connections table holds on to those connection functions. That’s basically a memory leak right there, and, is not something you want. You definitely want to be able to disconnect connections so that they won’t sit in memory even after they’re no longer used.

1 Like

Thanks for this advice, but I have one question.
For the remote events, I already disconnect then when you use :Destroy.

		for _,v in ipairs(self._connections)do
			v:Disconnect()
		end
           event:Destroy()

But, how would I implement this with remote functions?

	local remote = {
		_connections = {};
		Event = event;
		Connect = function(self,func)
			table.insert(self._connections,func)
		end,
		Fire = function(self,...)
			if type == "client"then
				return event:InvokeServer(...)
			elseif type == "serv"then
				return event:InvokeClient(...)
			end
		end;
		Destroy = function()
			event:Destroy()
		end,
	}
	event[connectmethod] = function()
		for _,v in ipairs(remote._connections)do
			return coroutine.wrap(v)()
		end
	end -- how 2 disconnect?

Is there anyway I can disconnect the stuff above?

Also, how would you fully remove the table? Do I just make self = nil when I use :Destroy?

And how would I make a shared metatable?

If I’m correct, can’t you just do for i = 1,1 for doing something once?

If I’m brainfarting, it may actually be for i = 0,1 or maybe even = 1,0 lol

Actually, imagine if it’s just i = 1…

:wink:

for Count = 1, 1 do

end

this should only run once

Here’s a little tip for a budding scripter like yourself, a useful tactic of mine for initiation of declarations happens to be something like this;

local Counts = nil;
(You can also use “local Counts” if you dont mind the visible false positive in type checking)

That’ll further futureproof your code :slight_smile:

I think this is getting off topic, can you help me now?

I’m a little confused. What do you mean by this/what is this in reference to?

If you want to do something once, you don’t need a loop, it doesn’t make sense to have one, so why wouldn’t you just use a do end block or something?

If you really want a loop that only runs once this is better since it doesn’t allocate any extra local variables:

repeat
	-- stuff
until true

(Fun little side fact: you can use repeat until loops with continue and break to do some interesting control flow stuff that you can’t with any other lua loop since the check happens at the end)

@ScandalousSolution

I think I missed the event code you mentioned, but, for your functions code you could use the index of the function, then you can remove the function at that index from the table to disconnect it. With typically, with regular RemoteFunction objects, there can only be one single callback, every time you set OnServerInvoke the callback gets fully overriden which is why this doesn’t have any real effect in regular Roblox code.

You don’t really need to, the garbage collector will collect the table as long as it isn’t in any active variables somewhere.

This is typically how you’ll see OOP code done especially lately because of how luau optimizes method calls:

local ExampleClass = {}
ExampleClass.__index = ExampleClass

-- Some default constant value
-- Tables wouldn't work since they're unique each time you create one, only stuff that doesn't change
ExampleClass.Abc = 123

-- Example
function ExampleClass:DoSomething(abc)
	self.Abc = abc
end

function ExampleClass:Destroy()
	-- destroy stuff
end

-- Later when an object is getting created
setmetatable({SomeTable = {}}, ExampleClass)

Some people like to define the constructor inside the metatable, for example as ExampleClass.new, but personally I like to have a separate table, kinda like this:

local Example = {
	Class = ExampleClass -- Sometimes I might define this below so rather than using ExampleClass I use Example.Class
}

function Example.new()
	-- Create an instance of the Example class and return it, making tables or including values
	return setmetatable({SomeTable = {}}, Example.Class)
end

But, it’s entirely preference in that case.

I loop through connections so you can use :Connect multiple times for each event. But, tbh, that probably wont even happen.

event:Connect(function() print("hi") end)
event:Connect(function() print("how are you?") end)

Also, for the metatables, you mean doing something like this?

local events = {}
events.__index = events
function events.remote(name)
local self = {
Connect stuff blah blah
}
return setmetatable(events,self)
end

I’m still learning metatables and I only know a little of OOP.

And finally, for the remote functions, I just do this?

table.clear(self._connections)

You should read the docs for table.clear, it’ll actually make the problem worse, not better (it basically sets all the values in the table to nil but doesn’t allow the table space to deallocate, which is bad)

The important thing to note is that the functions are defined on the __index table, not when the constructor is called. By defining the functions where the constructor is called, you basically create new function references every time, which, is not as good for performance/memory.

It also is faster for method calls (e.g. Event:Connect()) which you can find more info on on the luau page I linked.

Yes, this is good, and that’s usually how you want events to work. Events get connected by outside code a lot (but not as much inside, it depends).

One of your sections, I don’t remember which, inserts function callbacks into a table, but, they don’t get cleaned up ever, and there’s no sort of disconnect behaviour either, and that’s what I was talking about before.

For remote functions, typically you only have one callback, because, well, one of the main reasons of a remote function is to be able to return something. It doesn’t really make sense to have multiple remote function callbacks since events already work that way, and, that defeats being able to return from them.

Ohhhhh. That makes sense now. I’m probably going to rewrite the module code cause the current one is messy. Thank you!

EDIT: Rewrote the module, it’s so much quicker and the lines were reduced a lot.
Orignal module size: 86 lines
New Module size: 56 lines

1 Like