Skip to content
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

Support @check_allocs at callsites #54

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tecosaur
Copy link

This is an alternative to #46 that resolves #45 by creating an anonymous function for a given call, allowing @check_allocs to be applied to callsites.

julia> @check_allocs 1 + 1
2

is equivalent to

julia> @check_allocs f(a, b) = a + b
f (generic function with 1 method)

julia> f(1, 1)
2

This mechanism is different to #46, as that is based on calling check_allocs with the argument types.

@fredrikekre
Copy link
Member

What is the advantage of this version? I find this one a bit confusing -- why is the extra anonymous function layer needed for? No other similar macro (e.g. @code_typed, @code_llvm, ...) do it like this but instead do what #46 does.

@tecosaur
Copy link
Author

Doing it via an anonymous function is done in response to 875e3ba:

If you rely on allocation-free code for safety/correctness, it is not sufficient
to verify check_allocs in test code and expect that the corresponding call in
production will not allocate at runtime.

For this case, you must use @check_allocs instead.

Otherwise I'd just use check_allocs as #46 does, and the anonymous function layer wouldn't be needed.

@fredrikekre
Copy link
Member

Okay, I missed that. Looking at the expanded code, it seems at least possible to use the called function directly (e.g. use + instead of the fn_alias in your example). Perhaps it doesn't really matter though. Is any of this visible in e.g. a stacktrace?

Tangentially, would it be possible to add this type of indirection to check_alloc to make that stable? Perhaps that uses a completely different machinery though, I don't know.

@tecosaur
Copy link
Author

It seems like the stack trace is much less helpful, but it's like that of a @check_alloc function.

julia> @check_allocs rand(2,2) * rand(2, 2)
ERROR: @check_alloc function encountered 1 errors (1 allocations / 0 dynamic dispatches).
Stacktrace:
 [1] macro expansion
   @ ~/.julia/dev/AllocCheck/src/macro.jl:159 [inlined]
 [2] (::var"#8#10"{var"#7#9"})(arg#231::Matrix{Float64}, arg#232::Matrix{Float64})
   @ Main ./REPL[3]:158
 [3] macro expansion
   @ ~/.julia/dev/AllocCheck/src/macro.jl:188 [inlined]
 [4] top-level scope
   @ REPL[3]:1

julia> @check_allocs mymul(a, b) = a * b
mymul (generic function with 1 method)

julia> mymul(rand(2, 2), rand(2, 2))
ERROR: @check_alloc function encountered 1 errors (1 allocations / 0 dynamic dispatches).
Stacktrace:
 [1] macro expansion
   @ ~/.julia/dev/AllocCheck/src/macro.jl:159 [inlined]
 [2] mymul(a::Matrix{Float64}, b::Matrix{Float64})
   @ Main ./REPL[6]:158
 [3] top-level scope
   @ REPL[7]:1

Copy link
Member

@topolarity topolarity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @tecosaur! Sorry for the late response here.

It's been on my list to approach this PR from a slightly different direction, but in case you're interested here's what I've been thinking: I think our kwargs handling needs re-working (esp. in light of the bug you found in #52).

Mostly I think think that means handling kwargs by replacing a call to f with a call to Core.kwcall(merged_kwargs, f, ...), at which point our wrapper function would simply forward all received arguments and the call-site would not need a wrapper at all.

I think that would also fix the stack trace issues here.

# Callsite forms
@test 1 + x == @check_allocs 1 + x
@test x^2 == @check_allocs (a -> a^2)(x)
@test_throws AllocCheck.AllocCheckFailure @check_allocs same_ccall()
Copy link
Member

@topolarity topolarity Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spirit of your recent issues, I double-checked and I don't think this handles kwargs correctly right now:

julia> foo(a,b;c=1) = a + b + c
foo (generic function with 1 method)
julia> @check_allocs foo(1,2;c=50)
4
julia> foo(1,2;c=50)
53

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this check! I'm currently trying to work out why exactly this is happening. With @macroexpand and a @show invocation, this is what I see with your example:

@show local variables
ex = :(foo(1, 2; c = 30))
mod = Main
source = :(#= REPL[16]:1 =#)
args = Any[:($(Expr(:parameters, :($(Expr(:kw, :c, 30)))))), 1, 2]
args_template = Any[:($(Expr(:parameters, :c))), Symbol("##arg#255"), Symbol("##arg#256")]
passthrough_defun = :(function (var"##arg#255", var"##arg#256"; c)
      foo(var"##arg#255", var"##arg#256"; c)
  end)
original_fn = :(function (var"##arg#255", var"##arg#256", c;)
      foo(var"##arg#255", var"##arg#256"; )
  end)
f_sym = Symbol("##fn_alias#257")
wrapper_fn = :(function ($(Expr(:escape, :(var"##arg#255"::Any))), $(Expr(:escape, :(var"##arg#256"::Any))); $(Expr(:escape, :c)))
      #= REPL[16]:1 =#
      callable_tt = Tuple{map(Core.Typeof, ($(Expr(:escape, Symbol("##arg#255"))), $(Expr(:escape, Symbol("##arg#256"))), $(Expr(:escape, :c))))...}
      #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:157 =#
      callable = (AllocCheck.compile_callable)(var"##fn_alias#257", callable_tt; ignore_throw = true)
      #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:158 =#
      if length(callable.analysis) > 0
          #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:159 =#
          throw(AllocCheckFailure(callable.analysis))
      end
      #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:161 =#
      callable($(Expr(:escape, Symbol("##arg#255"))), $(Expr(:escape, Symbol("##arg#256"))), $(Expr(:escape, :c)))
  end)

@macroexpand
quote
    #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:188 =#
    let var"#98###fn_alias#257" = function (var"##arg#255", var"##arg#256", c;)
                foo(var"##arg#255", var"##arg#256"; )
            end
        #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:189 =#
        var"#99###alloccheck_fn#260" = function (var"##arg#255"::Any, var"##arg#256"::Any; c)
                #= REPL[16]:1 =#
                var"#102#callable_tt" = AllocCheck.Tuple{AllocCheck.map((AllocCheck.Core).Typeof, (var"##arg#255", var"##arg#256", c))...}
                #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:157 =#
                var"#103#callable" = (AllocCheck.compile_callable)(var"#98###fn_alias#257", var"#102#callable_tt"; ignore_throw = true)
                #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:158 =#
                if AllocCheck.length((var"#103#callable").analysis) > 0
                    #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:159 =#
                    AllocCheck.throw(AllocCheck.AllocCheckFailure((var"#103#callable").analysis))
                end
                #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:161 =#
                var"#103#callable"(var"##arg#255", var"##arg#256", c)
            end
        #= /home/tec/.julia/dev/AllocCheck/src/macro.jl:190 =#
        var"#99###alloccheck_fn#260"(1, 2; c = 30)
    end
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I'm a bit stuck on this, and help from someone else would be good to move this forward.

quote
let $f_sym = $(esc(original_fn))
$af_sym = $wrapper_fn
$(Expr(:call, af_sym, map(esc, args)...))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would $af_sym($(esc.(args)...)) work just as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd think so, it's just a difference in coding style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call-site macro
3 participants