Skip to content

Conversation

@jishnub
Copy link
Member

@jishnub jishnub commented Apr 5, 2025

Currently, we specialize copyto! for a triangular source. However, this has two branches, depending on whether the axes match. In the branch where the axes do match, we may use copy! instead, and specialize this in terms of the internal _copyto! which is identical in implementation.

We also use copy! in ldiv instead of copyto!. These should be equivalent in most cases, barring one extra axes check in copy!, as the fallback method for copy! calls copyto! internally. However, the advantage comes for triangular matrices, where copy! doesn't have branches, as the axes necessarily match.

As a consequence, this reduces the TTFX in operations like

julia> using Random, LinearAlgebra

julia> A = rand(4,4);

julia> @time A \ UpperTriangular(A);
  0.598575 seconds (1.28 M allocations: 61.788 MiB, 98.57% compilation time: 3% of which was recompilation) # master
  0.487267 seconds (1.01 M allocations: 49.726 MiB, 3.54% gc time, 98.95% compilation time) # this PR
julia> @time UpperTriangular(A) / A;
  0.826212 seconds (1.45 M allocations: 71.427 MiB, 16.96% gc time, 84.47% compilation time) # master
  0.616258 seconds (1.28 M allocations: 63.135 MiB, 2.65% gc time, 99.19% compilation time) # this PR

I've renamed the internal functions as _copyto! -> _copy! and copyto_unaliased! -> copy_unaliased!, as these are closer to the meaning of copy! than to copyto!.

@jishnub jishnub added the arrays [a, r, r, a, y, s] label Apr 5, 2025
@codecov
Copy link

codecov bot commented Apr 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.97%. Comparing base (925acef) to head (56aacff).
Report is 24 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1263      +/-   ##
==========================================
- Coverage   92.03%   91.97%   -0.07%     
==========================================
  Files          34       34              
  Lines       15475    15479       +4     
==========================================
- Hits        14243    14237       -6     
- Misses       1232     1242      +10     

☔ 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.

@jishnub jishnub added the ttfx The change pertains to first-call latency label Apr 5, 2025
@jishnub jishnub requested a review from dkarrasch April 8, 2025 05:46
@jishnub
Copy link
Member Author

jishnub commented Apr 11, 2025

Gentle bump

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.

SGTM

I haven't looked into the copy! and copyto! dispatch in a while and don't have the bandwidth to do that now, but this either doesn't change anything (like renaming or being led to the same implementation by multiple dispatch), or improves ttfx and/or runtime, right?

@jishnub
Copy link
Member Author

jishnub commented Apr 11, 2025

Yes, I think this should be a harmless change

@jishnub jishnub merged commit 3e525a8 into master Apr 12, 2025
3 of 4 checks passed
@jishnub jishnub deleted the jishnub/tri_copy branch April 12, 2025 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrays [a, r, r, a, y, s] ttfx The change pertains to first-call latency

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants