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

Regression: mul! of views allocates in v1.9+ for arrays of 4 or higher dimension, not in v1.8.5 #49332

Closed
pablosanjose opened this issue Apr 12, 2023 · 5 comments · Fixed by #53091
Labels
linear algebra Linear algebra regression Regression in behavior compared to a previous version regression 1.9 Regression in the 1.9 release

Comments

@pablosanjose
Copy link
Contributor

pablosanjose commented Apr 12, 2023

This seems like a regression: in Julia v1.9-rc2 and master

julia> using LinearAlgebra, BenchmarkTools

julia> A = rand(ComplexF64,4,4,1000,1000);

julia> B = similar(A);

julia> a,b = (view(B,:,:,1,1),view(A,:,:,1,1));

julia> @btime mul!($b,$a,$a);
  321.792 ns (10 allocations: 608 bytes)

This is on an M1 Max

julia> versioninfo()
Julia Version 1.9.0-rc2
Commit 72aec423c2a (2023-04-01 10:41 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin21.3.0)
  CPU: 10 × Apple M1 Max
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, apple-m1)
  Threads: 1 on 8 virtual cores
Environment:
  JULIA_EDITOR = code

But in v1.8.5 we don't allocate

julia> using LinearAlgebra, BenchmarkTools

julia> A = rand(ComplexF64,4,4,1000,1000);

julia> B = similar(A);

julia> a,b = (view(B,:,:,1,1),view(A,:,:,1,1));

julia> @btime mul!($b,$a,$a);
  97.076 ns (0 allocations: 0 bytes)

Note that if A has dimensions 3 or less, this issue does not arise.

@KristofferC
Copy link
Sponsor Member

I haven't benchmarked this but one difference is the following:

julia> A = rand(ComplexF64,4,4,1000,1000);

julia> B = similar(A);

julia> a,b = (view(B,:,:,1,1),view(A,:,:,1,1));

julia> f(A,B,C) = Base.require_one_based_indexing(A,B,C)
f (generic function with 1 method)

julia> @code_warntype optimize=true f(a, a, b)

This used to constant fold in 1.8 to true but on 1.9 it calls:

1%1  = invoke Base._any_tuple(Base.has_offset_axes::typeof(Base.has_offset_axes), false::Bool, A::SubArray{ComplexF64, 2, Array{ComplexF64, 4}, ...

@KristofferC
Copy link
Sponsor Member

KristofferC commented Apr 12, 2023

Seems to be f718238 cc @vtjnash

(but again, I have not confirmed that this is the actual cause of the speed or allocation regression here, just the change in inlining)

Seems to indeed be where the regression comes from:

Before:

  185.500 ns (0 allocations: 0 bytes)

After:

 509.057 ns (10 allocations: 608 bytes)

@dkarrasch dkarrasch added regression Regression in behavior compared to a previous version linear algebra Linear algebra labels Apr 13, 2023
@pablosanjose
Copy link
Contributor Author

Is it clear to anyone what would be the good strategy to fix this?

@pablosanjose
Copy link
Contributor Author

Friendly bump, in case anyone with some available time knows what could be done here.

This is a fairly severe regression that has our simulation code stuck on 1.8.5 because we need views of 5-dimensional arrays, and the runtime penalty is too large. With current nightly it's become even worse:

julia> @btime mul!($b,$a,$a);
  493.124 ns (10 allocations: 608 bytes)

@pablosanjose
Copy link
Contributor Author

It seems the root cause of this is the following

julia> A = view(rand(ComplexF64, 4, 4, 4, 4), :, :, 1, 1);

julia> @allocations Base.require_one_based_indexing(A,A,A)  # second run
11

This should be 1. So require_one_based_indexing is sometimes not elided as expected. The PR #45260 turns out to be insufficient for this particular combination of views of high-dimensional arrays.

N5N3 pushed a commit that referenced this issue Jan 30, 2024
…ews (#53091)

Closes #49332

---------

Co-authored-by: Denis Barucic <barucic.d@gmail.com>
KristofferC pushed a commit that referenced this issue Feb 6, 2024
…ews (#53091)

Closes #49332

---------

Co-authored-by: Denis Barucic <barucic.d@gmail.com>
(cherry picked from commit 9edf1dd)
Drvi pushed a commit to RelationalAI/julia that referenced this issue Jun 7, 2024
…ews (JuliaLang#53091)

Closes JuliaLang#49332

---------

Co-authored-by: Denis Barucic <barucic.d@gmail.com>
(cherry picked from commit 9edf1dd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra regression Regression in behavior compared to a previous version regression 1.9 Regression in the 1.9 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants