Type specification in Dg struct

[ranocha]

In GitLab by @ranocha on May 18, 2020, 19:14

If I remember correctly, @sloede mentioned performance problems when passing the Dg struct to some functions instead of the vectors/matrices (like nodes and dhat) used there. I think that's because the types are not fully specified in Dg. For example, we currently have

nodes::SVector{Np1}
dhat::SMatrix{Np1, Np1}

Hence, nodes and dhat are not concretely typed, since

julia> using StaticArrays

julia> SVector{4}
SArray{Tuple{4},T,1,4} where T

julia> SMatrix{4,4}
SArray{Tuple{4,4},T,2,L} where L where T

Being more specific here can probably increase the runtime performance. Additionally, it could be possible to just pass dg::Dg instead of the specific vectors/matrices. I would suggest being flexible and using something along the lines of

struct Dg{VectorNp1, MatrixNp1} <: AbstractSolver
  nodes::VectorNp1
  dhat::MatrixNp1
end

instead of specifying the types concretely, which can also be done, resulting in something like

struct Dg{Np1, Np1Square, RealT} <: AbstractSolver
  nodes::SVector{Np1,RealT}
  dhat::SArray{Tuple{Np1,Np1},RealT,2,Np1Square}
end

[ranocha]

In GitLab by @gregorgassner on May 19, 2020, 13:03

This sounds good to me. It would be great to just pass DG, but not losing performance...

[ranocha]

In GitLab by @ranocha on May 19, 2020, 13:05

That should hopefully be possible. If we look carefully for type instabilities like this and get rid of them, I expect to not lose any performance (but maybe even gain some).

[ranocha]

In GitLab by @sloede on May 20, 2020, 07:07

It sounds very tempting, but it does add complexity to the code (especially for new users not that familiar with the parametric type syntax). Thus, before we roll this out to all performance-critical code sections, I suggest we first run some tests that confirm that this modification will actually solve the performance issues when passing dg directly into a function. If it works as advertised, I am all for it! This actually has been bugging me since we had to start using this many levels of indirection we are using right now.

On a side note, will this also enable us to pass structs such as elements (of type ElementsContainer) to a function instead of unpacking it into u, u_t etc. at the call site?

[ranocha]

In GitLab by @ranocha on May 20, 2020, 07:15

I suggest we first run some tests that confirm that this modification will actually solve the performance issues

Sounds good to me.

This actually has been bugging me since we had to start using this many levels of indirection we are using right now.

Yes, in a sense you're using function barriers as workaround for type instability issues right now.

On a side note, will this also enable us to pass structs to a function instead of unpacking it at the call site?

I hope so, yes.

[ranocha]

In GitLab by @ranocha on May 24, 2020, 10:23

mentioned in merge request !52

[ranocha]

In GitLab by @sloede on May 30, 2020, 06:09

After !52 has been merged, can this be closed @ranocha?

[ranocha]

In GitLab by @ranocha on May 30, 2020, 09:30

I'm not sure. The line element_variables::Dict{Symbol, Union{Vector{Float64}, Vector{Int}}} still looks suspicious to me. Additionally, there are more opportunities to unpack structs inside the function calls and not before.

[ranocha]

Superseded by #177