Skip to content

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Sep 15, 2025

Do not add single-argument methods to:

  • eachindex

  • firstindex

  • lastindex

The above have fallbacks that work as intended as long as the two two-argument methods of eachindex are defined.

So add the two-argument methods of eachindex instead of adding single-argument methods of the three functions.

On nightly Julia, together with PR #652, this PR decreases the amount of sysimage invalidations that happen when loading SparseArrays from $26$ to $17$.

Without PR #652, this PR happens to increase the amount of invalidations, to $28$.

Do not add single-argument methods to:

* `eachindex`

* `firstindex`

* `lastindex`

The above have fallbacks that work as intended as long as the two
two-argument methods of `eachindex` are defined.

So add the two-argument methods of `eachindex` instead of adding
single-argument methods of the three functions.

On nightly Julia, together with PR #652, this PR decreases the amount
of sysimage invalidations that happen when loading SparseArrays from
$26$ to $17$.

Without PR #652, this PR happens to increase the amount of
invalidations, to $28$.
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.01%. Comparing base (306605d) to head (14976ec).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/readonly.jl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
- Coverage   84.02%   84.01%   -0.01%     
==========================================
  Files          12       12              
  Lines        9297     9299       +2     
==========================================
+ Hits         7812     7813       +1     
- Misses       1485     1486       +1     

☔ View full report in Codecov by Sentry.
📢 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.

dkarrasch
dkarrasch previously approved these changes Sep 15, 2025
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM up to a minor nitpick.

Co-authored-by: Daniel Karrasch <[email protected]>
@nsajko nsajko marked this pull request as ready for review September 15, 2025 15:48
@nsajko
Copy link
Contributor Author

nsajko commented Sep 20, 2025

merge?

@dkarrasch dkarrasch merged commit 26c80c8 into JuliaSparse:main Sep 21, 2025
8 of 10 checks passed
@nsajko nsajko deleted the invalidations_fixes_readonly_eachindex-firstindex branch September 21, 2025 10:17
@KristofferC KristofferC added the backport 1.12 Change should be backported to release-1.12 label Sep 22, 2025
KristofferC pushed a commit that referenced this pull request Sep 22, 2025
@KristofferC KristofferC mentioned this pull request Sep 22, 2025
7 tasks
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Sep 22, 2025
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.

3 participants