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

phi! allocates #126

Open
nathanaelbosch opened this issue Apr 7, 2023 · 2 comments
Open

phi! allocates #126

nathanaelbosch opened this issue Apr 7, 2023 · 2 comments

Comments

@nathanaelbosch
Copy link

The docstring of phi! states:

"""
phi!(out,A,k[;caches]) -> out
Non-allocating version of `phi` for non-diagonal matrix inputs.
"""

Yet, it allocates:

using ExponentialUtilities, BenchmarkTools

d, q = 4, 3
L = rand(d, d)
out = ExponentialUtilities.phi(L, q);
caches = (zeros(d), zeros(d, q + 1), zeros(d + q, d + q))
expmethod = ExpMethodHigham2005()

@btime ExponentialUtilities.phi!($out, $L, $q; caches=$caches, expmethod=$expmethod);
# 21.998 μs (32 allocations: 10.00 KiB)

(and figuring out how to choose out and caches already required a bit of digging in the source code as this part is basically undocumented)

In comparison, non-allocating exponential! is very straight-forward:

cache = ExponentialUtilities.alloc_mem(L, expmethod);
@btime ExponentialUtilities.exponential!(copy($L), $expmethod, $cache) 
# 3.214 μs (2 allocations: 288 bytes) 

The reason

Going through profilers and some code, the issue seems to be the combination of this call here

P = exponential!(cache, expmethod)

which then leads to some allocation here
function exponential!(A, method::ExpMethodHigham2005, _cache = alloc_mem(A, method))

Potential solution

Initialize this cache outside of the call to phi! and pass it as a function argument (e.g. expcache?). I tried this locally and such a change would remove most allocations:

expcache = ExponentialUtilities.alloc_mem(zeros(d + q, d + q), expmethod);
@btime ExponentialUtilities.phi!($out, $L, $q; caches=$caches, expmethod=$expmethod, expcache=$expcache,);
#  20.877 μs (4 allocations: 448 bytes)

(unfortunately it does not affect times a lot)

@ChrisRackauckas
Copy link
Member

yeah it should pre-allocate that cache in cache. That's an oversight and should get a test.

@nathanaelbosch
Copy link
Author

So what exactly is your suggestion to fix this issue? I'm more than happy to help here to have an (ideally easily accessible) allocation-free phi!.

From my point of view, I'd think that the preallocation for phi! in general could benefit from some improvements, e.g. the differences between caches and cache is not clear at all (the former is basically undocumented and there is no easy syntax to preallocate it right now), and maybe the output should also be more easily preallocateable.

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

No branches or pull requests

2 participants