feat(driver): add per-phase pipeline timing instrumentation#1741
Draft
ghaith wants to merge 4 commits into
Draft
feat(driver): add per-phase pipeline timing instrumentation#1741ghaith wants to merge 4 commits into
ghaith wants to merge 4 commits into
Conversation
Adds a small RAII timer (`PhaseTimer`) that records wall-clock time for each stage of `BuildPipeline` and for every participant invocation (`pre_index`, `post_index`, `pre_annotate`, `post_annotate`). The inner `ParsedProject::index` and `IndexedProject::annotate` methods self-time, so when a participant triggers an implicit whole-project re-index or re-annotate from inside one of its hooks, the cost shows up nested under the participant that caused it. The instrumentation is gated on `PLC_TIMING=1`. With the env var unset (the default, including CI), the timers compile to a near-noop and emit nothing — no behavioural change to existing tests, no diagnostic drift, no stdout/stderr noise. `PhaseTimer::new` takes a `&'static str` for static labels; `PhaseTimer::new_with(|| format!(...))` defers label allocation for dynamic labels so the instrumentation doesn't perturb the disabled-state baseline. `PipelineParticipant` and `PipelineParticipantMut` gain a `name()` method, used as the timer label. The default returns the implementing type's short name (module path and generics stripped); participants can override if needed. Documented in `book/src/development/phase_timing.md`. `scripts/oscat_multi_split.py` is bundled for benchmarking against multi-unit corpora. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps the link call in the codegen participant's `post_generate` hook with a `PhaseTimer`, so linking shows up as its own nested line under `generate (driver)` instead of being invisible inside that block. Extends `book/src/development/phase_timing.md` with two example traces — `plc build` (full pipeline, all four top-level driver scopes visible, with `link` nested under `generate (driver)`) and `plc check` (trace truncates at `annotate (driver)` because codegen and linking never run). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Build Artifacts🐧 Linux
From workflow run 🪟 Windows
From workflow run |
> _Drafted by Claude (Opus 4.7). Reviewer judgement applies as normal._
Builds on the timing instrumentation PR. Each PR's branch is on top of
the previous; review independently.
## What this does
Several post-annotate lowering participants used to unconditionally
re-annotate the whole project after their hook fired, regardless of
whether the project actually had any work for them. Each now guards
on an exact precondition against the global index. When the
precondition is false the participant returns the project unchanged
— no lowerer walk, no implicit re-pass.
Gates:
| Participant | Hook | Predicate |
|---|---|---|
| `PropertyLowerer` | `post_annotate` | any property declared |
| `InheritanceLowerer` | `post_annotate` | any FB / class with `EXTENDS`
or `IMPLEMENTS` |
| `AggregateTypeLowerer` | `post_annotate` | any POU returns an
aggregate (array, struct, string) |
| `PolymorphismLowerer` | `post_index` | any FB, class, method, or
interface |
| `PolymorphismLowerer` | `post_annotate` | any method, interface, FB /
class with `EXTENDS`, or user-declared `POINTER TO` / `REFERENCE TO` an
FB / class |
Each predicate is **exact** for the participant it gates: when it
returns false, the lowerer would walk the project and emit zero
changes, and the implicit re-annotate would reproduce the existing
state.
## The polymorphism split
The two polymorphism hooks use different predicates on purpose.
- `post_index` runs the **table pass**, which adds a `__vtable` type
+ `__vtable` member to every FB and class. The slot is part of
the FB ABI: a downstream library consumer that extends one of
these FBs must see a layout-compatible base, regardless of
whether the project itself dispatches polymorphically. So the
table pass fires for any FB / class / method / interface, even
in a pre-OOP library that declares only methodless FBs.
- `post_annotate` runs the **dispatch pass**, which rewrites call
sites that need vtable indirection: method calls through the
vtable, body calls through `POINTER TO Base`, calls through
interface variables, `SUPER^`. A pre-OOP library that uses only
standalone FBs (no methods, no `EXTENDS`, no interfaces, no
pointer-to-FB members) has nothing to rewrite even though its FBs
ship with vtable slots. So the dispatch pass uses a strictly
tighter predicate.
The `POINTER TO` / `REFERENCE TO` clause is essential for library
safety: a lib that declares `VAR ptr : POINTER TO FB_B; END_VAR`
and calls `ptr^()` may not have any methods or inheritance in its
own compilation, but a downstream consumer can retarget that
pointer to `FB_B_DERIVED`. The dispatch call site must be
vtable-indirected so the derived body runs. Compiler-synthesized
pointers — vtable body slots (`is_function: true`) and internal
`__auto_pointer_to_X` types — are excluded from the check, so
oscat-shaped libraries that have no user pointers don't trip the
predicate.
The split means a methodless pre-OOP library without `POINTER TO`
FB usage pays the table-pass cost (necessary, ABI-shaping) but skips
the dispatch pass (no-op for its shape).
## Regression tests
Five new lit tests guard the boundaries the dispatch gate is
protecting. All use `printf` markers so the dispatched body is
directly observable.
**The gate skips when it should** — concrete-typed FB member calls
must resolve to the static type's body, even when that type is
extended elsewhere in the compilation:
- **`concrete_member_call_not_dispatched/`** — single compilation
unit with `FB_A` holding a concrete `FB_B` member, plus
`FB_B_DERIVED EXTENDS FB_B`. `FB_A`'s `helper()` must run `FB_B`'s
body.
- **`concrete_member_call_not_dispatched_external/`** — two
separately-compiled units. The lib is built in isolation into
`libnopoly.so` with no derivation in scope; the app declares the
lib's FBs as `{external}` and adds `FB_B_DERIVED`. The pre-compiled
`FB_A` inside the .so must still call `FB_B`'s body.
**The gate fires when it should** — a pre-OOP library that exposes
FBs by pointer reference must still dispatch polymorphically, because
a downstream consumer can derive and retarget:
- **`pointer_to_fb_in_pre_oop_lib_dispatches/`** — lib compiled in
isolation with `VAR inst_fb_b : POINTER TO FB_B; END_VAR` and
`inst_fb_b^()`. App extends `FB_B` and retargets the pointer to a
derived instance. The lib's `FB_A.body`, compiled with no
derivation in scope, must still dispatch through the vtable so
the derived body runs.
- **`reference_to_fb_in_pre_oop_lib_dispatches/`** — same shape with
`REFERENCE TO FB_B`. `REFERENCE TO` is encoded as a
`Pointer { auto_deref: Some(_) }`, so the same check covers it.
**Known gap, documented** — `VAR_IN_OUT FB` parameters are *not*
currently dispatched polymorphically. This is a pre-existing
limitation in the polymorphism dispatch pass itself
(reproducible on a clean `master` build, unrelated to this PR), and
is tracked in #1743:
- **`var_in_out_fb_in_pre_oop_lib_dispatches/`** — marked `XFAIL: *`,
with the run.test header pointing at #1743. Removing the `XFAIL`
directive once #1743 is fixed flips it into a passing regression.
Included now so the gap doesn't get forgotten.
The two-compilation variants (`_external/`, `pointer_to_fb`,
`reference_to_fb`, `var_in_out_fb`) all use the same
`extern_st_polymorphism`-style `lit.local.cfg`: the library is
pre-built into a `.so` with an isolated compilation, then the app is
linked against it.
## Implication
Projects that don't use a given OOP feature stop paying for that
feature's lowering re-pass. Pre-OOP FB libraries — the common shape
for legacy PLC code — skip every post-annotate gate except
`AggregateTypeLowerer` (most legacy code does have STRING-returning
functions). Projects that exercise every feature see no change.
The cost of each precheck is one short loop over the index with an
early return on the first hit. The dominant cost on the hot path
was the lowerers themselves; the gates remove that cost when it
isn't earning anything.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| } | ||
|
|
||
| fn parse(&mut self) -> Result<ParsedProject, Diagnostic> { | ||
| let _t = PhaseTimer::new("parse"); |
Contributor
There was a problem hiding this comment.
Already chatted with @ghaith about this, but this pattern is quite strange and probably merits a rethink when we get back to this PR.
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
2 participants
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.
What this does
Adds a
PhaseTimerthat records wall-clock time for each stage ofBuildPipeline(parse,index,annotate,generate) and forevery participant invocation (
pre_index,post_index,pre_annotate,post_annotate). Enabled byPLC_TIMING=1; off bydefault.
ParsedProject::indexandIndexedProject::annotateare self-timed,so when a participant triggers an implicit whole-project re-index or
re-annotate from inside its hook, the cost shows up nested under the
participant that caused it.
How to use
Each phase logs its elapsed time on drop, indented by nesting depth.
The chapter at
book/src/development/phase_timing.mdshows sampletraces for both
plc build(full pipeline including codegen andlink) and
plc check(front-end only — trace stops atannotate),and explains how to add new timed scopes.
Implication
This gives us a quantitative trace of where time is spent in the
front end. The most actionable signal is the nesting: a participant
that produces an inner
index/annotateline is silently doing awhole-project re-pass from within its hook. Some of those are
unavoidable; many aren't, and they're worth a follow-up.
🤖 Generated with Claude Code