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

@allocated false positive on v1.12? #57064

Open
nsajko opened this issue Jan 16, 2025 · 10 comments
Open

@allocated false positive on v1.12? #57064

nsajko opened this issue Jan 16, 2025 · 10 comments
Labels
regression Regression in behavior compared to a previous version regression 1.12 Regression in the 1.12 release testsystem The unit testing framework and Test stdlib
Milestone

Comments

@nsajko
Copy link
Contributor

nsajko commented Jan 16, 2025

This passes on v1.11, but fails on v1.12:

using Test
function f(r)
    sum(r) 
end
@testset "t" begin
    @test 15 == f(1:5) 
    @test 0 == @allocated f(1:5) 
end

The workaround is:

const r = 1:5
@testset "t" begin
    @test 15 == f(r) 
    @test 0 == @allocated f(r) 
end
@nsajko nsajko added regression Regression in behavior compared to a previous version regression 1.12 Regression in the 1.12 release labels Jan 16, 2025
@nsajko nsajko added the testsystem The unit testing framework and Test stdlib label Jan 16, 2025
@Seelengrab
Copy link
Contributor

Is this really a regression? The range is allocated within @allocated after all. There is no stackframe it could be allocated on, so it needs to be done on the heap and thus counted.

@nsajko
Copy link
Contributor Author

nsajko commented Jan 16, 2025

Why did it work before v1.12, though? Are you saying this may have been a bugfix in v1.12, and it basically only works by accident in v1.11?

@nsajko
Copy link
Contributor Author

nsajko commented Jan 16, 2025

Another thing to note is that this is connected to Test somehow, because @allocated behaves as expected when called outside @testset. Furthermore, even inside @testset it works fine in some cases:

julia> function f(r)
           sum(r) 
       end
f (generic function with 1 method)

julia> using Test

julia> @testset "t" begin
           @test 0 == @allocated f(1:5)
       end;
Test Summary: | Pass  Total  Time
t             |    1      1  0.5s

julia> @testset "t" begin
           @test 15 == f(1:5)
           @test 0 == @allocated f(1:5)
       end;
t: Test Failed at REPL[4]:3
  Expression: 0 == #= REPL[4]:3 =# @allocated(f(1:5))
   Evaluated: 0 == 32

As you can see, the only difference between the failing and the passing test set is the seemingly-unrelated additional test.

@LilithHafner
Copy link
Member

Slightly more minimal MWE:

x@x:~$ julia
  o  | Version 1.11.2 (2024-12-01)
 o o | Official https://julialang.org/ release
julia> using Test

julia> @testset "t" begin
           @test 0 == @allocated sum(1:5)
       end;
Test Summary: | Pass  Total  Time
t             |    1      1  0.0s

julia> @testset "t" begin
           @test true
           @test 0 == @allocated sum(1:5)
       end;
Test Summary: | Pass  Total  Time
t             |    2      2  0.0s

julia> 
x@x:~$ julia +nightly
  o  | Version 1.12.0-DEV.1896 (2025-01-16)
 o o | Commit 2dc3337626c (0 days old master)
julia> @testset "t" begin
           @test 0 == @allocated sum(1:5)
       end;
Test Summary: | Pass  Total  Time
t             |    1      1  0.2s

julia> @testset "t" begin
           @test true
           @test 0 == @allocated sum(1:5)
       end;
t: Test Failed at REPL[2]:3
  Expression: 0 == #= REPL[2]:3 =# @allocated(sum(1:5))
   Evaluated: 0 == 5872

Stacktrace:
  [1] macro expansion
    @ ~/.julia/juliaup/julia-nightly/share/julia/stdlib/v1.12/Test/src/Test.jl:679 [inlined]
...
 [22] _start()
    @ Base ./client.jl:568
Test Summary: | Pass  Fail  Total  Time
t             |    1     1      2  1.2s
RNG of the outermost testset: Random.Xoshiro(0x2bf6d82096191994, 0x63e033bf3f918fda, 0x8450a16e9913f641, 0xcbc3aefcc9a646b0, 0x7f846370d5bd8737)
ERROR: Some tests did not pass: 1 passed, 1 failed, 0 errored, 0 broken.

@d-netto
Copy link
Member

d-netto commented Jan 22, 2025

Bisected this using the scripts below:

  • mwe.jl:
using Test

@testset "Check" begin
    str = "hello bello"
    @show @allocated hash(str)
    @show @allocated hash(str)
end
  • bisect.jl:
const GOOD_COMMIT_HASH = "a127ab7ceeb543483aa8cb86e985c32556c60248"
const BAD_COMMIT_HASH = "f91436eae7265a01bff17e35887cd9b8e15c8fdc"

const MWE_PATH = "..."
const JULIA_MASTER_BIN_PATH = "..."

const MANIFEST_PATH = "..."

function init_bisect()
    @info "Initializing bisect"
    run(`git reset --hard $BAD_COMMIT_HASH`)
    run(`git bisect reset`)
    run(`git bisect start`)
    run(`git checkout $GOOD_COMMIT_HASH`)
    run(`git bisect good`)
    run(`git checkout $BAD_COMMIT_HASH`)
    run(`git bisect bad`)
end

function bisect_has_finished()
    @info "Checking if bisect has finished"
    # grep for the string `is the first bad commit` in the output of the bisect log
    return contains(read(`git bisect log`, String), "is the first bad commit")
end

function run_bisect()
    while !bisect_has_finished()
        # clean and build the julia master branch
        run(`make cleanall`)
        run(`make -j8`)
        # create temporary output file
        run(`touch bisect_output.txt`)
        # rm the Manifest.toml
        rm("$MANIFEST_PATH", force=true)
        # run the MWE and save the output to the temporary files
        run(pipeline(`$JULIA_MASTER_BIN_PATH $MWE_PATH`, `tee bisect_output.txt`))
        # count the number of occurences of `@allocated(hash(str)) = 0` in the output
        n = count(r"@allocated\(hash\(str\)\) = 0", read("bisect_output.txt", String))
        @info "Number of occurences of @allocated(hash(str)) = 0: $n"
        if n == 2
            run(`git bisect good`)
        else
            run(`git bisect bad`)
        end
        rm("bisect_output.txt")
    end
end


function main()
    init_bisect()
    run_bisect()
end

main()

And got:

034e6093c53ce2aae989045cfd5942dade27198b is the first bad commit
commit 034e6093c53ce2aae989045cfd5942dade27198b
Author: Keno Fischer <[email protected]>
Date:   Wed Nov 20 23:51:38 2024 -0500

    Make world-age increments explicit (#56509)
    
    This PR introduces a new, toplevel-only, syntax form `:worldinc` that
    semantically represents the effect of raising the current task's world
    age to the latest world for the remainder of the current toplevel
    evaluation (that context being an entry to `eval` or a module
    expression). For detailed motivation on why this is desirable, see
    #55145, which I won't repeat here, but the gist is that we never really
    defined when world-age increments and worse are inconsistent about it.
    This is something we need to figure out now, because the bindings
    partition work will make world age even more observable via bindings.
    
    Having created a mechanism for world age increments, the big question is
    one of policy, i.e. when should these world age increments be inserted.
    
    Several reasonable options exist:
    1. After world-age affecting syntax constructs (as proprosed in #55145)
    2. Option 1 + some reasonable additional cases that people rely on
    3. Before any top level `call` expression
    4. Before any expression at toplevel whatsover
    
    As an example, case, consider `a == a` at toplevel. Depending on the
    semantics that could either be the same as in local scope, or each of
    the four world age dependent lookups (three binding lookups, one method
    lookup) could (potentially) occur in a different world age.
    
    The general tradeoff here is between the risk of exposing the user to
    confusing world age errors and our ability to optimize top-level code
    (in general, any `:worldinc` statement will require us to fully
    pessimize or recompile all following code).
    
    This PR basically implements option 2 with the following semantics:
    
    1. The interpreter explicit raises the world age only at `:worldinc`
    exprs or after `:module` exprs.
    2. The frontend inserts `:worldinc` after all struct definitions, method
    definitions, `using` and `import.
    3. The `@eval` macro inserts a worldinc following the call to `eval` if
    at toplevel
    4. A literal (syntactic) call to `include` gains an implicit `worldinc`.
    
    Of these the fourth is probably the most questionable, but is necessary
    to make this non-breaking for most code patterns. Perhaps it would have
    been better to make `include` a macro from the beginning (esp because it
    already has semantics that look a little like reaching into the calling
    module), but that ship has sailed.
    
    Unfortunately, I don't see any good intermediate options between this PR
    and option #3 above. I think option #3 is closest to what we have right
    now, but if we were to choose it and actually fix the soundness issues,
    I expect that we would be destroying all performance of global-scope
    code. For this reason, I would like to try to make the version in this
    PR work, even if the semantics are a little ugly.
    
    The biggest pattern that this PR does not catch is:
    ```
    eval(:(f() = 1))
    f()
    ```
    
    We could apply the same `include` special case to eval, but given the
    existence of `@eval` which allows addressing this at the macro level, I
    decided not to. We can decide which way we want to go on this based on
    what the package ecosystem looks like.

 Compiler/src/abstractinterpretation.jl | 226 ++++++++++++++-------------
 Compiler/src/inferencestate.jl         |   9 +-
 Compiler/src/ssair/irinterp.jl         |  12 +-
 Compiler/src/tfuncs.jl                 |   2 +-
 Compiler/src/types.jl                  |   1 +
 Compiler/src/validation.jl             |   1 +
 base/boot.jl                           |   3 +-
 base/essentials.jl                     |  12 +-
 base/sysimg.jl                         |   7 +
 base/tuple.jl                          |   2 +-
 src/codegen.cpp                        |  31 ++--
 src/interpreter.c                      |   2 -
 src/jlfrontend.scm                     |   2 +-
 src/julia-syntax.scm                   |  31 +++-
 src/toplevel.c                         |  20 ++-
 stdlib/Logging/test/runtests.jl        |   2 +-
 stdlib/REPL/src/REPL.jl                |   4 +-
 stdlib/REPL/src/REPLCompletions.jl     |   4 +-
 stdlib/REPL/test/replcompletions.jl    | 273 +++++++++++++++++----------------
 stdlib/Serialization/test/runtests.jl  |   4 +-
 stdlib/Test/src/Test.jl                |  24 +++
 test/arrayops.jl                       |  51 +++---
 test/core.jl                           |  22 ++-
 test/deprecation_exec.jl               |   1 +
 test/error.jl                          |   2 +-
 test/math.jl                           |  78 +++++-----
 test/ranges.jl                         |   6 +-
 test/sorting.jl                        |   7 +-
 test/syntax.jl                         |  25 ++-
 29 files changed, 508 insertions(+), 356 deletions(-)

@d-netto
Copy link
Member

d-netto commented Jan 22, 2025

Is #56509 a reasonable candidate for breaking this? (Not familiar with the PR).

@d-netto
Copy link
Member

d-netto commented Jan 22, 2025

CC: @Keno.

@vtjnash
Copy link
Member

vtjnash commented Jan 22, 2025

Yes, this is an explicit feature of that PR (insert_toplevel_latestworld):

diff --git a/stdlib/Test/src/Test.jl b/stdlib/Test/src/Test.jl
index 1b9505c59e..7c985828d4 100644
--- a/stdlib/Test/src/Test.jl
+++ b/stdlib/Test/src/Test.jl
@@ -1563,6 +1563,13 @@ parent test set (with the context object appended to any failing tests.)
 !!! compat "Julia 1.10"
     Multiple `let` assignments are supported since Julia 1.10.
 
+# Special implicit world age increment for `@testset begin`
+
+World age inside `@testset begin` increments implicitly after every statement.
+This matches the behavior of ordinary toplevel code, but not that of ordinary
+`begin/end` blocks, i.e. with respect to world age, `@testset begin` behaves
+as if the body of the `begin/end` block was written at toplevel.
+

@vtjnash
Copy link
Member

vtjnash commented Jan 22, 2025

In particular: #56509 (comment)

Maybe we add additional logic to grep for :meta :force_compile and delete any latestworld-if-toplevel nodes?

@Keno
Copy link
Member

Keno commented Jan 22, 2025

I think the answer depends on what that test is really trying to do. We don't really have semantic guarantees around @allocated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression in behavior compared to a previous version regression 1.12 Regression in the 1.12 release testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

No branches or pull requests

8 participants