Skip to content

Use ArrayOfVectorViews to back per-quadpoint state storage - #105

Open
knutambot wants to merge 1 commit into
KnutAM:mainfrom
knutambot:cb/aovv-states
Open

knutambot wants to merge 1 commit into
KnutAM:mainfrom
knutambot:cb/aovv-states

Conversation

@knutambot

Copy link
Copy Markdown
Contributor

Summary

PR #66 proposed backing StateVector's Dict{Int,SV} with Ferrite's ArrayOfVectorViews to cut per-cell allocations and replace dict-lookup with array indexing. That PR (Dec 2025) is stale and doesn't apply to the current states.jl, which has grown a documented AbstractDict-like interface (keys/values/pairs/haskey/get), copy_state/copy_state! mutation-identity guarantees, and facet-domain support that PR #66 never accounted for. This reimplements the same idea from scratch against the current code.

  • StateVector{SV,VV} is now backed by dense, cell-number-indexed storage instead of Dict{Int,SV}. A Vector{T}-per-cell state (the common "one value per quadrature point" case) is flattened into a single shared Ferrite.CollectionsOfViews.ArrayOfVectorViews per old/new — avoiding one small Vector allocation per cell — with getindex returning a SubArray view instead of a Vector. Any other per-cell state (scalar struct, Nothing, or a non-Vector AbstractArray) uses a plain dense Vector{SV}.
  • The public getindex/setindex!/get still check domain membership and throw KeyError like a Dict would. An internal, unchecked _raw_getindex/_raw_setindex! serves the assembly-hot path (get_state/get_old_state, _copy_states!) — this is exactly the lookup overhead being removed, so it's kept off that path rather than folded into the public accessors.
  • _copy_states! distinguishes packed storage (always safe to mutate element-wise, since the shared flat buffer is always ours and mutable) from non-packed AbstractArray values, which keep the original ismutable-based classification — so a user's own create_cell_state returning a SubArray directly still goes through copy_state/copy_state! as a whole value, unaffected by this change.

How this differs from PR #66

PR #66 added a separate inds::Vector{Int} cellnr→compact-row translation layer, keeping storage proportional to the domain's own cell count. This PR indexes directly by cell number instead (dense over the whole grid — no translation layer), matching the pattern already used in Workers/QuadPointEvaluator.jl. Trade-off: for a domain that's a strict subset of a larger multi-domain grid, this uses O(grid cells) index/slot overhead rather than O(domain cells) (cheap for the dense Int/Vector{SV} case; free for the ArrayOfVectorViews case, since excluded cells just get a zero-length view).

Breaking changes for users

  • A user's element_routine!/element_residual! method dispatching on a concrete state::Vector{T} must be widened to state::AbstractVector{T} — such a state now arrives as a SubArray. Updated in src/Utils/MaterialModelsBase.jl (the MaterialModelsBase.jl integration) and the test suite.
  • A per-cell Vector{T} state can no longer be resized (push!/pop!/resize!) during assembly — it's now a fixed-size view into shared storage. In-place content mutation (the common case) is unaffected.
  • Indexing a StateVector (or get_state(buffer, cellnum)) with a cell number outside the domain no longer goes through the same code path as membership-checked access — haskey/get remain correct, and getindex/setindex! on the public API still throw KeyError for a genuinely out-of-domain key (so the documented API surface is unchanged); only internal callers skip the check.

Performance

Benchmarked on a 60×60 quad grid (3600 cells) with a Vector{BenchState}-per-cell (per-quadpoint) material state, comparing against unmodified main:

Operation main (Dict) this PR (ArrayOfVectorViews)
get_state(buffer, cellnr) ~4.8 ns ~3.4 ns
work! (full 3600-cell assembly) ~455 μs ~405 μs
update_states! ~92 μs ~21 μs (~4.5x)
setup_domainbuffer construction 1.18 MiB / 14487 allocs 1.49 MiB / 14476 allocs (not improved)

The steady-state hot path (repeated every assembly iteration/time step) is meaningfully faster, especially update_states!. Construction is a one-time cost per domain and is not improved by this PR — it still builds a temporary per-cell Vector before packing it into the flat buffer, same as PR #66 left as a "can be optimized later" TODO. Worth a follow-up if construction-time allocation matters for a given workflow.

Review process

Implemented following this repo's /dual-review workflow: an independent Codex review of the implementation plan surfaced 4 findings (a type-parameter mismatch between the declared SV and ArrayOfVectorViews's actual SubArray element type, _copy_states! needing to iterate the domain's cell set rather than the full dense storage, the checked/unchecked getindex split, and ArrayOfVectorViews needing an explicit setindex!) — all accepted and incorporated before implementing. A second independent Codex review of the finished diff surfaced 4 more: a silent buffer-overflow risk when old/new per-cell state lengths mismatch during packing, getindex/setindex! losing KeyError-on-miss, get recursing into a StackOverflowError for a non-Int key, and a user-returned SubArray whole-cell state losing its whole-value copy_state dispatch — all four accepted and fixed, each verified with a targeted repro.

Test plan

  • Pkg.test() — full suite passes, including test/states.jl's 481 assertions (the AbstractDict interface, copy_state/copy_state! precedence, and mutation-identity guarantees)
  • All 6 tutorials and 6 howtos run individually without error
  • docs/make.jl builds clean
  • Targeted repros for all 8 Codex findings (4 plan-stage, 4 diff-stage), confirming each is fixed

🤖 Generated with Claude Code

https://claude.ai/code/session_01KySgpVgJ5fWR9AvPK1JQoU

Replace StateVector's Dict{Int,SV} with a dense, cell-number-indexed
backing. Vector-per-cell states (one entry per quadrature point) are
flattened into a single Ferrite.CollectionsOfViews.ArrayOfVectorViews
per old/new, avoiding one small Vector allocation per cell; other
per-cell states use a plain dense Vector{SV}. Inspired by PR KnutAM#66, but
reworked against the current codebase's AbstractDict-like interface,
copy_state/copy_state! mutation-identity guarantees, and facet domains
(none of which PR KnutAM#66, from Dec 2025, accounted for), and using direct
dense indexing instead of PR KnutAM#66's separate cellnr->row translation
layer.

Details:
- StateVector{SV,VV} now stores vals::VV (dense, indexed directly by
  cell number) plus the domain's sorted cell set; getindex/setindex!/
  get check membership (KeyError like a Dict), while a separate
  unchecked _raw_getindex/_raw_setindex! serves the assembly hot path
  (get_state/get_old_state, _copy_states!) so the membership check
  never appears there.
- create_states now builds old+new together in one pass and returns
  a full StateVariables; _pack_states flattens Vector{T}-per-cell
  states via ArrayOfVectorViews (dense over the whole grid, matching
  the existing pattern in Workers/QuadPointEvaluator.jl) or falls
  back to a plain Vector{SV} for any other per-cell state type
  (scalar struct, Nothing, or a non-Vector AbstractArray such as an
  immutable NTuple-backed wrapper).
- _copy_states! distinguishes packed (always safe to mutate
  element-wise, since the parent flat buffer is always ours and
  mutable) from non-packed SubArray/AbstractArray values (which keep
  the original ismutable-based classification, so a user-returned
  SubArray whole-cell state still goes through copy_state/copy_state!
  as a whole value).
- Breaking changes for users: element_routine!/element_residual!
  methods dispatching on a concrete state::Vector{T} must be widened
  to state::AbstractVector{T} (updated in
  Utils/MaterialModelsBase.jl and the test suite); a per-cell
  Vector{T} state can no longer be resized (push!/resize!) during
  assembly, since it is now a fixed-size view into shared storage.
- Benchmarked (3600-cell grid, Vector-per-cell state): get_state
  ~4.8ns -> ~3.4ns, update_states! ~92us -> ~21us (~4.5x), work!
  ~455us -> ~405us. Construction allocation count/memory is not
  improved (still two-pass: per-cell Vector then packed) and is
  worth a follow-up.

Testing: Pkg.test() passes (full suite, including test/states.jl's
481 assertions covering the AbstractDict interface, copy_state/
copy_state! precedence, and mutation-identity guarantees); all 6
tutorials and 6 howtos run individually; docs/make.jl builds clean.

Reviewed with an independent Codex pass at plan and diff stage;
accepted findings are reflected in the implementation (see PR
description for the full list of accepted/rejected findings).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KySgpVgJ5fWR9AvPK1JQoU
@codecov

codecov Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.85185% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.92%. Comparing base (0084073) to head (d95b954).

Files with missing lines Patch % Lines
src/states.jl 91.47% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
- Coverage   97.31%   96.92%   -0.39%     
==========================================
  Files          32       32              
  Lines        1376     1433      +57     
==========================================
+ Hits         1339     1389      +50     
- Misses         37       44       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant