-
Notifications
You must be signed in to change notification settings - Fork 42
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
Avoiding repeated allocations in mul! for CompositeMap #90
Comments
Very much yes! Reducing temporary allocations was something I started doing in #74, but got stuck. We have something in that direction for At first, I was skeptical about storing the temporary vectors in the struct, because of the using LinearMaps
struct MyMap{T,S<:LinearMap{T},V<:AbstractVector} <: LinearMap{T}
map::S
temp::V
end
A = MyMap(LinearMap(rand(3,3)), Vector{Float64}(undef, 3))
resize!(A.temp, 5) I wonder though if people would like to use that Another option is to let multiplication happen in internal helper functions, which are provided with memory by some external callee. In case you just call |
Thanks for the encouragement. Is For I agree that in the long run it would be nice to be able to recycle such memory. One option is to make the |
I think this is a good idea in theory. In practice, there could be some issues. The first I see is regarding element type. If say As you try to push this further and optimize this more (like how could you re-use that temporary variable at other places, what are the minimal number of temporary variables I need, how large should they be) you are essentially implementing your own memory handler. It does seem like this something that should be provided for is by the language. And it is, it's the garbage collector. Is there clear evidence that the extra allocations have a major impact in your use case @JeffFessler ? I will be the first to admit that Julia's GC has some problems with large temporary variables, which is why I also have some cache for temporaries in TensorOperations.jl. But there too it feels like reinventing the wheel (and probably in a bad way). |
@Jutho thanks for the comments and questions. I can do some more careful timing tests for my case and see what impact it might have. Most of your concerns also apply to basic |
Yes, |
Ah, good point. It would put even more onus on the advanced user to anticipate the appropriate type they want their
|
Here's a MWE that illustrates the benefits of in-place
|
Hm, I hadn't taken into account the eltype issue. So, #92 introduces a helper function that would allow to pass temporary memory to the computation, albeit not at the generic level. Since this package here is rather generic, I am concerned that there are many issues to be handled. For example, all I do think, however, that in a specific library like @JeffFessler may have in mind, these generic issues may not come up and you have control about output types, number of maps in the composition etc., maybe because these issues are not even floating on the API surface anyway. So, one has two options: (i) call |
Thanks for all the comments. I'll try to just make one last note about @Jutho 's comment that the |
Replacing
y=A*x
withmul!(y, A, x)
gives the (perhaps naive) user (e.g., me) the sense that no memory allocations will need to be made, and often that is the case.But if
A
is aCompositeMap
, e.g.,A = B * C
, then internallyA*x = B*(C*x)
and the current implementation will allocate a temporary variable for the productC*x
every time we doA*x
or themul!
version. This overhead adds up when iterating.We know the size of
C
at the time we make theCompositeMap
, so in principle the LM package could allocate a work space for it and store it as part of the struct, so thatmul!
operations would need no new allocations. (We'd also need a work space forB'
for the adjoint.)Perhaps this could be a user-selectable option when creating a
LinearMap
.Thoughts on this feature suggestion? I might try it myself unless anyone sees flaws in it...
The text was updated successfully, but these errors were encountered: