Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accelerate recursive Mul #153

Closed
wants to merge 5 commits into from

Conversation

putianyi889
Copy link
Contributor

@putianyi889 putianyi889 commented Jul 18, 2023

This change only takes effect after JuliaArrays/LazyArrays.jl#261

using LazyArrays

A = ApplyArray(*, [ones(10,10) for _ in 1:10]...);
@time for m in 1:10, n in 1:10
    A[m,n]
end

B = ones(10,10)
for k in 1:5
    B = ApplyArray(*, B, B)
end
@time for m in 1:10, n in 1:10
    B[m,n]
end

C = ones(10,10)
M = Mul(C, C)
@time for m in 1:10, n in 1:10
    M[m,n]
end

Before: exponential in depth

256.268356 seconds (451.16 M allocations: 13.413 GiB, 1.39% gc time)
0.615169 seconds (3.37 M allocations: 105.647 MiB, 4.57% gc time)
0.000029 seconds (100 allocations: 1.562 KiB)

After: linear in operations

0.003850 seconds (4.50 k allocations: 1.401 MiB)
0.059771 seconds (27.50 k allocations: 6.798 MiB)
0.000078 seconds (300 allocations: 29.688 KiB)

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (a7786a0) 86.49% compared to head (e893bea) 86.51%.
Report is 7 commits behind head on master.

❗ Current head e893bea differs from pull request most recent head dbeb413. Consider uploading reports for the commit dbeb413 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #153      +/-   ##
==========================================
+ Coverage   86.49%   86.51%   +0.02%     
==========================================
  Files          10       10              
  Lines        1740     1765      +25     
==========================================
+ Hits         1505     1527      +22     
- Misses        235      238       +3     
Files Changed Coverage Δ
src/ArrayLayouts.jl 79.58% <100.00%> (-2.55%) ⬇️
src/memorylayout.jl 84.53% <100.00%> (+0.34%) ⬆️
src/mul.jl 84.50% <100.00%> (+2.60%) ⬆️

... and 1 file with indirect coverage changes

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

@putianyi889
Copy link
Contributor Author

solves JuliaArrays/LazyArrays.jl#259

@dlfivefifty
Copy link
Member

dlfivefifty commented Jul 21, 2023

As we discussed, I think this should be in lazy arrays and only when of the layouts are ApplyLayout{typeof(*)}. Otherwise the old version is allocation free.

@dlfivefifty dlfivefifty reopened this Jul 21, 2023
@dlfivefifty
Copy link
Member

I've reopened as your proposal on maybeview might be a better approach. Though we'd have to make sure it works

@putianyi889
Copy link
Contributor Author

Though we'd have to make sure it works

I'm using a custom lazyview on my other branch and it's working fine. Overwriting maybeview is type piracy and needs more consideration.

@dlfivefifty
Copy link
Member

Maybe call it something descriptive like this:

"""
fast_maybeview(A, kr...)

returns a view if indexing is fast, otherwise calls `A[kr...]` to efficiently materialise an array.
""""
fast_maybeview(A, kr...) = ...

@putianyi889
Copy link
Contributor Author

putianyi889 commented Jul 21, 2023

or just fastview?

Edit: it doesn't work like view since setindex! doesn't always work. I prefer maybeview_layout as it's consistent with other naming conventions

@dlfivefifty
Copy link
Member

I think JuliaArrays/LazyArrays.jl#268 makes this unnecessary

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.

2 participants