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

specialize copyto! and multiplication by numbers for Q from qr #39533

Merged
merged 3 commits into from
Feb 23, 2021

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Feb 5, 2021

This fixes two performance bugs reported in #38972 and #38972 (multiplication of Q from qr by a Diagonal or UniformScaling). In particular, it improves the performance of generating random orthogonal matrices as described in #38972.

Latest Julia nightly (v1.7.0-DEV.448)

julia> using LinearAlgebra

julia> n = 200; Q, R = qr(randn(n, n)); A = randn(n, n); D = Diagonal(randn(n));

julia> @time Q * A;
  0.073661 seconds (263.33 k allocations: 16.445 MiB, 98.74% compilation time)

julia> @time A * Q;
  0.007283 seconds (4.03 k allocations: 885.695 KiB, 86.00% compilation time)

julia> @time Q * D;
  1.402949 seconds (718.24 k allocations: 241.546 MiB, 1.77% gc time, 12.63% compilation time)

julia> @time D * Q;
  1.262098 seconds (386.20 k allocations: 222.074 MiB, 0.53% gc time, 3.97% compilation time)

julia> @time Q * I;
  1.255886 seconds (363.48 k allocations: 221.299 MiB, 0.56% gc time, 3.27% compilation time)

julia> @time I * Q;
  1.250831 seconds (330.64 k allocations: 219.089 MiB, 0.54% gc time, 3.02% compilation time)

This PR

julia> using LinearAlgebra

julia> n = 200; Q, R = qr(randn(n, n)); A = randn(n, n); D = Diagonal(randn(n));

julia> @time Q * A;
  0.079362 seconds (263.33 k allocations: 16.442 MiB, 98.72% compilation time)

julia> @time A * Q;
  0.008170 seconds (4.03 k allocations: 885.695 KiB, 85.55% compilation time)

julia> @time Q * D;
  0.117407 seconds (366.77 k allocations: 21.280 MiB, 9.75% gc time, 98.93% compilation time)

julia> @time D * Q;
  0.083525 seconds (265.85 k allocations: 15.456 MiB, 98.40% compilation time)

julia> @time Q * I;
  0.013162 seconds (15.95 k allocations: 1.567 MiB, 90.46% compilation time)

julia> @time I * Q;
  0.010540 seconds (8.82 k allocations: 1.140 MiB, 88.74% compilation time)

Of course, further performance optimizations are likely possibly by introducing better specializations of various multiplication methods of Q by something from the left/right as mentioned in #38972, but this should be a good first step.

I cannot add labels to this, but "linear algebra" and "performance" would be appropriate, I think.

This fixes two performance bugs reported in JuliaLang#38972
and JuliaLang#38972 (multiplication of `Q` from `qr` by a
`Diagonal` or `UniformScaling`). In particular, it improves the performance of generating
random orthogonal matrices as described in https://discourse.julialang.org/t/random-orthogonal-matrices/9779/7.
@dkarrasch dkarrasch added linear algebra Linear algebra performance Must go faster labels Feb 5, 2021
@ranocha ranocha closed this Feb 8, 2021
@ranocha ranocha reopened this Feb 8, 2021
@sostock
Copy link
Contributor

sostock commented Feb 10, 2021

The same could be done for LQPackedQ (which is not a subtype of AbstractQ, I don’t know whether it should be). I just did some benchmarks and it leads to massive performance improvements.

@ranocha
Copy link
Member Author

ranocha commented Feb 10, 2021

I would be happy to include these modifications here (if you create a PR into my branch or post them as suggested changes as review). Otherwise, it would also be great if you could open a new PR to Julia. Anyway, it would be nice if someone could review this and we could get this merged.

@ranocha
Copy link
Member Author

ranocha commented Feb 23, 2021

Bump. Is there anything that needs to be done before this can be merged?

@oscardssmith oscardssmith merged commit 8e949d6 into JuliaLang:master Feb 23, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
…Lang#39533)

* specialize copyto! and multiplication by numbers for Q from qr

This fixes two performance bugs reported in JuliaLang#38972
and JuliaLang#38972 (multiplication of `Q` from `qr` by a
`Diagonal` or `UniformScaling`). In particular, it improves the performance of generating
random orthogonal matrices as described in https://discourse.julialang.org/t/random-orthogonal-matrices/9779/7.

* fix typo in new qr tests

* resolve mehod ambiguity of copyto!
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
…Lang#39533)

* specialize copyto! and multiplication by numbers for Q from qr

This fixes two performance bugs reported in JuliaLang#38972
and JuliaLang#38972 (multiplication of `Q` from `qr` by a
`Diagonal` or `UniformScaling`). In particular, it improves the performance of generating
random orthogonal matrices as described in https://discourse.julialang.org/t/random-orthogonal-matrices/9779/7.

* fix typo in new qr tests

* resolve mehod ambiguity of copyto!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants