-
Notifications
You must be signed in to change notification settings - Fork 138
Divide and specialize hook impls/callers #634
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
base: main
Are you sure you want to change the base?
Divide and specialize hook impls/callers #634
Conversation
…ntations Divide hook abstractions into specialized types that encode behavior through class identity rather than runtime flags. This enables type-based dispatch via match statements instead of conditional branching on attributes. Hook callers now specialize by result handling strategy (list, first-result, historic). Hook implementations specialize by execution style (normal vs wrapper, new vs legacy wrapper). The _multicall function receives separate lists for normal and wrapper implementations, enabling cleaner dispatch logic. Backward compatibility maintained via HookCaller alias and legacy HookImpl. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Shift responsibility for argument extraction and wrapper setup/teardown from _multicall into the hook implementation classes themselves. The hookexec loop is now a simple iteration: - setup_teardown() on each wrapper (returns teardown callable) - call() on each normal impl - teardown(result, exception) returns (result, exception) Key changes: - Add _get_args() method to base _HookImplementation - Add call() method to _NormalHookImplementation - Add setup_teardown() method to _WrapperHookImplementation - Teardown returns tuple instead of raising exceptions - Result objects only created for legacy hookwrappers 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update test_hook_and_wrappers_speed to use separate normal_impls and wrapper_impls lists with _create_hook_implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
bluetech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting change which has lots of potential, I think we should definitely look into it.
The PR itself is however somewhat hard to review. I think it would be easier to follow if it it's split at least to these parts:
- First, move calling logic (setup, call, teardown) from
_multicalltoHookImpl. Still no polymorphism here, just moving the logic. - Introduce polymorphism to
HookImpl(normal, old style wrapper, new style wrapper). - Introduce polymorphism to hook callers (normal, historic, first result, etc.). I haven't looked at this part yet, but I think this is orthogonal to the HookImpl part?
It would also be good to explicitly state the rationale for this work, e.g. is it
- Just cleaner code?
- Performance?
- Extensibility?
- Needed for future work?
Another thing we should ensure is that there is no performance regression. Right now it looks like 2.5x slowdown. Conceptually I don't think this change requires a slowdown, maybe even it should speed things up by removing some conditionals though that's likely very minor.
My guess is that the current slowdown stems from the usage of teardown closures, which certainly simplify the code but likely add a bunch of overhead (a function def on every wrapper hookimpl call). I haven't verified this though. Possibly a way to avoid closure here would be to change the protocol to return an opaque "teardown token" from setup, which for current old/new wrappers would be the generator, and then pass that to the hook impl teardown, instead of hiding it in a closure.
We also should make sure there is no coverage regression.
If you proceed with the PR split-up I'll be happy to review each PR further.
|
a goal behind this is full native async support - for that the structure needs to be divided and comprehensible also i want to replace the current hook tracing with something thats just tlike a hook wrapper that wraps around all hooks eventually i want to replace the messy tracing we currently have with open telemetry/eliot/any other tree aware tracing also eventually i want to expand hook types - i'd like to add contexts its a shame my claude max20 free promo ends on the 22th - im going to have a look at speedrunning this between the days |
Summary
_NormalHookCaller,_FirstResultHookCaller,_HistoricHookCaller) and implementations (_NormalHookImplementation,_NewStyleWrapper,_OldStyleWrapper)_multicallloop becomes trivially simple:Test plan
🤖 Generated with Claude Code