Scalarized gca_gca_intersection - #1688
Open
cmdupuis3 wants to merge 9 commits into
Open
Conversation
Add scalar-argument L1/L2 siblings (_accux_gca_scalar, _try_gca_gca_intersection_scalar) and rewire gca_gca_intersection to them, cutting 4 heap allocations per edge to 1. Bit-identical output; ~2.7x on the function in isolation, ~1.24x on serial face bounds.
gca_gca_intersection no longer calls the array-form _accux_gca / _try_gca_gca_intersection, so benchmarking them no longer reflects the dispatcher's actual cost. Point at the scalar siblings instead, skipping gracefully (via skip_benchmark_if) on commits that predate them.
cmdupuis3
added
scalability
Collaborator
Author
|
pre-commit.ci autofix |
Collaborator
Author
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
Benchmarks that have got worse:
|
Collaborator
Author
|
Before I remove the backwards-compatibility (which will be redundant after this merges), here are the CI benchmarks for gca_gca intersections, showing about a 10% speedup. Benchmarks that have improved:
|
Collaborator
Author
|
I'm wondering if we need the vector-valued |
cmdupuis3
marked this pull request as ready for review
August 24, 2026 22:39
Contributor
Yes, drop all three. _accux_constlat goes outright since gca_const_lat_intersection already uses _accux_constlat_scalar; the GCA/GCA pair needs a _accux_gca_scalar (six floats in/out, status logic inlined) first so gca_gca_intersection has a scalar path, then _accux_gca and _try_gca_gca_intersection can go. |
Collaborator
Author
|
I took out two of them, but I left |
Member
Everything in this PR looks good to me, and I will approve it once actions complete, but one question about these benchmarks: How much of confidence do we have about these microsecond times, e.g. measuring process overhead vs actual operations? |
Collaborator
Author
|
In general, a little less than larger ones, but in this case, you can check the last few commits and see the benchmarks are consistently about 10% lower, so I'm confident it's not a fluke here. |
erogluorhan
added
run-benchmark
erogluorhan
self-requested a review
August 25, 2026 17:43
erogluorhan
approved these changes
Aug 25, 2026
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
3 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.
Tangent issue to #1623, also related to #1648
Overview
The existing
gca_gca_intersectionuses fournp.ndarrayobjects of size 3. The overhead incurred by using these python objects is njit compiled loops is significant, and we get about a 2x speedup in there by scalarizinggca_gca_intersection,_try_gca_gca_intersection, and_accux_gca, and using_on_minor_arc_xyzinstead ofon_minor_arc. That precipitates a ~10% speedup overall of_gca_gca_intersection.This PR mainly provides scalarized versions of
_accux_gca_scalarand_try_gca_gca_intersection. The vector versions are preserved, as established with_accux_constlat, but I think it's reasonable to delete all the vector versions if they aren't being used otherwise. Benchmarks for scalarized routines are added separately, but we could also delete the vector benchmarks if the vector routines go.PR Checklist
General
Testing & Benchmarking
Documentation
_)AI Disclosure
AI Usage: Claude Opus 5 / Sonnet 5