Yup. I’ll be forking GoodSignal most likely, and just renaming DisconnectAll() to Destroy so it fits my interface.
Disconnect will also need a Destroy method, but no big deal.
There was discussion above about the linked list implementation, but I’m curious why you used a linked list in the first place. Is it simply that it’s faster than table insertion, deletion, etc.?
The linked list has very good iteration stability while also being fast.
If you delete an entry out of an array, the index of each entry after it shifts. That means that any for loops that are in the middle of iterating over it may have their behavior messed up.
The SimpleSignal implementation solves this by copying the handler list and iterating over the copy instead.
You could also solve it by using a copy on write list of handlers, where you copy the handler list before you add or remove an entry.
But the intrusive linked list approach (intrusive in that the connections themselves serve as the links) solves the problem without any copy at all, without even sacrificing any performance in other ways. This works because even if a node is removed from the linked list (through the previous node no longer pointing to it), any loops sitting on that node can still follow the next pointer to get to the next node in the list.
7 Likes
Not sure how I missed this awesome thread but here’s some pure Lua function comparison / benchmark for people who have doubts and not convinced to use the new task library
(I’ll drop place file later)
function Connection:Disconnect()
assert(self._connected, "Can't disconnect a connection twice.", 2)
I don’t think this should error since RBXScriptConnection doesn’t iirc
The only thing that I disagree with is there is no Type Luau and GoodSignal doesn’t have exactly the same spec as RBXScriptConnection
Doesn’t have .Connected
Other than that it’s a really solid module I’m definitely forking it!
For reference, the only reason that Roblox doesn’t throw an error in that case is that there’s too much legacy code accidentally calling it more than once by doing this kind of pattern:
function onEquip/Select()
if condition then
onMouseDownConnection = mouse.Button1Down...
end
end
function onUnequip()
if onMouseDownConnection then
onMouseDownConnection:Disconnect()
--> forgot to set onMouseDownConnection to nil :(
end
end
But disconnecting the same connection more than once is almost always a bug, so I chose to throw in my implementation.
1 Like
I don’t really know why he didn’t just make it a public thing.
This is new behaviour, right? I remember this being default behaviour; I guess this is from Deferred Events launching…
I think it would be cool to see a little variable which defines that behaviour inside the script, but it’s whatever I guess.
If anyone is forking this and removing the coroutine recycling part, you can optimize :Wait in a way that you change _func to the :Wait's thread and then just resume it from there, you do have to keep some value to say that it is a wait thread, but it does work.
That’s better if you’re using arrays for private values, as arrays are O(1) to index, so the impact of searching that index is almost none.
This is incorrect, and actually the opposite on Roblox! Using an array as the backing for your object instead of a map of fields is not only wildly unreadable but also slower as well!
This is because the Roblox Luau VM pre-caches the index into the hashmap that will be needed for field accesses, so most field accesses also only need to effectively do a single array lookup, while a numeric array lookup is slower because it needs to check if the type of the index is a number or different type to tell whether to index the array part or not (vs a field lookup immediately knows that it should look in the hash part).
2 Likes
I was wondering if it’s possible to add :ConnectParallel properly? I tried, I think it worked, not sure… Trying to use unsafe functions gave me an error, but I honestly can’t tell if it’s even in parallel.
I tried it like this:
Just want to clarify a few things:
This is because the Roblox Luau VM pre-caches the index into the hashmap that will be needed for field accesses,
-
- This is nothing special actually. In both Luau and Lua, string hashes are computed and cached when interned, and due to this, a hash map lookup doesn’t need to calculate the hash for the specific key.
because it needs to check if the type of the index is a number or different type to tell whether to index the array part or not
-
- Strictly speaking of the implementation of tables in Lua, an array will be considered an dictionary by Lua if it has explicit keys like the ones down below:
local t = {[1] = 1, [2] = 2} -- Lua thinks this is an dictionary, whereas
-- in reality, its an array.
-
- The type of index have to be checked in both array and hashmap lookups, not only for array lookups.
What I said is mostly relevant in Lua, however we don’t know much about Luau’s internals. It’s safe to say that the first point is relevant in both. However, the second and third may not be.
1 Like
Hiya, could you elaborate on what you mean by this? Running this benchmark
local array = {}
local dict = {}
for i = 1, 1e5 do
table.insert(array, i)
dict['a' .. i] = i
end
return {
ParameterGenerator = function()
return
end,
Functions = {
array = function()
for _ = 1, 1e5 do
local _ = array[1e5]
end
end,
dict = function()
for _ = 1, 1e5 do
local _ = dict.a1
end
end
}
}
results in this:
1 Like
Your benchmark needs some work. There’s a few factors confounding the result:
-
Your example is contrived because of the large number of fields in the object. No object in people’s code actually has 10000 unique fields in it and this actually matters too because field access is optimized for a small number of fields, so you’re hitting the bad case of field access time when you have so many fields.
-
You’re not caching the upvalue reference, and accessing that is going to be more than 50% of the total work the loop iterations are doing, so that will pollute the results with a lot of noise:
- You need to do way more work in each iteration of the loop. Simply running the loop itself is taking a lot of the time right now, again causing issues for the measurement.
Here’s my version of your benchmark which shows a dict based 4-element vector (4 because my connection class has 4 fields) being significantly faster than an array based one (with a transposed version of the access to show that sequential vs non-sequential access is not a relevant factor with this small struct size):
Though I should also note that at a very small object size and an operation as fast as field access, the details of how field access works are extremely sensitive, so you have to be pretty careful with how you benchmark.
1 Like
Just to extinguish any doubt, I wrote a converted the module over to use arrays internally instead of fields and used the same benchmarking tool on it with this benchmark code:
The dict based signal wins over the array based signal in both cases just like it did with my benchmarking when I was initially writing it, so the result from the above benchmark carries over to practictical scenarios as well.
Okay, in detail, what Roblox does is slightly more than this. It actually figures out what most likely hash bucket to look in is ahead of time and stores that information directly in the VM instruction, it doesn’t simply have the string hash pre-computed like normal Lua. Sorry for not very accurate explanation.
The fast-path stuff like expected-hash-bucket based indexing all happens right in the core VM loop before things even get down into to the core slow-path functions like luaH_get.
Okay, in detail, what Roblox does is slightly more than this. It actually figures out what most likely hash bucket to look in is ahead of time and stores that information directly in the VM instruction, it doesn’t simply have the string hash pre-computed like normal Lua. Sorry for not very accurate explanation.
This is incorrect. In the case of Lua, it doesn’t store any of the information in a VM instruction and nowhere in Luau is documented that the information is stored in a VM. The only thing different is that Luau inline caches tables for field lookups. Take a look at the header for string objects:
The hash bucket is literally the hash of the key, each hash bucket is a linked list. When luaH_get calls luaH_getStr (for a key which is a string), it already gets the precomputed hash of the key and iterates through a linked list, searching nodes and seeing if the key stored inside each node is a string and is equivalent to the key whose value you need to find.
The fast-path stuff like expected-hash-bucket based indexing all happens right in the core VM loop before things even get down into to the core slow-path functions like
luaH_get.
This is incorrect, all of the actual work happens in these 2 functions which luaH_get calls one of them depending on the type of key. You are most likely assuming information out your own at this point.
PS: It is getting very off topic here, much better to continue in PMS if you’d like.
1 Like
That’s for vanilla Lua, all of what we’re talking about here is specific to the Roblox Luau VM, which is significantly diverged from the vanilla Lua one. It goes as far as having modified and additional opcodes in the bytecode format as well as having a completely rewritten core interpreter loop.
Yes, in vanilla Lua using an array is basically always faster than a struct with actual fields, but as you can see from the benchmark above that’s not true for the Roblox Luau VM because of the things I mentioned.
That is correct, but you never cited any of your sources for saying that Luau stores that information inside a VM instruction (which seems redundant).
Yes, in vanilla Lua using an array is basically always faster than a struct with actual fields, but as you can see from the benchmark above that’s not true for the Roblox Luau VM because of the things I mentioned.
Yup, I cited in terms of Lua. Luau does some voodo magic for table field lookups as well as inline caching.
Sorry for the lack of citations, that’s me being a bit lazy. The info is spread buried deep inside many Luau Recap threads / release notes, so it’s pretty hard to dig up.
I thought it would be useful info regardless even with a citation, because many people still mucking up their code under the misconception that it will be faster if they use arrays where they should be using proper structs.
2 Likes
I see! so, to clarify - if I for instance have something of the like:
for _, Data in next, SomeTable do
local Math = Data[1] * Time + Data[2] / Data[3] -- etc
end
would it be better to convert the Data into a dictionary, then? I am iterating through ~20,000 elements and indexing at least 2-3 properties for each one, so would it be more worthwhile for me to make Data a dictionary instead here?
Depends what your needs are.
If what you care about is perf, then for a 3-5 element item fields should beat an array (IIRC for a 2 element item using an array still wins for some reason, somehow the bucket logic must not work out in that case)
If what you care most about above all else memory and you have specifically 3 or less numeric fields, then you should actually put the three values into a Vector3: Since Vector3s are now a native type, you will avoid doing any memory allocation of a table grouping them and the numeric data will just be stored directly in the main table. This also works for compressing an array of plain numbers by a factor of 3 at the expense of it being slower to access them.
1 Like
Doesn’t coroutine.wrap do the same? Where does the performance gain come from?
1 Like