perf: Speed up type-erased SourceLink unpacking - #5829
Draft
andiwand wants to merge 3 commits into
Draft
Conversation
Unpacking a `SourceLink` back to its concrete type is on the track finding hot path: the calibrator, the measurement selector and the surface accessor each do it for every measurement candidate. The payload is stored inline in the small buffer, so this should be a type check and a load, but three avoidable costs made it an out-of-line call instead. - `typeHash<T>()` was a runtime value cached in a function-local static, so every call site paid a thread-safe-static guard check, and the hash not being a constant kept `as<T>()` from being inlined. It is now computed at compile time from `std::source_location::function_name()`, which embeds the template arguments on both GCC and Clang. A `static_assert` on two probe types turns a compiler that does not do this into a build failure rather than silently colliding hashes. - `dataPtr()` loaded `m_handler->heapAllocated` and branched on it, even though `as<T>`/`asPtr<T>` know `T` statically. They now use a typed `dataPtrFor<T>()` with `if constexpr`, dropping a dependent load. - The `throw` sat inline in `as<T>()`. It moved into a cold, out-of-line helper so the accessor stays small enough to inline. Two follow-ons come for free once the hash is constant: the per-type `Handler` singleton is now constant-initialized, which removes the `__cxa_guard_acquire` from every `SourceLink` construction, and the type check leads with a pointer comparison against that singleton, keeping the hash comparison as a fallback in case the handler is duplicated across shared objects. Also adds unpacking cases to `ActsBenchmarkSourceLink`, which previously only covered construction and copying and so could not show any of this.
andiwand
force-pushed
the
perf-sourcelink-unpack
branch
from
August 4, 2026 13:38
580ceea to
8b6780f
Compare
Contributor
Public API surface diff+0 added, 1 breaking.
|
Member
|
Does this work across compilers? I was previously using a macro based compile time solution and that did not. |
andiwand
marked this pull request as draft
August 4, 2026 14:56
The previous commit made `typeHash<T>()` a compile-time value derived
from `std::source_location::function_name()`. That string is not the
same on GCC and Clang, not even for a plain `struct Foo`:
clang: std::string_view Acts::detail::typeIdentity() [T = Foo]
gcc: constexpr std::string_view Acts::detail::typeIdentity()
[with T = Foo; std::string_view = std::basic_string_view<char>]
so the hashes differed per compiler, where the previous
`typeid(T).name()` basis was stable because the Itanium ABI fixes the
mangling. The probe `static_assert` could not catch this: it only
checked that two types do not collide, not that the string is
compiler-independent. Normalising the string does not help either, the
spellings still differ (`const char *` vs `const char*`).
`typeHash<T>()` therefore goes back to what it was on main, and the
probe translation unit is dropped.
The compile-time hash was only needed so the per-type `Handler`
singleton could be constant-initialized. `Handler` already carries a
`const std::type_info*`, and `&typeid(T)` is a constant expression, so
dropping the `typeHash` field keeps `makeHandlerValue<T>()` constexpr
and the singleton free of a thread-safe-static guard. The `holds<T>()`
fallback now compares `type_info` directly, which is the same check the
hash of the mangled name was standing in for.
`throwBadAnyCast()` moves out of the class into `Acts::detail` and is
defined in Any.cpp, so it is out of line by linkage rather than by
`[[gnu::noinline, gnu::cold]]`. No compiler-specific attributes remain,
only the standard `[[likely]]`.
The `_ACTS_ANY_DEBUG` handler dump is restored, in `makeHandler<T>()`
rather than in the constant-evaluated `makeHandlerValue<T>()`, and only
compiled in when debug output is enabled.
Hot-path codegen is unchanged and identical on Clang 17 and GCC 15:
four instructions before the load, cold path out of line. The only
measurable difference against the previous commit is `getPtr<T>`, which
returns to its main level because the fallback now contains a call and
`asPtr` is no longer a leaf. `get<T>`, the path track finding takes, is
unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014JbikxN3YQZGjqzZiCZKRa
Member
|
We can try the type id equality again and revisit in case we get complaints |
GCC 15 rejects the trivial copy/move paths in `AnyBase` when the stored
type is empty:
Any.hpp:721: error: '<anonymous>.Acts::AnyBase<512>::m_data' may be
used uninitialized [-Werror=maybe-uninitialized]
reached through `MagneticFieldProvider::Cache` holding a
`ConstantBField::Cache`, which is an empty class. Placement-new of an
empty type writes nothing, so `m_data = std::move(fromAny.m_data)` copies
a buffer none of whose bytes were ever written.
This is not a compiler defect. The read is real, and it is also on main:
there the per-type `Handler` is initialised by a lambda at first use, so
GCC cannot fold `heapAllocated` and `moveConstruct == nullptr` and never
reaches the array copy. Constant-initializing the handler exposed it. It
is not undefined either -- `m_data` is `std::byte`, so the copied values
are unspecified rather than indeterminate, and they are never read as a
value. GCC draws the line at a buffer where *nothing* was written; a
single `int` payload in the same 512-byte buffer does not warn, nor does
one with padding.
An empty object still occupies one byte of the buffer, so writing that
byte before the object's lifetime starts restores the property the copy
paths already rely on for every other type. Guarded by
`if constexpr (std::is_empty_v<T>)`, so nothing is emitted for any type
that carries state, and the empty case gets a single byte store next to
a placement-new that generates no code. The trivial copy/move paths keep
their fixed-size array assignment and their codegen is untouched.
`ActsUnitTestAny` passes and `ActsBenchmarkSourceLink` is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DiFkbFFfQtQxedBUrMwnCb
|
Contributor
|
|
||
| h.typeHash = typeHash<T>(); | ||
| h.typeInfo = &typeid(T); | ||
| h.typeInfo = &typeid(T); |
Member
There was a problem hiding this comment.
I thought typeid is not constexpr?
Contributor
Author
There was a problem hiding this comment.
typeid is like sizeof I think. it returns a const std::type_info&
std::type_info itself does not provide constexpr methods in C++20 but we don't depend on that here
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.
Unpacking a
SourceLinkback to its concrete type is on the track finding hot path: the calibrator, the measurement selector and the surface accessor each do it for every measurement candidate. The payload is stored inline in the small buffer, so this should be a type check and a load, but three avoidable costs made it an out-of-line call instead.Handlersingleton was initialised by a lambda at first use, so everySourceLinkconstruction paid a thread-safe-static guard check, andas<T>()compared against atypeHash<T>()that was itself a guarded function-local static.Handleris now built by aconstexprfunction and the singleton is constant-initialized, which removes both guards.typeHash<T>()is gone from the type check entirely: the check leads with a pointer comparison against that singleton and falls back to comparingtype_info, which covers handlers duplicated across shared objects.dataPtr()loadedm_handler->heapAllocatedand branched on it, even thoughas<T>/asPtr<T>knowTstatically. They now use a typeddataPtrFor<T>()withif constexpr, dropping a dependent load.throwsat inline inas<T>(). It moved intoActs::detail::throwBadAnyCast(), declared in the header and defined inAny.cpp, so it is out of line by linkage and the accessor stays small enough to inline. No compiler-specific attributes are used, only the standard[[likely]].Also adds unpacking cases to
ActsBenchmarkSourceLink, which previously only covered construction and copying and so could not show any of this.--- END COMMIT MESSAGE ---
Numbers
Measured on both compilers, since the whole point of the type check is that it should not depend on one.
ActsBenchmarkSourceLinklinkslibActsCore, so a GCC binary cannot be linked against a Clang build of it; the numbers below come from the same benchmark source rebuilt as a standalone translation unit, minus the twoVectorMultiTrajectorycases. Same flags (-O2 -DNDEBUG), only the header tree swapped between main and this branch, macOS arm64. Three repeats each, stable to ±0.01 ns on Clang and ±0.03 ns on GCC. New benchmark cases in bold.Apple clang 17, ns/iteration:
get<T>getPtr<T>GCC 15.1, ns/iteration:
get<T>getPtr<T>Absolute values are not comparable across the two tables — the compilers generate different code for the benchmark harness itself, which is why GCC's
createbaseline already sits at 0.58 where Clang's is 1.01. The before/after within each compiler is the meaningful part. The twoVectorMultiTrajectorycases, measured separately with the in-tree benchmark under Clang, are unchanged: create track state 15.9 -> 15.6, assign source link to track state 16.4 -> 16.2.Two asymmetries worth calling out:
getPtr<T>does not improve under Clang. Thetype_infofallback contains a call, soasPtrstops being a leaf function and lands back at its main level. GCC improves it 4.3x like everything else. An earlier iteration of this PR, which keyed the check on aconstexpr typeHash<T>(), reached 0.55 there under Clang; that is what thetype_infocomparison gives back, and it only shows up on one compiler.get<T>, which is the path track finding actually takes, is unaffected on both.createbarely moves under GCC (0.58 -> 0.55) where Clang gains 1.3x, because GCC was already folding away most of the guard.as<T>()previously did not inline at all — the call sites emitted abltoAnyBase::as<IndexSourceLink>. It now inlines to five instructions, and the hot path is identical on both compilers:The 0.30 ns on GCC is low enough to suspect the check was hoisted out of the loop, so I checked: feeding a 50/50 mix of two stored types instead of one costs 2.46 ns on GCC and 3.57 ns on Clang, 8x and 5x the all-matching case. The compare really does run every iteration, it is just perfectly predicted.
That mixed case also shows the one regression this change carries: when the type check fails, Clang is about 0.8 ns slower than main (2.75 -> 3.57), because the fallback is now a
type_infocomparison with a call rather than an inline hash compare. GCC is still faster there (3.31 -> 2.46). This only matters for code running failingis<T>()/getPtr<T>checks in a hot loop, which is not a pattern on the track finding path.In a benchmark shaped like the CKF per-candidate sequence (adapter-iterator construct, calibrator copy plus unpack, then the
stayOnSeedread-back plus unpack) this is 5.5 ns -> 3.2 ns per candidate, about 2.3 ns saved. Against roughly 25 ns per candidate oncemakeTrackStateis included that is around 9% of the per-candidate track state creation work.Verified with
AnyTests,AnyDebug,AnyGridView,AnyTrackProxy,AnyTrackStateProxy,SourceLink,MultiTrajectoryandDataHandleon Clang; the headers andAnyTestsadditionally compile clean under GCC 15.A note on
typeHash<T>()typeHash<T>()is unchanged from main. It was briefly madeconstexprviastd::source_location::function_name(), but that string differs between compilers even for a plainstruct Foo:Normalising it does not help, the spellings still differ (
const char *vsconst char*). The existingtypeid(T).name()basis is stable across compilers because the Itanium ABI fixes the mangling — verified identical hashes under Clang 17 and GCC 15 forFoo,ns::Nested::Inner,Wrap<Foo>,intandconst char*.Anyno longer needs a compile-time hash at all, since&typeid(T)is already a constant expression and is what the handler stores.Possible follow-up:
const SourceLink&accessorAfter this change, copies rather than unpacks dominate what is left: of the remaining ~3.1 ns per candidate, ~1.6 ns is two
SourceLinkcopies. A large part of that is thatgetUncalibratedSourceLink()returns by value all the way down (TrackStateProxy->MultiTrajectory-> backend), so every read copies. Measured onVectorMultiTrajectoryin the shape ofisSeedCandidate, adding a reference accessor gives:getUncalibratedSourceLink()(by value)const SourceLink&)With one to two such reads per candidate (
isSeedCandidatefor every candidate understayOnSeed,copyFromfor every selected one) that is a further ~0.8-1.6 ns/candidate, comparable to what this PR saves.This cannot be a straight signature change though.
MultiTrajectoryBackendConceptrequiresstd::same_as<SourceLink>, so it would break every backend including out-of-tree ones; the Podio backend materialises the source link by scanning a link collection and has no stored object to reference; and callers writingconst auto& sl = ts.getUncalibratedSourceLink();would go from a lifetime-extended temporary to a reference into storage that dangles if the container reallocates. An opt-in backend hook selected withif constexpr, falling back to the by-value path, would avoid all three. Happy to follow up with that separately if there is interest.🤖 Generated with Claude Code