Hook library rewrite (prioritization and performance enhancements)

[meepen]

This is based on my rewrite from https://gist.github.com/meepen/5566c9598e0e610e5885d12fad2ec3a8

Notes:

• This maintains almost 100% backwards compatibility with the current hook system, the only exception is that hook.GetTable() modifications won't add or remove hooks to run. I believe this will be fine because ULX does the same, so developers have come to expect it won't work on a majority of servers.
• Adds priority to the hook.Add function (higher number = higher priority)
• Adds hook.Priority (a table) with a single member NO_RETURN
• NO_RETURN is NOT enforced in code but should be followed as a standard. It will always be ran first in the priority list.
• Adds hook.GetPriority(event, name)
• Adds hook.GetInternal() so people who want to irritate the hook library can do so if they want without rewriting it or replacing it with a whole new library. Please don't document this on the wiki, or make it say it is internal and should not be used.
• Adds some type checking to arguments for sanity's sake

credits: @ThatLing for reviewing my code to ensure everything is tip top shape and can be read

[thegrb93]

Should've used sorted array instead of linked list. This is not ideal.

[meepen]

sorted arrays and linked lists have no performance differences in lua.

[meepen]

] - local T,t,nop,i,s,this={{}},SysTime,function(e)end,1 for i=1,1e7 do T[i]={{}}end this=T[i]s=t()::s:: nop(this[1]) i=i+1 this=T[i] if this then goto s end print(t()-s)
0.18326508712698
] - local this,t,nop,i,s,n={},SysTime,function(e)end,1 n=this for i=1,1e7 do n[1]={}n=n[1]end s=t()::s:: nop(this[1]) this=this[1] if this then goto s end print(t()-s)
0.13612537420715

[thegrb93]

Why are you iterating a sequential array with 'goto'? No shit it'll be slower that way.

Goto for loop:

> local T,t,nop,i,s,this={{}},SysTime,function(e)end,1 for i=1,1e7 do T[i]={{}}end this=T[i]s=t()::s:: nop(this[1]) i=i+1 this=T[i] if this then goto s end print(t()-s)...
0.14187382361831

Goto Linked list:

local this,t,nop,i,s,n={},SysTime,function(e)end,1 n=this for i=1,1e7 do n[1]={}n=n[1]end s=t()::s:: nop(this[1]) this=this[1] if this then goto s end print(t()-s)...
0.10879372726475

For loop:

> local T,t,nop,i,s,this={{}},SysTime,function(e)end,1 for i=1,1e7 do T[i]={{}} end s=t() for i=1, #T do this=T[i] nop(this[1]) end print(t()-s)...
0.054398570300123

[meepen]

I think the main problem is the branching in the linked list, after that it's fine. If I use an iterative loop as you are doing it would be fine. I will update it.

[meepen]

local this,t,nop,i,s,n={},SysTime,function(e)end,1 n=this for i=1,1e7 do n[1]={false}n=n[1]end s=t()for i = 1, 1e7 do nop(this[1]) this=this[1] end print(t()-s)
0.041814087393362
local T,t,nop,i,s,this={{}},SysTime,function(e)end,1 for i=1,1e7 do T[i]={{}} end s=t() for i=1, #T do this=T[i] nop(this[1]) end print(t()-s)
0.0430561556039

[mcNuggets1]

Please merge this, I'll pay with my liver!

[meepen]

@thegrb93 I remember why I didn't use arrays and that is because when removing something after IsValid fails will screw up ordering. Linked lists made it a ton simpler.

[meepen]

It's a LOT faster now, and still works perfectly.

[thegrb93]

It looks a lot better, just one more thing. The Add function can't modify the 'event_table' because it might happen while Call is iterating, causing skipping or multi-calling of hooks. It should modify (or create if it doesn't exist) a separate staging table which 'Call' will check for and update to before iterating. Of course hook.Call can be called from inside a hook as well so I think a counting sentinel variable is needed to keep track of how many hook.Call is in the stack, and only update if it's the first one. Idk, maybe you have a better solution.

This would allow the Remove function to remove the hook instead of setting it to noop as well.

[thegrb93]

Tested the original vs https://github.com/Facepunch/garrysmod/pull/1485/commits/59b0b95347bbbdfa302b7ab0da9f0f12a4e1e269 vs https://github.com/Facepunch/garrysmod/pull/1485/commits/be3ec1c9c60e100f3adfe2652d9569dd8a4f4386

with

local a = SysTime()
for i=1, 1000 do
	hook.Add("TestHooks",tostring(i),function() end)
end
a = SysTime() - a
local b = SysTime()
for i=1, 100000 do
	hook.Call("TestHooks")
end
b = SysTime() - b
local c = SysTime()
for i=1, 1000 do
	hook.Remove("TestHooks",tostring(i),function() end)
end
c = SysTime() - c
print("hook.Add x 1000: "..a.."\nhook.Call x 100000: "..b.."\nhook.Remove x 1000: "..c)

original

] lua_openscript testhookcall.lua
Running script testhookcall.lua...
hook.Add x 1000: 0.00068461756018223
hook.Call x 100000: 3.7898931138393
hook.Remove x 1000: 0.00061366894578896
] lua_openscript testhookcall.lua
Running script testhookcall.lua...
hook.Add x 1000: 0.0006204956162037
hook.Call x 100000: 3.7789762919474
hook.Remove x 1000: 0.00058343654609416
] lua_openscript testhookcall.lua
Running script testhookcall.lua...
hook.Add x 1000: 0.00062707847746424
hook.Call x 100000: 3.7821104652867
hook.Remove x 1000: 0.00057977940105047

https://github.com/Facepunch/garrysmod/pull/1485/commits/59b0b95347bbbdfa302b7ab0da9f0f12a4e1e269

] lua_openscript testhookcall.lua
Running script testhookcall.lua...
hook.Add x 1000: 0.0050909897697125
hook.Call x 100000: 1.7501787124899
hook.Remove x 1000: 0.0048793629740658
] lua_openscript testhookcall.lua
Running script testhookcall.lua...
hook.Add x 1000: 0.0048444981909483
hook.Call x 100000: 1.6998171427457
hook.Remove x 1000: 0.0048374277104131
] lua_openscript testhookcall.lua
Running script testhookcall.lua...
hook.Add x 1000: 0.0049232487151016
hook.Call x 100000: 1.7172124752533
hook.Remove x 1000: 0.0045911799413147

https://github.com/Facepunch/garrysmod/pull/1485/commits/be3ec1c9c60e100f3adfe2652d9569dd8a4f4386

] lua_openscript testhookcall.lua
Running script testhookcall.lua...
hook.Add x 1000: 0.15113273973793
hook.Call x 100000: 1.7683649635746
hook.Remove x 1000: 0.0013819132232129
] lua_openscript testhookcall.lua
Running script testhookcall.lua...
hook.Add x 1000: 0.15183661826222
hook.Call x 100000: 1.771345049201
hook.Remove x 1000: 0.0013687475011466
] lua_openscript testhookcall.lua
Running script testhookcall.lua...
hook.Add x 1000: 0.15025258682044
hook.Call x 100000: 1.7688340533846
hook.Remove x 1000: 0.0013592389236692

I'm guessing the queue is adding some overhead. hook.Add is quite a bit slower than linked list version. Both hook.Call are considerably better than original though.

[meepen]

Add and Remove performance doesn't matter since it's not called 5 bajillion times a second.

[FredyH]

I think I know one major improvement of this implementation. Currently the order hooks are called is does not correspond to the order they were added in. In your implementation this is also a problem since hooks of the same priority will also get called in any order. I don't know if it's feasible to add this without sacrificing a lot of performance, but if it is you should go for it.

I think you should also really change these comments: [ --[[comment]] ] and rather have them be above the line or at the end of the line, because currently the code is really annoying to read.

[meepen]

I think I could make it so order of hook adding matters.

Also, the comments like that are for easy replacing if it ever gets changed. I probably can just add locals to the top of the file though.

[thegrb93]

I've been testing https://github.com/Facepunch/garrysmod/pull/1485/commits/be3ec1c9c60e100f3adfe2652d9569dd8a4f4386 for a while and found a case where hook.Add wasn't working. I'll update to latest and see if its fixed.

[thegrb93]

If you add a remove a hook from inside its call and try re-adding it, the add will fail

hook.Add("test","blah",function()
    hook.Remove("test","blah")
    hook.Add("test","blah",function() end)
end)

[mcNuggets1]

Tomorrow this is opened for 1 month. Can we get any reply from the devs or the author about "thegrb93's" comment about this?

[meepen]

Hey, I'm working on this again. Currently optimizing and the fix for grb's situation was just changing the direction the update list was iterated.

[meepen]

I change my mind, I'm going back to linked lists.

BENCHMARK
-------------
CallInvalid (200 calls)
hooklib/oldhook.lua (-357.29%)
includes/modules/hook.lua: 0.010206976 s
hooklib/oldhook.lua:       0.002232051 s
-------------
CallNoHooks (200000000 calls)
includes/modules/hook.lua (-3.44%)
includes/modules/hook.lua: 0.056299842 s
hooklib/oldhook.lua:       0.058236223 s
-------------
CallGMOnly (200000000 calls)
includes/modules/hook.lua (-3.85%)
includes/modules/hook.lua: 0.056151497 s
hooklib/oldhook.lua:       0.058313454 s
-------------
CallNoGM (32000000 calls)
includes/modules/hook.lua (-75642.34%)
includes/modules/hook.lua: 0.009049022 s
hooklib/oldhook.lua:       6.853941213 s
-------------
CallGM (32000000 calls)
includes/modules/hook.lua (-71738.51%)
includes/modules/hook.lua: 0.010311990 s
hooklib/oldhook.lua:       7.407980329 s

This benchmark was done with 3 hooks in each Call* that had hooks. CallInvalid added 25 invalid and valid hooks each time, so the performance will probably be messed up from the Add calls.

The problem with the old version(s) was that they had issues or were unoptimized or had code that was blacklisted by the jit.

I will be updating the main post soon.

[mcNuggets1]

ULib goes from lowest to highest, priority wise, this is the opposite. Thus it will break ULib compatibility.

[Kefta]

ULib would still override the hook lib with their version.

[mcNuggets1]

Yeah, but I guess if they want to remove their hook replacement, they have to do more work in order to do so. Also it breaks a lot of addons ulib dependant.

[Kefta]

They can just reverse the priority values for their replacement since it uses enums and not raw numbers.

[mcNuggets1]

Yeah, works for me. Even tho a lot of ulib hooks are gonna break possibly.