Open mattuntergassmair opened 4 years ago
Could potentially be a breaking change if users were relying on the return type of ind2x
to be Array
rather than AbstractArray
@MaximeBouton shall we merge this?
I like using StaticArrays! Shouldn't you be able to get it down to 0 allocs though? You have only reduced it to 1000000 from 2000000
I like using StaticArrays! Shouldn't you be able to get it down to 0 allocs though? You have only reduced it to 1000000 from 2000000
As for the allocations, I'm running the function benchmark_ind2x()
with default parameters n_dims = 6, n_points_per_dim = 10
, leading to 10^6 points. The benchmark function then calls ind2x()
10^6=1000000 times. So I think the allocs are as expected (as opposed to ind2x!()
which should be non-allocating). I think what was happening earlier was that there was a copy somewhere leading to 2 x 10^6 allocs.
Could potentially be a breaking change if users were relying on the return type of
ind2x
to beArray
rather thanAbstractArray
I think this is fine. If people are relying on a concrete array type, I will not feel sorry if their code gets broken :)
Shouldn't we expect a similar speed up by replacing those: https://github.com/sisl/GridInterpolations.jl/blob/master/src/GridInterpolations.jl#L36 by mutable statically sized arrays?
Shouldn't we expect a similar speed up by replacing those: https://github.com/sisl/GridInterpolations.jl/blob/master/src/GridInterpolations.jl#L36 by mutable statically sized arrays?
True, we could probly fix the size of most vectors that are part of the RectangleGrid
struct and that should help with allocation and performance.
I could look into it but might need some help since I don't feel I understand the code base sufficiently well. Anyone want to team up on this (for a different PR)?
Those allocations are in the constructor, so I don't think it will make too much difference because they are only done once when the object is constructed.
On Fri, Mar 6, 2020 at 4:29 PM Maxime notifications@github.com wrote:
Shouldn't we expect a similar speed up by replacing those:
https://github.com/sisl/GridInterpolations.jl/blob/master/src/GridInterpolations.jl#L36 by mutable statically sized arrays?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sisl/GridInterpolations.jl/pull/32?email_source=notifications&email_token=ABALI26NZBQ4T2K4OVNV5OTRGGBODA5CNFSM4LCUEXH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEODFVIY#issuecomment-596007587, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABALI27K3ONJYOXWJJKLJTLRGGBODANCNFSM4LCUEXHQ .
Those allocations are in the constructor, so I don't think it will make too much difference because they are only done once when the object is constructed. … On Fri, Mar 6, 2020 at 4:29 PM Maxime @.***> wrote: Shouldn't we expect a similar speed up by replacing those: https://github.com/sisl/GridInterpolations.jl/blob/master/src/GridInterpolations.jl#L36 by mutable statically sized arrays? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32?email_source=notifications&email_token=ABALI26NZBQ4T2K4OVNV5OTRGGBODA5CNFSM4LCUEXH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEODFVIY#issuecomment-596007587>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABALI27K3ONJYOXWJJKLJTLRGGBODANCNFSM4LCUEXHQ .
Agreed that it may not make a difference in the constructor itself, but we could make the member variables static in size, maybe the fixed size helps with memory optimization. I can give it a shot and report back.
As for the allocations, I'm running the function
benchmark_ind2x()
with default parametersn_dims = 6, n_points_per_dim = 10
, leading to 10^6 points. The benchmark function then callsind2x()
10^6=1000000 times. So I think the allocs are as expected (as opposed toind2x!()
which should be non-allocating). I think what was happening earlier was that there was a copy somewhere leading to 2 x 10^6 allocs.
@MVector zeros(D)
Allocates. We should really do this with SVectors. Right now we are getting a 2x speedup with mvectors. I bet we will get a 10 or 100x speedup with svectors
For a broader utilization of MVector
for the interpolation code, one thing to keep in mind is that if the size is larger than 100 we should expect decrease in performance according to the StaticArrays.jl readme. Since the size is given by 2^num_dimensions, it grows pretty fast.
What is the status of this PR?
It seems that pre-specifying the vector size using
MVector
fromStaticArrays
helps to significantly improve the performance ofind2x