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

Implement hook stacking and priorities #1843

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marzent
Copy link
Contributor

@marzent marzent commented Jun 15, 2024

Hook stacking without having multiple managed to native and native to managed transitions was the primary objective here, but priorities were relatively easy to implement on top of that.

Currently this implements the following priority bands:

  • 0: After-notify only hooks (always run last)
  • 1-127: Normal priority hooks
  • 128-191: Dalamud internal hooks
  • 192-254: High priority hooks
  • 255: Before-notify only hooks (always run first)

Normal priority hooks convert to Dalamud internal ones automatically based on the calling assembly. For fine grained control inside a priority band, the optional precedence parameter can be used to lower or raise the effective priority.

This also makes some locking a bit stricter (like making sure jumps are followed only under the global hook lock).

There is some IL dynamically generated in 3 places (most of the time only in 1), I don't believe there is a reasonably performant alternative to that unfortunately.
When stacking hooks of incompatible signatures (like ST private delegate void* GlobalEventDelegate(AtkUnitBase* atkUnitBase, AtkEventType eventType, uint eventParam, AtkResNode** eventData, uint* a5) and Dalamuds private delegate IntPtr AtkUnitBaseReceiveGlobalEvent(AtkUnitBase* thisPtr, ushort cmd, uint a3, IntPtr a4, uint* a5)) a warning is logged and IL thunks are generated inside a CastingHook<,>.

AsmHook seem to exist a bit outside the other hooks, so these currently do not stack, but are now also under the global hook lock.

This also fixes the callingAssembly not resolving to the actual plugin and marking all hooks as "Dalamud Hooks".

@marzent marzent requested a review from a team as a code owner June 15, 2024 22:39
@WorkingRobot
Copy link
Contributor

Could you also look into adding a version bump to Reloaded itself? The current version is quite unoptimized, as seen here and can cause hitches up to a second long in some cases.

@marzent
Copy link
Contributor Author

marzent commented Jun 20, 2024

The current reloaded also uses a thread-local assembler iirc...
But switching that out would be probably best done in its own PR I think, given that this one is already a bit unwieldy.

@goaaats goaaats deleted the branch goatcorp:master July 1, 2024 18:48
@goaaats goaaats closed this Jul 1, 2024
@KazWolfe KazWolfe reopened this Jul 1, 2024
@KazWolfe KazWolfe changed the base branch from apiX to master July 1, 2024 18:54
@KazWolfe
Copy link
Member

KazWolfe commented Jul 7, 2024

Another note: if Dalamud is starting to own hooks, can we have some way of tracking hook timings? That's right now our biggest blind spot in terms of figuring out what plugins are or aren't doing timing-wise.

@marzent
Copy link
Contributor Author

marzent commented Jul 9, 2024

Another note: if Dalamud is starting to own hooks, can we have some way of tracking hook timings? That's right now our biggest blind spot in terms of figuring out what plugins are or aren't doing timing-wise.

Yeah this can be done, wanted to clean up the priority calculation a bit more and will try to add that as well.

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

Successfully merging this pull request may close these issues.

4 participants