Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
KaelaSavia opened this issue Dec 5, 2023 · 5 comments

Comments

@KaelaSavia
Copy link
Contributor

KaelaSavia commented Dec 5, 2023

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
sourcehook

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
Copy link
Contributor Author

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
Copy link
Member

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

@psychonic
Copy link
Member

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
Copy link
Member

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
Copy link
Member

psychonic commented Dec 14, 2023

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?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants