Add #[inline] to the hot Kernel/Ctx/NanoTime accessors - #872
Open
0-jake-0 wants to merge 1 commit into
Open
Conversation
`nitro!`'s `compiled()` tier expands into the *downstream* crate — the derive emits `::wingfoil::op::Ctx::new`, `__ctx.time()`, `__ctx.wall_time()`, `__ctx.start_time()`, `__ctx.schedule(..)` and `__k.end_cycle(..)` — and that includes this repo's own benches, examples and integration tests, which link the lib as an extern crate. The per-node-per-cycle accessors those calls land on were concrete, non-generic `pub fn`s with no `#[inline]`, so nothing guaranteed they could be inlined across the crate boundary. `latency.rs` and `ops.rs` already annotate their hot leaves; this applies the same pattern to the kernel/ctx/time layer. Attributes only — no function body is touched, so the documented invariants around the two clocks are unchanged: `Ctx::wall_time` stays on `&self` (the `Cell` is what allows it), `Ctx::new` still leaves `wall_time: None` rather than copying the kernel's snap, and `begin_cycle` still only invalidates. `begin_cycle` is deliberately not annotated: it is large, it runs once per cycle rather than per node, and inlining it is not the ask. No workspace `[profile]` change either — profiles apply only to builds rooted at this workspace and never to downstream consumers, so LTO cannot address the reported problem. Closes #823
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.
What this changes
#[inline]on the small, monomorphic per-node-per-cycle accessors inop.rs,runtime/kernel.rsandruntime/time.rs, so a downstream crate compiling anitro!compiled()graph can inline them across the crate boundary insteadof emitting a real call.
Attributes only — not one function body is touched. The diff is 31 added
lines, all of them
#[inline].Why
Closes #823.
nitro!'scompiled()tier expands into the downstream crate: the deriveemits
::wingfoil::op::Ctx::new,__ctx.time(),__ctx.wall_time(),__ctx.start_time(),__ctx.schedule(..),__k.begin_cycle(..)and__k.end_cycle(..)at the call site. That includes this repo's own benches,examples and integration tests, which link the lib as an extern crate. Those
calls land on concrete, non-generic
pub fns that carried no#[inline], sonothing guaranteed they were inlinable across the boundary.
latency.rsandops.rsalready annotate their hot leaves; this applies the same pattern tothe kernel/ctx/time layer.
The two clocks are untouched
Every invariant CLAUDE.md names under "Two clocks: engine time and the wall
snap" still holds, because no body changed:
Ctx::wall_time()stays on&self— theCellis what allows that.Ctx::newstill leaveswall_time: Nonerather than copying the kernel'ssnap, so a cycle in which no op stamps still reads the clock zero times.
begin_cyclestill only invalidates the snap; it never takes one eagerly.What was annotated, and what was not
Annotated (
op.rs):Ctx::new,nested,time,wall_time,wall_time_precise,start_time,is_last_cycle,run_mode,schedule.Annotated (
kernel.rs):Kernel::start_time,time,wall_time,run_mode,is_last_cycle,schedule,end_cycle,mark.Annotated (
time.rs):NanoTime::now,from_nanos_u128_saturating, the fourMulimpls, the threeAddimpls,Sub, and the trivialFromconversionsu64 <-> NanoTimeandDuration <-> NanoTime.Deviations from the issue's list, in both directions. The line ranges were
treated as a guide, per the issue's own framing.
Added beyond the list, each on the same per-node-per-cycle path:
Ctx::nested— the island tier's constructor, built once per inner node peractivation. Exactly
Ctx::new's role for the other tier, and emitteddownstream by the same derive.
Ctx::start_time/Ctx::is_last_cycle/Ctx::run_mode— sibling scalargetters to
Ctx::time.start_timeis emitted directly by the derive;is_last_cycleandrun_modeare read fromcyclebodies that monomorphizedownstream (window/buffer flush, the run-mode-gated IO sinks).
Ctx::wall_time_precise— a one-line forwarder to the now-inlinedNanoTime::now, on the latency-stamping path.Kernel::start_time— called byCtx::newitself, so once per node percycle.
NanoTimeFromconversions foru64andDuration— each is a singlefield access or a
from_nanos, and they sit inside the annotated arithmetic(
Add<Duration>goes through both) and the kernel's bound checks. Thedisplay-oriented conversions (
f64,NaiveDateTime,pretty, the kdbtimestamp pair) are cold and were left alone.
Excluded although in or near the cited range:
Kernel::begin_cycle— left alone, as the issue asks. Large, and onceper cycle rather than per node.
[profile]/ LTO change. Profiles apply only to buildsrooted at this workspace and never to downstream consumers, so LTO cannot
address the reported problem.
Kernel::run_for,cycles,new,with_ready,build,set_spin,set_timer_policy,due— construction, configuration or reporting, not thehot path.
duein particular is only read by the in-crate interpretedrunner, never emitted downstream.
Kernel::drain_ready— per cycle, does a channel drain, and is reached onlyfrom
begin_cycle, which is staying out of line anyway.TimeQueue,Bucket) — they monomorphize downstream alreadyand need nothing.
One annotated item deserves a note:
Kernel::markis private, so thecross-crate argument does not apply to it. It is included because
#[inline]still enables cross-codegen-unit inlining within the crate (release defaults
to 16 CGUs), and it is small and runs once per due node per cycle. Happy to
drop it if a reviewer would rather keep the diff strictly to the pub surface.
No
#[inline(always)]anywhere. Plain#[inline]throughout; nothing hereneeded the stronger hint.
How it was verified
cargo fmt --allcargo lintandcargo lint-allcargo test -p wingfoil --all-features— 81 suites pass. The nine*_integrationsuites (aeron, etcd, fluvio, kafka, otlp, postgres,redis, zmq_cross_lang, zmq_etcd) fail with
failed to initialize a docker client: Socket not found: /var/run/docker.sock— no Docker in this sandbox, pre-existing andunrelated.
cargo test -p wingfoil-derivethis changes no behaviour.
Measurement
No timing improvement is claimed, because none could be measured here. The
honest reading follows.
Timing: the sandbox is too noisy to say anything. Building
--bench tiers(a bench target is a downstream crate running
compiled()graphs) andre-running the identical binary against its own criterion baseline gave:
dense_chain/compiledaccumulate/compileddense_chain/nestedaccumulate/nestedA ±10% swing with no code change at all is the noise floor of a shared cloud
VM. Any before/after number from this machine would be meaningless, so none is
reported.
Object code: a deterministic check that is not noise-sensitive. Comparing
nm -Con thetiersbench binary built before and after the change(rustc 1.94.1,
benchprofile, no LTO):Those two were emitted as real out-of-line functions in the downstream binary
before this change and are fully inlined away after it.
The rest of the issue's list never appears as an out-of-line symbol in either
build: on this toolchain rustc's
cross_crate_inlinableheuristic alreadyauto-exports MIR for sufficiently trivial non-generic functions, which covers
the plain field getters and the arithmetic impls.
Ctx::wall_timeandCtx::scheduleare precisely the two withmatchbodies that exceed thatthreshold — which is why they were the two still paying a call.
That is worth being clear about in both directions. It narrows the present
impact relative to the issue's framing, and it is the main argument for
landing the change anyway: the heuristic is an unstable implementation detail
with no guarantee attached. It varies by toolchain version, and a getter drops
out of it the moment its body grows a branch — silently, with no diagnostic.
#[inline]turns "currently inlined by luck" into "inlined by contract", andpins the two that were already losing that lottery.
Kernel::begin_cycleremains an out-of-line symbol in both builds, asintended.
Notes for the reviewer
The interesting judgement calls are
Kernel::mark(private — see above) andthe
NanoTimeFromimpls (in the arithmetic hot path but not named in theissue). Both are easy to drop if you disagree.
Worth knowing that the object-code comparison above is the reason this PR does
not lead with a percentage: the change is defensible as making an existing
guarantee explicit, not as a measured speedup.
Generated by Claude Code