New hook lib

[Srlion]

Faster hook library

I have released this on workshop few months ago to make sure that it doesn't break anything.

• This version is slightly different, this one is faster when objects become invalid and get removed.
• Thanks to @WilliamVenner for telling me to upload it first on workshop.
• Big thanks to @meepen for his hook library, which inspired me to make this.

benchmark (using a slightly modified version of https://github.com/meepen/gmod-hooks-revamped/blob/master/hooksuite.lua):

If you don't care about "CallInvalid" performance, I could change the code to be more readable.

Srlion's New Lib...
Srlion's New Lib took 0.511766100 s

Default Lib...
Default Lib took 14.408792900 s

BENCHMARK
-------------
CallInvalid (500 calls)
Srlion's New Lib (218.29% faster)
Default Lib:      0.005260700 s
Srlion's New Lib: 0.001652800 s
-------------
CallNoHooks (200000000 calls)
Default Lib (1.36% faster)
Default Lib:      0.213272500 s
Srlion's New Lib: 0.216162900 s
-------------
CallGMOnly (200000000 calls)
Default Lib (2.97% faster)
Default Lib:      0.215109900 s
Srlion's New Lib: 0.221490100 s
-------------
CallNoGM (32000000 calls)
Srlion's New Lib (19258.95% faster)
Default Lib:      6.825907900 s
Srlion's New Lib: 0.035259700 s
-------------
CallGM (32000000 calls)
Srlion's New Lib (19239.46% faster)
Default Lib:      7.149178400 s
Srlion's New Lib: 0.036966800 s
-------------

[thegrb93]

Why are you defining your own isstring and isfunction? They already exist. Also there's already like 3 other pull requests for hook lib rewrites. Get in line.

[Srlion]

So hook.Add gets compiled, also other hook library fails in this test:

hook.Add("T", "a", function()
    print("a")
    hook.Remove("T", "a")
    hook.Add("T", "b", function()
        print("b2")
    end)
end)
hook.Add("T", "b", function()
    print("b")
end)
hook.Add("T", "c", function()
    print("c")
end)

hook.Call("T")
print("==================")
hook.Call("T")

Here is what yours produces:

-==-=-=-=--==
a
b
c
b2
==================
c
b2

Here is what mine produces:

-==-=-=-=--==
a
b2
c
==================
c
b2

---

Here is another test that yours and the default hook library fails in:

hook.Add("T", "A", function()
    print("A")
    hook.Add("T", "E", function()
        print("E")
    end)
end)
hook.Add("T", "B", function()
    print("B")
end)

hook.Call("T")
print("==================")
hook.Call("T")

Here is what yours produces:

-==-=-=-=--==
A
B
E
==================
A
B
E

Here is what the default lib produces:

-==-=-=-=--==
A
A
E
==================
B
A
E

Here is what mine produces:

-==-=-=-=--==
A
B
==================
A
B
E

and it's the expected output.

Mine is well tested against any case, it's even more reliable than the default hook library. I had more tests that I don't remember where I have placed :s

[thegrb93]

Considering the behavior of those cases are undefined, those tests really don't mean anything.

[thegrb93]

I'm surprised the default lib prints A twice in the first hook.Call of the second test though. wtf caused that. You sure you did that one right?

[Srlion]

I'm just showing that mine is more "reliable" and have expected behavior. I don't really know why it prints twice and skips "B" lol

[thegrb93]

That's merely an opinion if there's no definition of the behavior. I get the feeling you messed up the default lib test. The default lib is just a 'pairs' loop so I can't see how it fucked up that bad.

[Srlion]

You can test it yourself lol

[thegrb93]

Huh, apparently adding stuff to a table being iterated with pairs in lua can cause it to iterate over something more than once. Did not know that.

[Srlion]

I think it's only in lua5.1, tested it here https://www.lua.org/demo.html and it works fine.

[thegrb93]

local t = {}
t.A = function() print("A") t.E = function() print("E") end end
t.B = function() print("B") end
for k, v in pairs(t) do
	v()
end

Yeah, lua demo: B A

gmod: A A E

[thegrb93]

I think this looks fine then. I still like mine better though.

[Srlion]

Tested it on latest luajit build and lua 5.1.5 and they produce same output, so I guess it won't be fixed.

[thegrb93]

Turns out that behavior is also undefined lol

https://www.lua.org/manual/5.3/manual.html#pdf-next "The behavior of next is undefined if, during the traversal, you assign any value to a non-existent field in the table. You may however modify existing fields. In particular, you may clear existing fields. "

[ExtReMLapin]

if you want your stuff to entirely compile, don't use pairs

[Srlion]

I could have an array and hashed table to avoid using pairs, but I don’t think it’s worth it?

The test thing I’m using made me forget about VARG, maybe I should do something like “a, b, d, ..etc”? Would require @robotboy655 to know if he would like that :s

[ExtReMLapin]

The JIT compilation of GetTable doesn't really matter as it's most likely only used for debug.

However, a function with more than 3 args should probably not exist.

A function with more than 3 args on a hook should 100% not exist at all.

You pretty much do what you want when calling internal functions of your addon, but if a dev decides to plug one of his functions to the hooking system, he should follow calling conventions.

[Srlion]

We can’t just limit it now, would break a lot of addons :s

[ExtReMLapin]

"a lot" is to define

If you code a function that takes 6 arguments and you allow people to hook on it, you should probably reconsider your career choice.

We're not talking about breaking all functions with > 5 args for example but funcs which are supposed to follow a clean calling convention.

[ExtReMLapin]

Also, while test cases doesn't mean anything without specs, it make sense in not breaking what's already in place.

[Srlion]

It would depend on Garrys Mod developers saying yes or no about it (I can’t change the default behavior for anything here :s). Also I don’t know if hook.Run calls from C++ to Lua would make any difference? Could you test it, if it does’t perform better then it’s not really needed to replace the default hook library with this :c

[ExtReMLapin]

Already found the first FUCKER using more than 5 args :

GM:CalcViewModelView( Weapon, ViewModel, OldEyePos, OldEyeAng, EyePos, EyeAng )

@garrynewman fix your calling conventions

[Kefta]

It's the same args as Source's CalcViewModelView. It's very arbitrary to design your entire API around a JIT limitation.

[ExtReMLapin]

As I said before, the difference here is the function is exposed to other developers, this is why i'm talking about calling conventions.

We could still go full monkey mode and do something like

function Call( event_name, gm, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8 )

	local event = events[ event_name ]
	if ( event ) then

		local i, n = 2, event.n

		::loop:: -- https://github.com/Facepunch/garrysmod/pull/1508#issuecomment-398231260
		local func = event[ i ]
		if ( func ) then

			local object = event[ i + 1 ]
			if ( object ) then

				if ( object.IsValid && object:IsValid() ) then
					--
					-- The object is valid - pass it as the first argument (self)
					--
					local a, b, c, d, e, f = func( object, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8 )
					if ( a != nil ) then
						return a, b, c, d, e, f
					end
				else
					--
					-- The object has become invalid - remove it
					--
					event:Remove( event[ i - 1 ] --[[name]] )
				end

			else
				local a, b, c, d, e, f = func( arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8 )
				if ( a != nil ) then
					return a, b, c, d, e, f
				end
			end
...

I mean, look at a, b, c, d, e, f it's pretty much the same thing.

[Kefta]

Compatibility with current addons should be the number one priority: it's too late to be limiting the number of args sent into hooks. Varadic hook returns were removed during the GM13 update as compatibility was no concern.

As I said before, the difference here is the function is exposed to other developers,

So is CalcViewModelView for Sourcemods. It's part of the public Source API that other developers can override in their custom entities. You're still basing your entire design philosophy around JIT which would be fine if this wasn't a legacy codebase.

[Srlion]

@ExtReMLapin could you measure hook.Run calls from C++, because calling Lua functions from C++ is heavy, and I don’t know if this would make any difference. (Important for things like Think, HUDPaint, ..etc)

[ExtReMLapin]

Either way and even with stitching, jit compilation is kinda dead by design as there is too much Calls to C API in the lua code from addons

[thegrb93]

Yeah I'm afraid you're worried about optimizing a ~10 element loop. Not something to go insane over.

[DBotThePony]

https://github.com/Facepunch/garrysmod/pull/1642#issuecomment-601288451

Implementing any kind of sorting over pairs produce known to me issues with existing addons. I know, we better had hook sorting from start of GM13, but changing it now will only break stuff

My hook's output by code provided in comment:

-==-=-=-=--==
b
a
c
==================
b2
c
-==-=-=-=--==
B
A
==================
B
A
E

Which should be very close to current "pairs undefined sorting", hence default lib, but without undefined behavior when removing hooks of event inside that event call.

I think it is better to leave current hook system as-is, until Rubat/Willox see how it can be really improved (no, really), without any breakage or weird behaviors.

[Srlion]

@DBotThePony Your one fails when you modify/remove:

hook.Add("T", "a", function()
    print("a")
    hook.Add("T", "b", function()
        print("new b")
    end)
end)
hook.Add("T", "b", function()
    print("b")
end)
hook.Call("T")
print("==================")
hook.Call("T")

Default Lib:

a
new b
==================
a
new b

Yours:

a
b
==================
a
new b

Like I said, these tests to show that this hook library is stable and works like the default one with being faster and no undefined behaviors. (Don't talk about sorting because it's not something to worry about at all.)

Here is the default behavior you should expect with some cases:

• If you add a new hook while the even is called, you expect it to run in next call. (Default Lib has undefined behavior when adding new hooks, but most of the time it runs in next call.)
• If you modify a hook before it's called, the new function will be called not the old one. (Just like the test above.)
• If you remove a hook before it's called, then it won't be called in this event call.

Last 2 cases are not undefined behaviors according to https://www.lua.org/manual/5.1/manual.html#pdf-next

I doubt that this "CallInvalid" benchmark is correct, adding/removing hooks with your hook library could be kinda heavy because of "hook.Reconstruct"

Also addons don't relay on "pairs undefined sorting", so we don't have to worry about sorting.

[DBotThePony]

I, personally, expect from normal hook library that when i call hook.Call, all existing functions at moment of call will be called back, not more, not less. Concurrent modifications of hook table while iterating it is evil (and will produce undefined result no matter what you try to do), and good part of languages disallow concurrent modifications while iterating data structures.

CallInvalid - hook.Remove's call to hook.Reconstruct is deferred until next hook.Call/hook.Add if they were called inside hook.Call (to save cpu time).

Sorting - i tried implementing sorted hook back in 2017, it didn't end well with one of my friend's addon and with at least one workshop addon - Playable Piano (it just refused to work because it's hook got moved in iteration loop).