Skip to content

Conversation

@vtjnash
Copy link
Member

@vtjnash vtjnash commented Apr 30, 2025

Refs #58057

@odow
Copy link
Contributor

odow commented Apr 30, 2025

Is this the right fix? x-ref #58057 (comment)

julia> import MathOptInterface as MOI

julia> using Test

julia> function my_is_simply_call(Base.@nospecialize ex)
           Meta.isexpr(ex, :call) || return false
           for a in ex.args
               a isa QuoteNode && continue
               a isa Symbol && continue
               Base.is_self_quoting(a) && continue
               Base.is_quoted(a) && continue
               return false
           end
           return true
       end
my_is_simply_call (generic function with 1 method)

julia> macro my_allocated(ex)
           ex = macroexpand(__module__, ex)
           if !my_is_simply_call(ex)
               ex = :((() -> $ex)())
           end
           pushfirst!(ex.args, Base.GlobalRef(Base, :allocated))
           return esc(ex)
       end
@my_allocated (macro with 1 method)

julia> function test_main_captured_x()
           r = MOI.Nonlinear.OperatorRegistry()
           x = [1.1, 1.0]
           H = zeros(2, 2)
           ret = @my_allocated MOI.Nonlinear.eval_multivariate_hessian(r, :^, H, x)
           x = [1.1, 2.0]
           return ret
       end
test_main_captured_x (generic function with 1 method)

julia> test_main_captured_x()
112

julia> test_main_captured_x()
112

@oscardssmith oscardssmith added the needs tests Unit tests are required for this change label Apr 30, 2025
@vtjnash
Copy link
Member Author

vtjnash commented Apr 30, 2025

Yes, but you're still not allowed to have highly complicated expressions with arbitrary side-effects (MOI.Nonlinear.eval_multivariate_hessian) to benefit from this automatic simple-call simplification

@odow
Copy link
Contributor

odow commented Apr 30, 2025

but you're still not allowed to have highly complicated expressions with arbitrary side-effects

I'm not sure I understand this part. The code does not allocate if I remove the @allocated macro. Previously, @allocated reported 0. Now it reports > 0? That seems like a break in behavior, even if the fix is intended to allow optimizations for other code.

@JeffBezanson
Copy link
Member

Maybe we should just make @allocated officially only operate on the called function, i.e. allocations in argument expressions are not counted. That's a more dramatic change but it's probably what is wanted most of the time.

@vtjnash
Copy link
Member Author

vtjnash commented May 1, 2025

Perhaps most of the time, but it would be a breaking change for several packages (not all packages measure a call expression)

@JeffBezanson
Copy link
Member

Fair enough. But the unintuitive thing here is that A.B.C is seen as a name, and not a computation. If you switch it to

                  f = MOI.Nonlinear.eval_multivariate_hessian
                  ret = @my_allocated f(r, :^, H, x)

you get 0 as expected. Maybe the right heuristic is just "is the expression a call"? Then if you really want to measure everything, all you need to do is wrap it in begin...end.

Cause it to capture all values of any expression (even values of
globals), not just values passed to a single call.
@vtjnash vtjnash force-pushed the jn/58057-quoted branch from e333199 to ed1d4fb Compare May 1, 2025 20:20
@vtjnash vtjnash closed this May 1, 2025
@vtjnash
Copy link
Member Author

vtjnash commented May 1, 2025

Shoot, if it needs a test and all that, this PR isn't worth it anymore. There's too many cases in the wild already doing various things. The PR currently breaks because we don't know that things are globals, and capturing those as arguments fails very badly for inference if they turn out to have been a Module.

@giordano giordano deleted the jn/58057-quoted branch October 30, 2025 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs tests Unit tests are required for this change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants