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 #998

Closed
pablosanjose opened this issue Apr 12, 2023 · 5 comments · Fixed by JuliaLang/julia#53091
Labels
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
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
Member

KristofferC commented Apr 12, 2023

Seems to be f718238af297f5179fe113ff76236 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 the regression Regression in behavior compared to a previous version label 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 JuliaLang/julia#45260 turns out to be insufficient for this particular combination of views of high-dimensional arrays.

N5N3 referenced this issue in JuliaLang/julia Jan 30, 2024
…ews (#53091)

Closes #49332

---------

Co-authored-by: Denis Barucic <barucic.d@gmail.com>
KristofferC referenced this issue in JuliaLang/julia Feb 6, 2024
…ews (#53091)

Closes #49332

---------

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

Closes #49332

---------

Co-authored-by: Denis Barucic <barucic.d@gmail.com>
(cherry picked from commit 9edf1dd)
@KristofferC KristofferC transferred this issue from JuliaLang/julia Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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