Hoist some getproperty calls inside broadcast expr

[charleskawczynski]

This PR should improve performance a bit via manual code hoisting, see #341.

I can confirm that this reduces the number of JET complaints.

[charleskawczynski]

bors try

[jakebolewski]

It is annoying that we have to do this but LGTM

[jakebolewski]

I wonder if we construct the property views directly (with Subarray bypassing all the checks) if that would help....

[charleskawczynski]

It is annoying that we have to do this but LGTM

Yeah, at least the fix is easy, though. Also, one nice thing that I've found with doing this is that it helps decouple the structure of the state vector (Y), which is nice.

I wonder if we construct the property views directly (with Subarray bypassing all the checks) if that would help....

Maybe, I'm still looking through the JET results..

[charleskawczynski]

bors r+

[jakebolewski]

We should just do it consistently in any case, I'll go through and make that change.

[charleskawczynski]

bors r-

[bors]

Canceled.

[jakebolewski]

Could you try?

diff --git a/src/DataLayouts/DataLayouts.jl b/src/DataLayouts/DataLayouts.jl
index 0ddeba0..20eb00d 100644
--- a/src/DataLayouts/DataLayouts.jl
+++ b/src/DataLayouts/DataLayouts.jl
@@ -869,13 +869,16 @@ end
     data::VIFH{S, Ni, A},
     ::Val{Idx},
 ) where {S, Ni, A, Idx}
+    array = parent(data)
+    Nv = size(array, 1)
+    Nh = size(array, 4)
     SS = fieldtype(S, Idx)
     T = eltype(A)
     offset = fieldtypeoffset(T, S, Idx)
     nbytes = typesize(T, SS)
     field_byterange = (offset + 1):(offset + nbytes)
     return :(VIFH{$SS, $Ni}(
-        @inbounds view(parent(data), :, :, $field_byterange, :)
+        @inbounds SubArray(parent(data), Base.Slice(Base.OneTo($Nv)), Base.Slice(Base.OneTo($Ni)), $field_byterange, Base.Slice(Base.OneTo($Nh)))
     ))
 end

[charleskawczynski]

Could you try? ...

I'll try this in a follow up PR, so that we know what perf changes are attributed to.

[jakebolewski]

actually with a generated function, that doesn't work if we don't have the size extents in the type :-/. We could try it with the getproperty call that isn't a generated function by switching the impl at the top of that file. It might be good to revisit if we even need these again.

[charleskawczynski]

bors r+

[bors]

try

Build failed:

• buildkite/climacore-ci

[jakebolewski]

Releated: #11 (comment)

[bors]

Build succeeded:

• buildkite/climacore-ci
• docbuild
• format
• lib-climacore-makie
• lib-climacore-plots
• lib-climacore-vtk
• test-os (macos-latest)
• test-os (ubuntu-latest)
• test-os (windows-latest)