JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
87 stars 8 forks source link

More method specializations for `allocate_result` #137

Closed mateuszbaran closed 1 year ago

mateuszbaran commented 1 year ago

We have an unfortunate side-effect of our trait system. Method recursion prevents some type inference from happening around, for example, allocate_result. This can cause massive slow-downs, for example the current Manifolds.jl does 32 allocations to copy a point on SpecialOrthogonal(3). This PR solves this partially -- with these changes it's now just 4 -- but we will have to introduce specialized methods to work around inefficiencies in some key parts. I don't really see how to improve it further generically.

codecov[bot] commented 1 year ago

Codecov Report

Merging #137 (d0d09e3) into master (16f28da) will decrease coverage by 0.00%. The diff coverage is 100.00%.

:exclamation: Current head d0d09e3 differs from pull request most recent head 87210ef. Consider uploading reports for the commit 87210ef to get more accurate results

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
- Coverage   99.74%   99.74%   -0.01%     
==========================================
  Files          18       18              
  Lines        2391     2386       -5     
==========================================
- Hits         2385     2380       -5     
  Misses          6        6              
Impacted Files Coverage Δ
src/decorator_trait.jl 100.00% <100.00%> (ø)
src/ManifoldsBase.jl 98.58% <0.00%> (-0.95%) :arrow_down:
src/numbers.jl 100.00% <0.00%> (ø)
src/PowerManifold.jl 99.79% <0.00%> (+0.40%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

kellertuer commented 1 year ago

Interesting; I do not understand enough of the implications here, I fear.

mateuszbaran commented 1 year ago

I don't completely understand it either. I've just tried benchmarking copy again and it went to 1 allocation :thinking: . Julia's optimizer can sometimes be surprisingly nondeterministic.

kellertuer commented 1 year ago

Maybe we could even ask that on discourse whether someone can take a look and give us a hint where we could improve that?

mateuszbaran commented 1 year ago

I just did that :smiley: https://discourse.julialang.org/t/extremely-slow-invoke-when-inlined/90665

mateuszbaran commented 1 year ago

That is, my question is for another issue which is even less clear to me. This thing with allocate_result is less troubling for me.

kellertuer commented 1 year ago

But cool that you work on these technical details :)

mateuszbaran commented 1 year ago

I just like when things work fast :slightly_smiling_face:

kellertuer commented 1 year ago

That is the same for me, but I do not have learned about all the technical details you always figure out ;)