Remove CompositeTracer wrapper layer (item 2 of #115)#409
Open
islamicfinanceaaoifi-ops wants to merge 1 commit into
Open
Conversation
The session lifecycle (push/pop the patching_module, wire sys.monitoring on 3.12+ or sys.settrace on earlier versions, start/stop the tracer) used to live in a Python `CompositeTracer` class that simply wrapped a `self.ctracer = CTracer()` and forwarded most methods through. The maintainer's spec on pschanely#115 calls for removing that layer entirely. This change folds the wrapper's responsibilities into CTracer in C: - `CTracer` gains a `patching_module` attribute (tp_getset) so callers no longer have to reach through a wrapper object to access it. - `CTracer.__enter__` / `CTracer.__exit__` are now native methods that push the patching_module, configure sys.monitoring (3.12+) or save and install via sys.settrace (older), call start/stop, then unwind. - `CTracer.trace_caller` is implemented in C (no-op on 3.12+; on older Python it enables opcode tracing on the calling frame, matching the previous Python implementation that used `sys._getframe(2)`). - The unconditional `sys.monitoring.restart_events()` that used to live in `CompositeTracer.push_module` is now invoked from `CTracer_push_module` itself (only when the tracer is currently enabled, since restarting events has no effect otherwise). - Helpers `_ch_load_sys_monitoring` and `_ch_call_monitoring_method` centralize the sys.monitoring lookup so the lifecycle methods stay readable. `tracers.py`: deletes the `CompositeTracer` class entirely. The module singleton is now just `COMPOSITE_TRACER = CTracer()` with its `patching_module` set explicitly. `is_tracing`, `NoTracing`, and `ResumedTracing` go through the CTracer instance directly instead of `COMPOSITE_TRACER.ctracer`. Callers are updated to the unified API: - `COMPOSITE_TRACER.pop_config(m)` -> `COMPOSITE_TRACER.pop_module(m)` - `COMPOSITE_TRACER.set_postop_callback(cb, frame)` -> `COMPOSITE_TRACER.push_postop_callback(frame, cb)` (note arg swap) Touches: opcode_intercept.py (7 callsites), enforce.py, core.py, tracers_test.py. Verified: tracers_test.py, _tracers_test.py, condition_parser_test.py, enforce_test.py, statespace_test.py, util_test.py, opcode_intercept_test.py, fuzz_core_test.py, core_test.py, diff_behavior_test.py, path_cover_test.py all pass on Python 3.14.3 / Windows.
Owner
|
@islamicfinanceaaoifi-ops thanks for helping! Please check contributing.rst for getting the formatting/linting etc right.
I expect that we'll still need this to happen because the tracer can be re-enabled later. If the tests pass anyway, I'd want to know whether we need test coverage there, or why my understanding is flawed! :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses item (2) of #115 — "CompositeTracer is now largely just a wrapper around CTracer. Let's try to remove the CompositeTracer layer entirely."
Item (3) (porting
TracingModuleto C) is left for a follow-up PR so the diff stays reviewable; happy to take that on next if this approach looks right.What changed
The old
CompositeTracerclass owned aself.ctracer = CTracer()instance and forwarded most methods through. The session lifecycle — push/pop thepatching_module, wiresys.monitoring(3.12+) orsys.settrace(older), start/stop the tracer — lived in its__enter__/__exit__. This PR folds those responsibilities intoCTraceritself and deletes the wrapper.C side (
_tracers.c,_tracers.h)patching_moduleslot onCTracer, exposed viatp_getset. Defaults toNone.__enter__/__exit__C methods that push thepatching_module, configuresys.monitoringon 3.12+ (or stashsys.gettrace()and enable opcode tracing on the current frame for older Python), callstart()/stop(), then unwind on exit.trace_callerC method (no-op on 3.12+; on older Python enables opcode tracing on the calling frame — same as the previoussys._getframe(2)-based Python implementation).CTracer.push_modulenow invokessys.monitoring.restart_events()itself on 3.12+ (only when the tracer is enabled — restarting has no effect otherwise). Thatrestart_events()call used to live in the now-deletedCompositeTracer.push_module._ch_load_sys_monitoringand_ch_call_monitoring_methodcentralize thesys.monitoringlookup so the lifecycle methods stay readable.Python side (
tracers.py)CompositeTracerclass is gone. The module singleton is now justCOMPOSITE_TRACER = CTracer()withpatching_module = PatchingModule()set explicitly.is_tracing,NoTracing,ResumedTracing, andPushedModulenow go through theCTracerinstance directly instead ofCOMPOSITE_TRACER.ctracer.Caller renames
The wrapper provided two name-only shims; callers move to the unified
CTracerAPI:COMPOSITE_TRACER.pop_config(m)->COMPOSITE_TRACER.pop_module(m)COMPOSITE_TRACER.set_postop_callback(callback, frame)->COMPOSITE_TRACER.push_postop_callback(frame, callback)(argument order swap)Touches
opcode_intercept.py(7 callsites),enforce.py,core.py,tracers_test.py.Verified locally (Python 3.14.3 / Windows 11 / MSVC 14.50)
libimpl/also runs through; the only failure I saw (datetimelib_ch_test::test_builtin[check_datetime_astimezone]) reproduces on unmodifiedmainand isn't related to this refactor — current main CI is red for the same family of path-exploration failures.Design notes / questions for reviewer
COMPOSITE_TRACERso this PR doesn't ripple a rename through downstream consumers. If you'd rather call itTRACERor similar now that it's just aCTracer, happy to follow up.sys.monitoring.restart_events()insidepush_moduleis now guarded byself->enabled. The previous Python wrapper called it unconditionally. Let me know if you'd prefer the unconditional behavior — I went conservative on the perf side since it's only meaningful when the tracer is actively running._PyFrame_GetBackBorrowintrace_callerwalks back one frame to mirror the oldsys._getframe(2)semantics (top Python frame on the eval stack is the caller oftrace_caller, one back is the function we want to trace).prev_traceobjis stored on the struct for the older-Python branch so__exit__can restoresys.settraceto whatever was set before the session.Ready for item (3) (porting
TracingModule.trace_op+_CALL_HANDLERSand thehandle_call_*family to C) as a follow-up — let me know if you'd like me to open that PR next.IssueHunt Summary
Referenced issues
This pull request has been submitted to: