Follow-ups to #593: iterator interop, checked Expr::at(), hash invalidation - #594
Conversation
ExprIteratorImpl's heterogeneous operator-/operator==/operator<=> read `other.ptr_` of the *other* specialization, which is private and had no friend declaration, so every one of them was a hard error the moment it was instantiated. Nothing in tree mixed the two iterator types, so this went unnoticed: `expr.begin() != expr.cend()`, or pairing sequant::cbegin(ExprPtr const&) with sequant::end(ExprPtr&), failed to compile. There was also no ExprIterator -> ConstExprIterator conversion. - befriend all ExprIteratorImpl specializations and collapse each pair of <is_const>/<!is_const> overloads into a single member template - add a converting constructor (mutable -> const only); it is a constructor template so that it is never treated as a copy constructor - drop `operator-(difference_type, ExprIteratorImpl)`: `n - it` is not a valid random-access-iterator expression, and it silently computed `it - n` (the `n + it` counterpart is valid and stays) - pin all of the above down with static_asserts next to the existing random_access_iterator ones
Dropping ranges::view_interface also dropped its at(), which threw when the index was out of range. The replacement forwards to operator[], whose only guard is SEQUANT_ASSERT -- a no-op unless SEQUANT_ASSERT_ENABLED is #defined. So in a build configured with SEQUANT_ASSERT_BEHAVIOR=IGNORE, `sum.at(p)` (e.g. optimize/sum.cpp:118) degraded from a thrown exception to an out-of-bounds read returning a garbage ExprPtr&. The parameter also went from a signed difference type to std::size_t, so at(-1) went from throwing to wrapping to SIZE_MAX. back() had the same problem from the other end: at(size() - 1) on an empty Expr -- every atom -- computes at(SIZE_MAX). at() now always checks and throws sequant::Exception; the cold throwing path stays out of line so it does not bloat callers. operator[] keeps its assert-only check, now documented as unchecked. N.B. the exception type is sequant::Exception rather than the std::out_of_range the range-v3 at() used to throw. Nothing in tree catches std::out_of_range from here, and sequant::Exception is the project convention.
begin/end/cbegin/cend/size/empty/operator[]/at/front/back are one-line forwarders on the hottest paths in the library. Expr::is_atom() alone is called from visit_impl(), is_scalar(), is_cnumber(), ExprRange::next_atom() and the Wick/canonicalization code, and out-of-line definitions turn each of these into a non-inlinable cross-TU call on top of the virtual dispatch they already pay for. Also make empty() compare begin/end rather than compute size() == 0, saving a pair of virtual calls. This is a restoration, not a new decision: before the switch away from ranges::view_interface these were all header-inline (as CRTP base templates), and is_atom() was `ranges::empty(*this)`. Measured, Release/-O3, SEQUANT_ASSERT_BEHAVIOR=IGNORE, Apple clang 17, against the identical tree with these bodies moved back to expr.cpp, 3 interleaved rounds of 3 repetitions each, comparing per-benchmark medians over sequant_benchmarks (canonicalize, simplify, rapid_simplify, spintrace, tensor_block, random_tensor_network): 53 of 53 benchmarks faster; median -2.29%, mean -2.26% best -4.31% (rapid_simplify/1), worst -0.09% No code-size cost -- the trivial bodies inline away rather than bloat: libSeQuant-symb.a 7437952 B inline vs 7441224 B out-of-line sequant_benchmarks 4985824 B inline vs 4986816 B out-of-line Mechanism, for the record: out-of-line, libSeQuant-symb.a alone carries 34 undefined cross-TU references to these accessors and emits 16 definitions for them; inline, it has zero of either. N.B. cc_full_derivation was excluded from the benchmark set -- it segfaults, but it does so on unmodified master too, so it is unrelated to this branch.
Product::end_cursor() used to call reset_hash_value(); the end_subexpr() that replaced it does not, so `*(--product.end()) = new_factor` mutates a factor while leaving the memoized hash in place. That trips the `*hash_value_ == compute_hash()` assert in Product::memoizing_hash(), and with asserts disabled leaves a stale hash that makes static_equal() short-circuit to false for products that are in fact equal. Sum::begin_subexpr() gained the reset the old Sum::begin_cursor() never had, which is the right call -- handing out a mutable iterator into the storage has to invalidate the hash. Give Sum::end_subexpr() the same treatment so both ends of both containers agree.
Regression tests for the three fixes in this branch. Each fails without its fix: - "mixed const/non-const iteration" does not compile at all without the friend declaration and the converting constructor - "checked element access" fails in a build with SEQUANT_ASSERT_BEHAVIOR=IGNORE, where at() used to read out of bounds instead of throwing (with asserts enabled the assert masks it) - "hash invalidation on mutable iteration" fails on both Product and Sum when only begin_subexpr() resets the memoized hash
1ac05e4 to
b20c46d
Compare
|
I don't think it is good practice to move implementations into header files unless crucial for performance. Enabling LTO seems like the better way to address potential performance bottlenecks from these sorts of things 🤔 |
There was a problem hiding this comment.
Pull request overview
This PR follows up on #593’s shift of Expr to a proper range by fixing const/non-const iterator interoperability, restoring checked element access semantics for Expr::at()/front()/back(), and ensuring memoized hashes are invalidated when handing out mutable iterators (including from end()), with regression tests for each.
Changes:
- Fix
ExprIteratorImplconst/non-const interop (conversion + heterogeneous comparisons/differences) and remove invalidn - itoperator. - Restore always-checked
Expr::at()(andfront()/back()) that throwssequant::Exceptionon out-of-range access, independent ofSEQUANT_ASSERT. - Invalidate memoized hash when returning mutable iterators from
Sum::end_subexpr()andProduct::end_subexpr(), plus add unit coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_expr.cpp | Adds regression tests for mixed const/non-const iteration, checked access throwing, and hash invalidation on mutable iteration. |
| SeQuant/core/expressions/sum.hpp | Resets memoized hash when handing out mutable iterators from end_subexpr(). |
| SeQuant/core/expressions/product.hpp | Resets memoized hash when handing out mutable iterators from end_subexpr(). |
| SeQuant/core/expressions/expr.hpp | Moves hot accessors inline; implements checked at() throwing Exception; documents unchecked operator[]. |
| SeQuant/core/expressions/expr.cpp | Removes out-of-line accessor bodies; adds out-of-line throwing helper to avoid inlining cold paths. |
| SeQuant/core/expressions/expr_iterator.hpp | Fixes iterator const/non-const interop and removes invalid n - it overload; adds concept/static-assert checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
LTO is tricky, especially in packaging scenarios: both SQ and consumer would need to be built with LTO flags, cross-package LTO is reportedly fragile. |
expr.cpp uses std::ostringstream in throw_out_of_range() and in
not_implemented(), but never included <sstream>. It compiled only
because the header arrives transitively at depth 6, via
algorithm.hpp -> container.hpp -> hash.hpp
-> boost/container_hash/hash.hpp -> <complex> -> <sstream>
i.e. through a Boost header and a libstdc++ implementation detail
(<complex> is not required to include <sstream>). Either link can be
dropped by an upgrade at any time.
|
I can create a LTO PR. However, I would consider it cleaner to move the implementations back into the CPP file. Alternatively, I could make moving them to the CPP file part of my expr impl cleanup PR, if you prefer. I guess from the benchmark results here, we should run some benchmarks before merging that PR anyway 👀 |
|
Yes, LTO PR would be good.
This sounds good. |
Conflict resolution notes: - expr.hpp / expr.cpp: master (#589) keeps the Expr accessors out of line, so the inlining from 27f0225 is dropped here, per the PR discussion (Krzmbrzl's cleanup PR owns the inline/out-of-line question, to be revisited together with LTO). The checked at()/front()/back() semantics are kept, with the bounds check and throw_out_of_range() in expr.cpp. - sum.hpp / product.hpp: master moved the *_subexpr() definitions into sum.cpp / product.cpp; the end_subexpr() hash invalidation moves with them.
That PR has now already been merged so I would suggest this PR leaves the implementation in the CPP file, we merge the LTO PR and then do benchmarks on master and compare to the results for this PR (after having merged in to latest master, including LTO). |
This reverts commit 27f0225. Per the discussion on #594: the Expr accessors stay out of line in expr.cpp. The inline-vs-out-of-line question is revisited once the LTO PR lands, by benchmarking master against this branch rebuilt on top of it -- not here. Deliberately empty: the merge with master (d050240) already resolved expr.hpp/expr.cpp to master's out-of-line layout, since #589 restructured both files. There is nothing left to reverse; this commit records the decision where `git log` will show it.
Agreed, and already the case — I merged master in before seeing this, and #589's restructuring made it the natural resolution anyway. The whole diff against master is now 162 insertions over 6 files, and the only additions to Section 4 of the description is kept, marked as dropped, so the numbers are still around as the baseline for the comparison after the LTO PR lands. Conflict resolution, for the record:
Re-ran the suite on the merged tree: Debug/ One thing worth splitting off, unrelated to this PR: |
|
Split the |
Product::factors() non-const hands out a mutable reference to factors_ without invalidating the memoized hash. Mutating a factor through it leaves a stale hash in place, which trips the SEQUANT_ASSERT(*hash_value_ == compute_hash()) in Product::memoizing_hash() when asserts are enabled, and with SEQUANT_ASSERT_BEHAVIOR=IGNORE silently returns the stale value, making static_equal() short-circuit to false for Products that are in fact equal. This is the same defect that #594 fixes for begin_subexpr() and end_subexpr(); factors() is the remaining hole. No caller in tree mutates through the reference today, so the bug is latent rather than live. The reset is deliberately *not* guarded by `!factors_.empty()` the way begin_subexpr()/end_subexpr() are: those hand out iterators, and an empty range yields nothing to dereference, whereas a caller can grow an empty factors_ through this reference. Both cases are covered by tests; the second fails if the guard is added. Sum needs no equivalent change -- it exposes summands() as const only.
Follow-up to #593, which turned
Exprinto a proper range. Three defects that CI could not catch, plus regression tests for each.Merged with master and the perf commit dropped. #589 landed in the meantime and restructured
expr.hpp/expr.cpp; the merge resolves them to master's out-of-line layout, so the accessor inlining from27f02258is no longer part of this PR (reverted inbd4d00d9). The three correctness fixes are untouched. Section 4 below is kept as the record for the later LTO comparison.1. const/non-const iterators do not interoperate at all
ExprIteratorImpl's heterogeneousoperator-/operator==/operator<=>readother.ptr_of the other specialization, which is private, with nofrienddeclaration anywhere. Every one of those overloads is a hard error the moment it is instantiated:Nothing in tree currently mixes the two types, which is why master builds. But
expr.begin() != expr.cend()does not compile, and neither does pairingsequant::cbegin(ExprPtr const&)(expr_algorithms.hpp:65) with the non-constsequant::end(ExprPtr&)(expr_algorithms.hpp:78). There was also noExprIterator→ConstExprIteratorconversion, so the const/non-const interop these overloads exist to provide was entirely non-functional.Fixed by befriending all specializations, collapsing each
<is_const>/<!is_const>overload pair into one member template, and adding a converting constructor (mutable → const only; it is a constructor template so it is never treated as a copy constructor). Also droppedoperator-(difference_type, ExprIteratorImpl)—n - itis not a valid random-access-iterator expression and it silently computedit - n. The validn + itcounterpart stays.2.
Expr::at()lost its bounds checkDropping
ranges::view_interfacealso dropped itsat(), which threw when the index was out of range. The replacement forwards tooperator[], whose only guard isSEQUANT_ASSERT— a no-op unlessSEQUANT_ASSERT_ENABLEDis#defined. In a build configured withSEQUANT_ASSERT_BEHAVIOR=IGNORE,sum.at(p)(optimize/sum.cpp:118) degrades from a thrown exception to an out-of-bounds read returning a garbageExprPtr&. The parameter also changed from a signed difference type tostd::size_t, soat(-1)went from throwing to wrapping toSIZE_MAX.back()had the same problem from the other end:at(size() - 1)on an emptyExpr— every atom — computesat(SIZE_MAX).at()now always checks and throwssequant::Exception(project convention; nothing in tree catchesstd::out_of_rangefrom here). The throwing path is out of line so it does not bloat callers.operator[]keeps its assert-only check, now documented as unchecked.3.
Product::end_subexpr()no longer invalidates the memoized hashProduct::end_cursor()used to callreset_hash_value(); theend_subexpr()that replaced it does not. So*(--product.end()) = new_factormutates a factor while leaving the memoized hash in place — which trips the*hash_value_ == compute_hash()assert inProduct::memoizing_hash(), and with asserts disabled leaves a stale hash that makesstatic_equal()short-circuit tofalsefor products that are in fact equal.Sum::begin_subexpr()gained the reset the oldSum::begin_cursor()never had, which is the right call.Sum::end_subexpr()gets it too, so both ends of both containers agree.4. Hot accessors moved back into the header — dropped from this PR
begin/end/cbegin/cend/size/empty/operator[]/at/front/backare one-line forwarders on the hottest paths in the library.Expr::is_atom()alone is called fromvisit_impl(),is_scalar(),is_cnumber(),ExprRange::next_atom()and the Wick/canonicalization code; out-of-line definitions turn each into a non-inlinable cross-TU call on top of the virtual dispatch they already pay for.empty()now comparesbegin/endrather than computingsize() == 0, saving a pair of virtual calls.It was a restoration, not a new decision — before the switch away from
ranges::view_interfacethese were all header-inline (as CRTP base templates), andis_atom()wasranges::empty(*this).Measured rather than asserted. Release/
-O3,SEQUANT_ASSERT_BEHAVIOR=IGNORE, Apple clang 17, against the identical tree with these bodies moved back toexpr.cpp— 3 interleaved rounds of 3 repetitions, comparing per-benchmark medians oversequant_benchmarks(canonicalize,simplify,rapid_simplify,spintrace,tensor_block,random_tensor_network):rapid_simplify/1) / −0.09%No code-size cost — the trivial bodies inline away rather than bloat:
libSeQuant-symb.asequant_benchmarksMechanism, for the record: out-of-line,
libSeQuant-symb.aalone carries 34 undefined cross-TU references to these accessors and emits 16 definitions for them; inline it has zero of either.cc_full_derivationwas excluded from the benchmark set: it segfaults, but it does so on unmodified master (3168a655) too, so it is unrelated to this branch — reported separately.Verification
Each new test fails without its fix:
mixed const/non-const iteration'ptr_' is a private member,no viable conversion)checked element accessREQUIRE_THROWS_AS(sum->at(2), Exception)fails withSEQUANT_ASSERT_BEHAVIOR=IGNOREhash invalidation on mutable iterationProductandSumFull unit suite run locally in two configurations, re-run after the merge with master:
SEQUANT_ASSERT_BEHAVIOR=THROW: 6822 assertions in 62 test cases, all passedSEQUANT_ASSERT_BEHAVIOR=IGNORE: 310982 assertions in 63 test cases, all passedBoth regression tests were re-confirmed against the merged tree by reverting each fix in the Release/
IGNOREbuild:checked element accessandhash invalidation on mutable iterationfail, and pass again once restored.CI is green on all 16 checks — GCC 14 and clang, Debug and Release, sanitizers, Valgrind.