Refactor: unify profiling/DFX/timeout config naming under SIMPLER_ (incl. strace.h include fix)#1268
Refactor: unify profiling/DFX/timeout config naming under SIMPLER_ (incl. strace.h include fix)#1268ChaoWao wants to merge 2 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change makes ChangesStrace Header Fix
Estimated code review effort: 1 (Trivial) | ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request includes "profiling_config.h" in the low-level header "strace.h" to ensure it is self-contained and correctly resolves the SIMPLER_PROFILING macro. The reviewer notes that this introduces a layering violation, as low-level logging utilities should not depend on higher-level task interface modules, and suggests relocating the configuration or using structured include paths to prevent compilation issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // `#if SIMPLER_PROFILING` gate below evaluate false and silently compile every | ||
| // STRACE to a no-op — dropping the simpler_run host-trace markers that | ||
| // consumers (pypto-serving) read from the log. | ||
| #include "profiling_config.h" |
There was a problem hiding this comment.
Including profiling_config.h directly from task_interface inside a low-level header in common/log introduces a layering violation (dependency inversion). Low-level utilities like logging should not depend on higher-level modules like task_interface. This forces any target using common/log to include task_interface in its header search paths, which can break compilation for other targets or modules that do not use task_interface.
Additionally, using a flat include like #include "profiling_config.h" makes the dependency implicit and prone to header name collisions.
Suggested Improvements:
- Relocate the configuration: Move
profiling_config.h(or at least theSIMPLER_PROFILINGdefinition) to a lower-level common configuration directory (e.g.,src/common/log/include/common/or a dedicatedcommon/config/directory). - Use structured paths: If the dependency must remain, use a structured include path (e.g.,
#include "task_interface/profiling_config.h") and ensure that thecommon/logCMake target publicly propagates this dependency so that all consumers can resolve the include path.
| #include "profiling_config.h" | |
| #include "task_interface/profiling_config.h" |
ChaoWao
changed the title
ChaoWao
force-pushed
the
fix-strace-profiling-include
branch
2 times, most recently
from
8710b23 to
282319a
Compare
…d in c_api_shared)
strace.h gated its macros on `#if SIMPLER_PROFILING` but never included
profiling_config.h (the header that defines SIMPLER_PROFILING=1). It relied on
each TU to pull the definition in transitively — which the runtime pto_*.h
headers do, but the platform c_api_shared.cpp (which wraps simpler_run and owns
the root STRACE("simpler_run") / .bind / .runner_run / .validate spans) does
not. There, SIMPLER_PROFILING was undefined, the gate evaluated false, and every
STRACE compiled to a no-op.
Net effect: the host-trace markers for each simpler_run were silently dropped on
the platform path (the very path the DFX contract relies on — pypto-serving
reads per-stage timing of each simpler_run purely from these [STRACE] log
lines). This surfaced as a persistent CI failure in
examples/workers/l2/vector_add/test_run_timing.py::test_simpler_run_emits_strace_markers,
which asserts the markers are present.
Fix: make strace.h self-contained by including profiling_config.h itself, so
SIMPLER_PROFILING is always defined for any TU that uses the macros. One-line
include; no behavior change for TUs that already had it.
Verified: the previously-failing test now passes on a2a3sim; all 4 platforms ×
2 runtimes build clean; a2a3sim/a5sim hbg + tensormap scene tests pass.
ChaoWao
force-pushed
the
fix-strace-profiling-include
branch
from
282319a to
cd707ed
Compare
ChaoWao
force-pushed
the
fix-strace-profiling-include
branch
2 times, most recently
from
c24abfd to
cf035d6
Compare
Renames the configuration surface (compile macros + runtime env vars + DFX
flag bitmask) to a consistent scheme, and documents the rules in
docs/profiling-config-naming.md.
Rules (see the doc):
1. Compile macros are bare names; env vars carry a value-shape suffix
(_ENABLE / _LEVEL / _TYPE / _US / _MS) — readable as macro-vs-env.
2. Name reflects what's gated: umbrella is DFX (broad instrumentation),
sub-tiers keep PROFILING (they ARE profiling counters), host strace gate
is HOST_STRACE (not PROFILING).
3. Runtime-subsystem knobs carry an owner prefix (HBG_ / TMR_); platform
knobs do not.
4. Everything carries the SIMPLER_ project prefix.
Renames:
- PTO2_PROFILING -> SIMPLER_DFX (device DFX umbrella)
- PTO2_ORCH/SCHED/TENSORMAP_PROFILING -> SIMPLER_*_PROFILING (sub-tiers; PROFILING kept — accurate)
- SIMPLER_PROFILING -> SIMPLER_HOST_STRACE (host [STRACE] gate)
- SIMPLER_DEVICE_PROFILING -> SIMPLER_DEVICE_STRACE_ENABLE (env)
- PTO2_SERIAL_ORCH_SCHED -> SIMPLER_TMR_SERIAL_ORCH_SCHED_ENABLE (env)
- PTO2_OP_EXECUTE_TIMEOUT_US / STREAM_SYNC_TIMEOUT_MS / SCHEDULER_TIMEOUT_MS
-> SIMPLER_* (env; via the _ENV constexpr indirection)
- PROFILING_FLAG_* + GET/SET/CLEAR_PROFILING_FLAG
-> SIMPLER_DFX_FLAG_* + SIMPLER_GET/SET/CLEAR_DFX_FLAG
No behavior change. Verified: all 4 platforms x 2 runtimes build with -Werror;
the simpler_run [STRACE] marker test passes (the strace.h self-include fix is
preserved under the renamed SIMPLER_HOST_STRACE); a2a3sim/a5sim hbg scene
tests pass.
Includes docs/profiling-config-naming.md (new) and updates ci.yml + all
in-repo doc/skill references to the new names.
1 participant
Summary
Two changes (the branch now carries both):
1. Fix:
strace.hmust includeprofiling_config.h(the original, urgent bugfix)strace.hgated its macros on#if SIMPLER_HOST_STRACEbut never included the header that defines it (profiling_config.h). TUs not pulling in a runtimepto_*.h(notably the platformc_api_shared.cppwrappingsimpler_run) saw the macro undefined → the gate evaluated false → everySTRACEsilently compiled to a no-op, dropping thesimpler_runhost-trace markers. Fixed by makingstrace.hself-contained.2. Refactor: unify profiling/DFX/timeout config naming under
SIMPLER_Renames the configuration surface (compile macros + runtime env vars + DFX flag bitmask) to a consistent scheme, documented in docs/profiling-config-naming.md.
Naming rules:
_ENABLE/_LEVEL/_TYPE/_US/_MS).DFX(broad), sub-tiers keepPROFILING(they are counters), host-strace gateHOST_STRACE.HBG_/TMR_); platform knobs do not.SIMPLER_project prefix.Key renames:
PTO2_PROFILING→SIMPLER_DFX;PTO2_{ORCH,SCHED,TENSORMAP}_PROFILING→SIMPLER_*_PROFILINGSIMPLER_PROFILING→SIMPLER_HOST_STRACE;SIMPLER_DEVICE_PROFILING→SIMPLER_DEVICE_STRACE_ENABLEPTO2_SERIAL_ORCH_SCHED→SIMPLER_TMR_SERIAL_ORCH_SCHED_ENABLEPTO2_{OP_EXECUTE_TIMEOUT_US,STREAM_SYNC_TIMEOUT_MS,SCHEDULER_TIMEOUT_MS}→SIMPLER_*(env, via the_ENVconstexpr indirection)PROFILING_FLAG_*+GET/SET/CLEAR_PROFILING_FLAG→SIMPLER_DFX_FLAG_*+SIMPLER_GET/SET/CLEAR_DFX_FLAGNo behavior change.
Testing
-Werror.test_simpler_run_emits_strace_markerspasses (the[STRACE]fix holds under the renamedSIMPLER_HOST_STRACE).