perf: fix performance regression for non-abelian index manipulations - #521
Conversation
|
Some benchmark results to show that this resolves #517, using the reproduction script from that issue (single-threaded, Julia 1.12.6; different machine than the issue's numbers, so compare ratios rather than absolute times). Minimum times, allocations per call in parentheses:
The 1.7–1.9× regression relative to v0.16.5 that the issue reported is gone for the non-abelian cases, and the allocation counts are back at v0.16.5 levels (SU(2): 2845 → 1932 vs 1841 on v0.16.5). A few additional observations:
|
Codecov Report❌ Patch coverage is
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Test failure is unrelated |
|
Follow-up with the full benchmark suite, run via AirspeedVelocity on a dedicated cluster node (exclusive node, Julia 1.12.6,
Rest of the suite (127 linalg / tensor-network benchmarks): median PR/main = 0.99 and median PR/v0.16.5 = 0.99, i.e. no systematic change outside the targeted kernels. The tails:
|
| for j in 2:min(length(resources), n) | ||
| local r = resources[j] | ||
| Threads.@spawn _taskforeach_worker(Base.Fix2(f, r), items′, counter, n) | ||
| end | ||
| _taskforeach_worker(Base.Fix2(f, first(resources)), items′, counter, n) |
There was a problem hiding this comment.
Does this need an inbounds for the resources indexing ? Probably that is negligible overhead compared to spawning tasks?
I would also write resources[1] instead of first(resources) for consistency with the other indexing, but that is a personal matter of taste.
Use a let block for the per-task resource capture, make the two methods structurally identical (shared nworkers local and zero-worker early return), and update the docstring to the actual signatures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
leburgel
left a comment
leburgel
left a comment
There was a problem hiding this comment.
Looks great, thanks for sorting this out!
`add_transform_kernel!` gained a `buffersize(tsrc, fblocks)` call in #521, but its `tdst`/`tsrc` are untyped, so the analysis widens `tsrc` to `Union{AbstractTensorMap, HomSpace}` and the `HomSpace` arm has no `buffersize` method. The only caller already passes tensor maps, and the two sibling methods take `DenseVector`, so annotate the signature to match. Also switch the `GenericTreeTransformer` matcher to `AnyFrameMethod`: #521 moved the `@debug` into a `taskforeach` closure, so the innermost frame is now a gensym'd closure name and `LastFrameMethod` no longer matches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
This refactors some of the process for handling the allocators for the implementation of the
GenericTreeTransformer, which should restore the performance from v0.16.5 by no longer having to allocate buffers for each separate block. Along the way I also addedtaskforeachsince that pattern kept showing up and the version with resources is hard to express with OhMyThreads.jl, so in some follow-up PRs I might actually switch more things to that as well.This fixes #517.