game.tf2.ext.2.tf2.so introduces significant performance overhead when weapons are created/destroyed

[KaelaSavia]

Whenever new tf_weapon_ entity is created, criticals.cpp gives rise to hook related to it. Same goes for unhooking in destruction.
https://github.com/alliedmodders/sourcemod/blob/master/extensions/tf2/criticals.cpp#L116

This can result in significant performance issues in majority of custom tf2 gamemodes which make use of TF2_RemoveWeaponSlot or TF2_RemoveAllWeapons or TF2_RegeneratePlayer when weapons are stripped/dropped.

It also occurs with vanilla functionality such as player changing loadout or class.

One solution could be replacing hooking/unhooking by default by TF2_HookCrits(int entity) TF2_UnHookCrits(int entity). But that would break a lot of plugins.

A bit relevant to #1935

[KaelaSavia]

As a bit of update, performance overhead shouldn't happen if none of plugins you have loaded use TF2_CalcIsAttackCritical as criticals.cpp hooking functionality will be not enabled then (if im understanding code correctly).

Until criticals.cpp impact is somewhat mitigated, if you find it impacting your performance i'd personally recommend removing any plugins using it, or replace it's use with detour as dhooks support detours now

[KyleSanderson]

As a blanket statement: Almost all of the hooks on entities providing forwards like this should be vtable hooks.

[psychonic]

We could probably just do a dvp hook on the first of each ent class we see, also storing the ent to a list of ones we care about the hook for, just removing from that when the entity is removed, and all active hooks on no plugins using.

[KyleSanderson]

We could probably just do a dvp hook on the first of each ent class we see, also storing the ent to a list of ones we care about the hook for, just removing from that when the entity is removed, and all active hooks on no plugins using.

That's the correct way (and the way SDKHooks has been), unfortunately there's this whole series: #2094

But yeah, if the vector code wants to be ported here I don't see a problem with it.

[psychonic]

We could probably just do a dvp hook on the first of each ent class we see, also storing the ent to a list of ones we care about the hook for, just removing from that when the entity is removed, and all active hooks on no plugins using.

That's the correct way (and the way SDKHooks has been), unfortunately there's this whole series: #2094

But yeah, if the vector code wants to be ported here I don't see a problem with it.

I'm not very familiar with the SDKHooks code since that change, but I am aware of #2094. I was under the impression that the performance issue was related to hooks being constantly added and removed. Does what I said not address that here? (Only unhooking any of the dvp hooks upon no plugin using the forward, rather than on removal of any one entity, just adding tracking of which entities we care about for when the callback hits?)