change @allocated again

[JeffBezanson]

Also add donotdelete to ensure the code actually runs.

fixes #58780

[JeffBezanson]

From the test failures:

• @test (() -> @allocations "a" * "b")() == 0 # constant propagation
  I believe this is wrong. We weren't constprop'ing this, just deleting it. I also think @allocations shouldn't work this way --- to me this is asking "does string * allocate?" and the answer is yes, so I think giving 0 is misleading. If you want to check the exact code written, you can use @allocations (()->"a"*"b")(). Picking one of those forms or the other is quite easy, as opposed to trying to trick the compiler into not doing constprop so you can check whether your code allocates generally.

So I want to mark the test functions as constprop :none, but I'm a bit worried this will break many package tests since some of them probably depend on the constprop, possibly intentionally. Unfortunately the old implementation of @allocated wrapped the entire expression in a force_compile block, which meant that (1) the function and argument expressions would be optimized together and you can get constprop, but (2) at the toplevel the arguments could get boxed, adding spurious allocations. So it was wrong in both directions.

If we want the constprop behavior, then I think the right thing is to wrap the given expression in a closure every time to make sure it is always optimized as a function on its own. But then you are going to get a fair number of false negatives.

[vtjnash]

This does not fix #58780, since the example there looks like this:

    a = @allocated if d_rotor[end] > 0
        RPl, RPw = discretizeRotor(floris.rotor_points)
    else
        RPl = [0.0 0.0 0.0]
        RPw = [1.0]
    end

(small nit: making the first line of the commit message say "fix #xxxxx" is not useful, since the number is meaningless to others, but it reduces the amount of useful descriptive text you can put in that line before it may get truncated, and you have to repeat that number in the body anyways)

[JeffBezanson]

Where is that from? That example is not in the issue. And unfortunately I don't see any way to make that work.

[vtjnash]

It is the code posted for the issue to be reopened (#58780 (comment))

[JeffBezanson]

Goodness, I never imagined anyone would use this in-line with their code. It's intended for testing and profiling. I don't think there's any way to support both use cases except to have two macros.

In that example, what you want is basically

b0 = gcbytes()
... run arbitrary code ...
return gcbytes() - b0

where every byte allocated in that region, for whatever reason, is captured. But that is not what most users of @allocated want. They want to know whether a certain function allocates, skipping the overhead of calling it in context.

I agree that the first use is compatible with the docstring, which says "during evaluation of the expression". But there are insane numbers of tests out there assuming a different meaning. I propose:

• We put this macro back to what it was in 1.11, since then at least it's compatible
• We introduce a new macro or argument to this macro that does what's in this PR
• Those who want the surgical inside-the-function behavior need to gradually transition to the new macro when they get 32 == 0 test failures
• The new macro can be much stricter and require a simple call expression so it is unambiguous

[vtjnash]

We put this macro back to what it was in 1.11, since then at least it's compatible

I think we had changed that because that will break a lot of tests

[JeffBezanson]

In that case the only way to fix it would be to have some way to avoid the latestworld updates between statements.

[JeffBezanson]

Actually this was only a problem inside testsets, since the code they generate is so complex. Maybe there is a workaround possible there.

Update: turns out this is only caused by our deliberately inserting world updates in testsets. I don't know if we can just remove that, or maybe make it an option?

[JeffBezanson]

OK here's another proposal: use the new implementation for call expressions, for anything more complex use the old implementation. That might reduce the failing cases to an acceptably low level.

[vtjnash]

I believe that was tried before, and failed several packages also. "Just a call" is far more restrictive than people assume, since it is just exactly one call--not even fusion is possible. I also have a branch that supports the generalized version which does support multiple calls, before I concluded that heuristic mess is a bad idea

[JeffBezanson]

Yeah, any sort of heuristic will never fully work. I think long term the best thing is to have two macros, one that works like the old @allocated and one for looking inside a function like code_typed.

In the meantime, I believe this could be fixed by fixing #58511; then we would not need world age updates in testsets and it will generally work much more like it did before.

[j-fu]

Why is it that @allocated can return a nonzero value when @allocations returns 0 ?
See an MWE here: #58634 (comment) .

[JeffBezanson]

It looks like the compiler is able to delete the allocation of the ReshapedArray in one case but not the other, amazingly, due to the slightly different code in the two macros. I'll see if I can fix this.

[JeffBezanson]

In the linked MWE, the trouble is that Y is not really needed so it is easy-ish for the compiler to delete it, but the compiler is also pretty easily confused. That kind of code pattern will probably always be prone to unexpected results from these macros. For now, I was able to work around it by making the code for allocations artificially more similar to allocated to make it less likely they will be optimized differently.

Ready for review.

[JeffBezanson]

It looks like we might need to subtract allocations from the compiler. But, I'm not sure it's worth it since in those cases you probably get argument boxing as well so allocated == 0 will still fail.

[JeffBezanson]

OK I came up with another good trick and now I think the behavior is very accurate and predictable (see tests). You will get different numbers sometimes, but that was going to happen anyway. The good part is you now have pretty good control over what happens, without any new features. If you were relying on the call site getting optimized to get 0 allocations, you can just wrap in an extra closure. If you get spurious new allocations, refactor to measure call sites individually or wrap in a closure. If you want to see spurious allocations from argument boxing, wrap your call in a begin...end block 😛

[vtjnash]

IIRC, I didn't use this form since it didn't fix most of the PkgEval failures, so we should check on that

[vtjnash]

There was also a more advanced version of this PR in the original PR (#58057 (comment)) that did fix more PkgEval failures, but I thought it was getting a bit too complicated and magical (I think I have a branch somewhere with that comment turned into a working example)

resolved I think