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

feat: run duplicate pre-hooks just once #31

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

jaypaik
Copy link
Collaborator

@jaypaik jaypaik commented Jan 22, 2024

With #29, which removes the ability for plugins to depend on other plugin's hooks, the only cases where there will be duplicate hooks applied to a selector are:

  1. When a plugin's manifest incorrectly applies the same pre-hook twice to a selector.
  2. When different plugins apply ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY to the same selector.

This makes running duplicate pre-hooks multiple times unnecessary.

Note that duplicate post-hooks may still run multiple times if they are attached to different pre-hooks, since the pre-hook return data that are passed to them may be different. And since they will be configured from the same plugin, any duplicate runs will be intentional.


Edit: to expand, in cases where a plugin might want to apply {preHook: A, postHook: B} and {preHook: A: postHook: C} to a selector, AND they want A to be executed twice and not once, this can be addressed by collapsing the logic in B and C into a single post-hook.

@jaypaik jaypaik requested a review from adam-alchemy January 22, 2024 04:21
@jaypaik jaypaik force-pushed the 01-21-refactor_use_bool_instead_of_struct_to_store_permitted_calls branch from bff3964 to d154a96 Compare January 23, 2024 20:07
@jaypaik jaypaik force-pushed the 01-21-feat_run_duplicate_pre-hooks_just_once branch from 540df6f to f298cfe Compare January 23, 2024 20:07
Copy link
Contributor

@adam-alchemy adam-alchemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for also collapsing the redundant internal functions that were re-used for permitted call hooks.

Base automatically changed from 01-21-refactor_use_bool_instead_of_struct_to_store_permitted_calls to main January 23, 2024 23:46
@jaypaik jaypaik force-pushed the 01-21-feat_run_duplicate_pre-hooks_just_once branch from f298cfe to 1869bd4 Compare January 23, 2024 23:46
@jaypaik jaypaik merged commit 01a428a into main Jan 23, 2024
3 checks passed
@jaypaik jaypaik deleted the 01-21-feat_run_duplicate_pre-hooks_just_once branch January 23, 2024 23:55
@huaweigu
Copy link
Contributor

Thanks for the simplification! Do you think if it's still worth of storing duplicated preHooks if they're only run once now?
(It seems like HookGroup still records the count of dupes)

@jaypaik
Copy link
Collaborator Author

jaypaik commented Jan 25, 2024

Thanks for the simplification! Do you think if it's still worth of storing duplicated preHooks if they're only run once now? (It seems like HookGroup still records the count of dupes)

@huaweigu Yes, but it can be simplified further to reduce costs. There is still the possibility of the magic values like PRE_HOOK_ALWAYS_DENY being set multiple times for a given function, so we need to keep track of the number of times it has been set to facilitate proper uninstalls.

@huaweigu
Copy link
Contributor

Thanks for the simplification! Do you think if it's still worth of storing duplicated preHooks if they're only run once now? (It seems like HookGroup still records the count of dupes)

@huaweigu Yes, but it can be simplified further to reduce costs. There is still the possibility of the magic values like PRE_HOOK_ALWAYS_DENY being set multiple times for a given function, so we need to keep track of the number of times it has been set to facilitate proper uninstalls.

Thanks, I was thinking even if PRE_HOOK_ALWAYS_DENY being applied to the same selector multiple times, the result from running them would be the same(please correct me if I'm missing anything)? If that's the case, could we consider just installing/storing one preHook and uninstalling it later?

@jaypaik
Copy link
Collaborator Author

jaypaik commented Jan 26, 2024

Thanks for the simplification! Do you think if it's still worth of storing duplicated preHooks if they're only run once now? (It seems like HookGroup still records the count of dupes)

@huaweigu Yes, but it can be simplified further to reduce costs. There is still the possibility of the magic values like PRE_HOOK_ALWAYS_DENY being set multiple times for a given function, so we need to keep track of the number of times it has been set to facilitate proper uninstalls.

Thanks, I was thinking even if PRE_HOOK_ALWAYS_DENY being applied to the same selector multiple times, the result from running them would be the same(please correct me if I'm missing anything)? If that's the case, could we consider just installing/storing one preHook and uninstalling it later?

Correct, the result of running them would be the same. When uninstalling though, you'd want to make sure that the plugin you're uninstalling applied the last instance of that hook to that function before removing it. If two plugins applied that hook, and one of them is uninstalled, you'd still want to keep that hook applied instead of removing it.

@huaweigu
Copy link
Contributor

Thanks for the simplification! Do you think if it's still worth of storing duplicated preHooks if they're only run once now? (It seems like HookGroup still records the count of dupes)

@huaweigu Yes, but it can be simplified further to reduce costs. There is still the possibility of the magic values like PRE_HOOK_ALWAYS_DENY being set multiple times for a given function, so we need to keep track of the number of times it has been set to facilitate proper uninstalls.

Thanks, I was thinking even if PRE_HOOK_ALWAYS_DENY being applied to the same selector multiple times, the result from running them would be the same(please correct me if I'm missing anything)? If that's the case, could we consider just installing/storing one preHook and uninstalling it later?

Correct, the result of running them would be the same. When uninstalling though, you'd want to make sure that the plugin you're uninstalling applied the last instance of that hook to that function before removing it. If two plugins applied that hook, and one of them is uninstalled, you'd still want to keep that hook applied instead of removing it.

Thank you Jay for your explanation. It makes sense for PRE_HOOK_ALWAYS_DENY since plugin address doesn't reallly play a role here in the hook resolving process. If it's just for this special case, I feel like maybe there's some optimization we can come up to cut a potential sstore.

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.

3 participants