Conversation
| zb = Base.stack(collect(zb for _ in k)) | ||
| return z .≤ zb | ||
| _zb = Base.stack(collect(zb for _ in k)) | ||
| return z .≤ _zb |
There was a problem hiding this comment.
I honestly don't understand what this code, introduced in #4242, is doing. In particular, as far as I can tell Base.stack is entirely redundant around collect. Anyway, renaming the variable is sufficient to resolve the boxing
There was a problem hiding this comment.
I think this was added for some kind of Enzyme/Reactant reason, @wsmoses
There was a problem hiding this comment.
Yeah it was added to support array input versions
There was a problem hiding this comment.
Just curious how did you find the boxes?
There was a problem hiding this comment.
I used the script in JuliaLang/julia#60478, and I added a slimmed down version of that script to the tests
| minimal_dim_name(var_name, grid, LX, LY, LZ, dim) = | ||
| suffixed_dim_name_generator(var_name, grid, LX, LY, LZ, dim; connector="_", location_letters=minimal_location_string(grid, LX, LY, LZ, dim)) | ||
| minimal_dim_name(var_name, grid::ImmersedBoundaryGrid, args...) = minimal_dim_name(var_name, grid.underlying_grid, args...) |
There was a problem hiding this comment.
This was fun: #4730 (comment). I'm guessing that @tomchor forgot to copy a couple of lines when moving code around in #4730. This is my dumb attempt to resolve the issue, but as I noted in the linked comment this code is untested, so I can't be sure it's doing what it was meant to.
There was a problem hiding this comment.
Yeah, sorry I think that's exactly what happened. You solved it correctly btw. Thanks! I'll add a test for this after this PR is merged.
Also, good to see codecov is back. Super useful and would have prevented this.
| function written_names(filename) | ||
| field_names = String[] | ||
| jldopen(filename, "r") do file | ||
| return jldopen(filename, "r") do file | ||
| all_names = keys(file["timeseries"]) | ||
| field_names = filter(n -> n != "t", all_names) | ||
| filter(n -> n != "t", all_names) | ||
| end | ||
| return field_names | ||
| end |
There was a problem hiding this comment.
This was easy to fix, but I wonder whether this function is used anywhere: it's unused in Oceananigans (besides being exported), it's untested, and not ostensibly used anywhere in Julia ecosystem. Could just be removed if no one is going to miss it?
There was a problem hiding this comment.
It'd be ok to remove this for sure. I thought I would start working on FieldDataset more but never got around to it.
|
I resolved the missing |
|
Just to conclude and provide a concrete motivation for this PR, before: julia> using Oceananigans
julia> code_warntype(Oceananigans.Advection.metaprogrammed_smoothness_operation, (Int,))
MethodInstance for Oceananigans.Advection.metaprogrammed_smoothness_operation(::Int64)
from metaprogrammed_smoothness_operation(buffer) @ Oceananigans.Advection ~/.julia/dev/Oceananigans/src/Advection/weno_interpolants.jl:204
Arguments
#self#::Core.Const(Oceananigans.Advection.metaprogrammed_smoothness_operation)
buffer::Int64
Locals
@_3::Union{Nothing, Tuple{Int64, Int64}}
c_idx@_4::Core.Box
elem::Vector{Any}
#45::Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1"{Int64}
stencil::Int64
stencil_sum::Expr
c_idx@_9::Union{}
c_idx@_10::Union{}
Body::Expr
1 ── nothing
│ (c_idx@_4 = Core.Box())
│ %3 = Oceananigans.Advection.Vector::Core.Const(Vector)
│ %4 = Oceananigans.Advection.undef::Core.Const(UndefInitializer())
│ (elem = (%3)(%4, buffer))
│ %6 = c_idx@_4::Core.Box
│ Core.setfield!(%6, :contents, 1)
│ %8 = Oceananigans.Advection.:(:)::Core.Const(Colon())
│ %9 = Oceananigans.Advection.:-::Core.Const(-)
│ %10 = (%9)(buffer, 1)::Int64
│ %11 = (%8)(1, %10)::Core.PartialStruct(UnitRange{Int64}, Any[Core.Const(1), Int64])
│ (@_3 = Base.iterate(%11))
│ %13 = @_3::Union{Nothing, Tuple{Int64, Int64}}
│ %14 = (%13 === nothing)::Bool
│ %15 = Base.not_int(%14)::Bool
└─── goto #7 if not %15
2 ┄─ %17 = @_3::Tuple{Int64, Int64}
│ (stencil = Core.getfield(%17, 1))
│ %19 = Core.getfield(%17, 2)::Int64
│ %20 = Oceananigans.Advection.Expr::Core.Const(Expr)
│ %21 = (:call, :+)::Core.Const((:call, :+))
│ %22 = Oceananigans.Advection.:(var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1")::Core.Const(Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1")
│ %23 = stencil::Int64
│ %24 = Core._typeof_captured_variable(%23)::Core.Const(Int64)
│ %25 = Core.apply_type(%22, %24)::Core.Const(Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1"{Int64})
│ %26 = c_idx@_4::Core.Box
│ %27 = stencil::Int64
│ (#45 = %new(%25, %26, %27))
│ %29 = #45::Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1"{Int64}
│ %30 = Oceananigans.Advection.:(:)::Core.Const(Colon())
│ %31 = stencil::Int64
│ %32 = (%30)(%31, buffer)::UnitRange{Int64}
│ %33 = Base.Generator(%29, %32)::Base.Generator{UnitRange{Int64}, Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1"{Int64}}
│ (stencil_sum = Core._apply_iterate(Base.iterate, %20, %21, %33))
│ %35 = stencil::Int64
│ %36 = Core._expr(:ref, :ψ, %35)::Expr
│ %37 = stencil_sum::Expr
│ %38 = Core._expr(:call, :*, %36, %37)::Expr
│ %39 = elem::Vector{Any}
│ %40 = stencil::Int64
│ Base.setindex!(%39, %38, %40)
│ %42 = Oceananigans.Advection.:+::Core.Const(+)
│ %43 = c_idx@_4::Core.Box
│ %44 = Core.isdefined(%43, :contents)::Bool
└─── goto #4 if not %44
3 ── goto #5
4 ── Core.NewvarNode(:(c_idx@_9))
└─── c_idx@_9
5 ┄─ %49 = c_idx@_4::Core.PartialStruct(Core.Box, Any[Any])
│ %50 = Core.getfield(%49, :contents)::Any
│ %51 = Oceananigans.Advection.:+::Core.Const(+)
│ %52 = Oceananigans.Advection.:-::Core.Const(-)
│ %53 = stencil::Int64
│ %54 = (%52)(buffer, %53)::Int64
│ %55 = (%51)(%54, 1)::Int64
│ %56 = (%42)(%50, %55)::Any
│ %57 = c_idx@_4::Core.PartialStruct(Core.Box, Any[Any])
│ Core.setfield!(%57, :contents, %56)
│ (@_3 = Base.iterate(%11, %19))
│ %60 = @_3::Union{Nothing, Tuple{Int64, Int64}}
│ %61 = (%60 === nothing)::Bool
│ %62 = Base.not_int(%61)::Bool
└─── goto #7 if not %62
6 ── goto #2
7 ┄─ %65 = Core._expr(:ref, :ψ, buffer)::Expr
│ %66 = Core._expr(:ref, :ψ, buffer)::Expr
│ %67 = c_idx@_4::Core.Box
│ %68 = Core.isdefined(%67, :contents)::Bool
└─── goto #9 if not %68
8 ── goto #10
9 ── Core.NewvarNode(:(c_idx@_10))
└─── c_idx@_10
10 ┄ %73 = c_idx@_4::Core.PartialStruct(Core.Box, Any[Any])
│ %74 = Core.getfield(%73, :contents)::Any
│ %75 = Core._expr(:ref, :C, %74)::Expr
│ %76 = Core._expr(:call, :*, %65, %66, %75)::Expr
│ %77 = elem::Vector{Any}
│ Base.setindex!(%77, %76, buffer)
│ %79 = Oceananigans.Advection.Expr::Core.Const(Expr)
│ %80 = (:call, :+)::Core.Const((:call, :+))
│ %81 = elem::Vector{Any}
│ %82 = Core._apply_iterate(Base.iterate, %79, %80, %81)::Expr
└─── return %82After this PR: julia> using Oceananigans
julia> code_warntype(Oceananigans.Advection.metaprogrammed_smoothness_operation, (Int,))
MethodInstance for Oceananigans.Advection.metaprogrammed_smoothness_operation(::Int64)
from metaprogrammed_smoothness_operation(buffer) @ Oceananigans.Advection ~/.julia/dev/Oceananigans/src/Advection/weno_interpolants.jl:204
Arguments
#self#::Core.Const(Oceananigans.Advection.metaprogrammed_smoothness_operation)
buffer::Int64
Locals
@_3::Union{Nothing, Tuple{Int64, Int64}}
c_idx::Int64
elem::Vector{Expr}
#45::Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1"{Int64, Int64}
c::Int64
stencil::Int64
stencil_sum::Expr
Body::Expr
1 ─ nothing
│ %2 = Oceananigans.Advection.Vector::Core.Const(Vector)
│ %3 = Oceananigans.Advection.Expr::Core.Const(Expr)
│ %4 = Core.apply_type(%2, %3)::Core.Const(Vector{Expr})
│ %5 = Oceananigans.Advection.undef::Core.Const(UndefInitializer())
│ (elem = (%4)(%5, buffer))
│ (c_idx = 1)
│ %8 = Oceananigans.Advection.:(:)::Core.Const(Colon())
│ %9 = Oceananigans.Advection.:-::Core.Const(-)
│ %10 = (%9)(buffer, 1)::Int64
│ %11 = (%8)(1, %10)::Core.PartialStruct(UnitRange{Int64}, Any[Core.Const(1), Int64])
│ (@_3 = Base.iterate(%11))
│ %13 = @_3::Union{Nothing, Tuple{Int64, Int64}}
│ %14 = (%13 === nothing)::Bool
│ %15 = Base.not_int(%14)::Bool
└── goto #4 if not %15
2 ┄ %17 = @_3::Tuple{Int64, Int64}
│ (stencil = Core.getfield(%17, 1))
│ %19 = Core.getfield(%17, 2)::Int64
│ %20 = c_idx::Int64
│ (c = %20)
│ %22 = Oceananigans.Advection.Expr::Core.Const(Expr)
│ %23 = (:call, :+)::Core.Const((:call, :+))
│ %24 = Oceananigans.Advection.:(var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1")::Core.Const(Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1")
│ %25 = c::Int64
│ %26 = Core._typeof_captured_variable(%25)::Core.Const(Int64)
│ %27 = stencil::Int64
│ %28 = Core._typeof_captured_variable(%27)::Core.Const(Int64)
│ %29 = Core.apply_type(%24, %26, %28)::Core.Const(Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1"{Int64, Int64})
│ %30 = c::Int64
│ %31 = stencil::Int64
│ (#45 = %new(%29, %30, %31))
│ %33 = #45::Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1"{Int64, Int64}
│ %34 = Oceananigans.Advection.:(:)::Core.Const(Colon())
│ %35 = stencil::Int64
│ %36 = (%34)(%35, buffer)::UnitRange{Int64}
│ %37 = Base.Generator(%33, %36)::Base.Generator{UnitRange{Int64}, Oceananigans.Advection.var"#metaprogrammed_smoothness_operation##0#metaprogrammed_smoothness_operation##1"{Int64, Int64}}
│ (stencil_sum = Core._apply_iterate(Base.iterate, %22, %23, %37))
│ %39 = stencil::Int64
│ %40 = Core._expr(:ref, :ψ, %39)::Expr
│ %41 = stencil_sum::Expr
│ %42 = Core._expr(:call, :*, %40, %41)::Expr
│ %43 = elem::Vector{Expr}
│ %44 = stencil::Int64
│ Base.setindex!(%43, %42, %44)
│ %46 = Oceananigans.Advection.:+::Core.Const(+)
│ %47 = c_idx::Int64
│ %48 = Oceananigans.Advection.:+::Core.Const(+)
│ %49 = Oceananigans.Advection.:-::Core.Const(-)
│ %50 = stencil::Int64
│ %51 = (%49)(buffer, %50)::Int64
│ %52 = (%48)(%51, 1)::Int64
│ (c_idx = (%46)(%47, %52))
│ (@_3 = Base.iterate(%11, %19))
│ %55 = @_3::Union{Nothing, Tuple{Int64, Int64}}
│ %56 = (%55 === nothing)::Bool
│ %57 = Base.not_int(%56)::Bool
└── goto #4 if not %57
3 ─ goto #2
4 ┄ %60 = Core._expr(:ref, :ψ, buffer)::Expr
│ %61 = Core._expr(:ref, :ψ, buffer)::Expr
│ %62 = c_idx::Int64
│ %63 = Core._expr(:ref, :C, %62)::Expr
│ %64 = Core._expr(:call, :*, %60, %61, %63)::Expr
│ %65 = elem::Vector{Expr}
│ Base.setindex!(%65, %64, buffer)
│ %67 = Oceananigans.Advection.Expr::Core.Const(Expr)
│ %68 = (:call, :+)::Core.Const((:call, :+))
│ %69 = elem::Vector{Expr}
│ %70 = Core._apply_iterate(Base.iterate, %67, %68, %69)::Expr
└── return %70Note how |
|
What are |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5133 +/- ##
==========================================
- Coverage 72.59% 72.35% -0.25%
==========================================
Files 391 391
Lines 21578 21560 -18
==========================================
- Hits 15665 15600 -65
- Misses 5913 5960 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
navidcy
left a comment
navidcy
left a comment
There was a problem hiding this comment.
This feels like an excellent cleanup. I'm leaning to approve. I just don't feel quite confident that I can be critical enough for this PR.
Boxing is a generic programming strategy where a variable is turned into a reference. The Julia compiler uses boxing in some cases when inference fails (for example because of variable reassignment or capture in a closure, in Oceananigans there were cases where both were happening at the same time), but this causes type instability (inference failure at some point prevents inference down the line) preventing potential performance optimisations and bringing extra memory allocations (that's generally the case with type instability) and more compilation latency. Now, I don't think any of the changes in this PR brought significant performance boosts (CI time is more or less the same as before), but removing boxes is still a net win which makes the compiler's life easier overall (see the example above where |
| _F = F[1:TF] | ||
|
|
||
| Δᶜ = [F[i + 1] - F[i] for i = 1:TF-1] | ||
| Δᶜ = [_F[i + 1] - _F[i] for i = 1:TF-1] |
There was a problem hiding this comment.
what does the underscore prefix denote?
There was a problem hiding this comment.
Nothing, just changing the name not to always reuse the same name, which is the entire problem.
There was a problem hiding this comment.
Yeap...
I guess the convention is that _ prefix is used for internal variable/method names, right?
There was a problem hiding this comment.
So F is a longer vector, and then _F is trimmed (reading the comment above). maybe trimmed_F or something like that?
There was a problem hiding this comment.
Yes, but in this case I really just wanted "another name", and prefixing with the underscore was the dumbest thing I could think of. If you have better names, let me know! The important is not do things like
X = f(X)or
X = [f(X) for _ in Y]where a label depends on itself, and here the compiler currently really can't disentangle the two names (hopefully in the future this will be resolved, but this is what is now).
Note how above there was
and then
and then
The same name
F was used multiple times, for different things, each of them depending on the same name.
There was a problem hiding this comment.
I just want to encourage using descriptive names. Reusing names can sometimes produce readable code. I wonder if we want to avoid default to generic name mangling and encourage using descriptive name mangling?
There was a problem hiding this comment.
Reusing same name for different things isn't a tenable strategy, especially in the presence of closures, not with the current parser/lowering system. There's hope JuliaLowering.jl will fix JuliaLang/julia#15276, but that's for the future, won't help with existing versions.
Anyway, I renamed _F to trimmed_F
There was a problem hiding this comment.
Reusing same name for different things isn't a tenable strategy, especially in the presence of closures, not with the current parser/lowering system.
Sorry, that's what I was trying to point out. The purpose of that design was readability, so given this new tenet which declares that design invalid, we should find another strategy to maintain readability.
There was a problem hiding this comment.
I made some other variable names (hopefully!) clearer.
|
Should this speed up precompilation? |
Exactly my thinking/question! It seems that it should, right @giordano? |
|
Potentially, but in practice I didn't see any significant difference one way or the other. As I mentioned above, CI time is also pretty much the same, within the usual large jitter of a busy system. |
|
Someone observed marginal memory reduction doing the same work in Makie: MakieOrg/Makie.jl#5475 (comment). At least here in Oceananigans there weren't that many boxes anyway, 25 or so when I started, and probably not in critical paths. |
|
@navidcy there's a test failure, but because of the code coverage stuff it's not actually failing the job, so all tests are "passing" now. |
| break | ||
| end | ||
| end | ||
| M★ = something(findlast(>(0), averaging_weights), firstindex(averaging_weights)) |
There was a problem hiding this comment.
I am not sure how this syntax works, but does it produce the same effect?
There was a problem hiding this comment.
it's kind of obscure 😅
There was a problem hiding this comment.
It should: findlast(>(0), averaging_weights) is almost the same as the loop that was there before, except it returns nothing in case it doesn't find anything, which is why I added something(..., firstindex(averaging_weights)
6 participants
Similar to NumericalEarth/Breeze.jl#400 in spirit, this PR gets rid of
almostall of theCore.Boxes in OceananigansThe only remaining issue isc_idxwhich is supposedly being boxed inOceananigans.jl/src/Advection/weno_interpolants.jl
Lines 204 to 216 in 7e4ffb6
but frankly I'm struggling to see what's the issue.Edit: now resolved, allCore.Boxes that can be detected statically are gone.Most of the issues are things like
Whenever you reuse the same label for different things, and especially when the label is used both on the right- and left-hand sides, the compiler struggles to complete inference. Most of the changes here are simply about renaming variables.