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

Track backedges through not-yet-defined bindings? #53958

Closed
Tortar opened this issue Apr 4, 2024 · 4 comments
Closed

Track backedges through not-yet-defined bindings? #53958

Tortar opened this issue Apr 4, 2024 · 4 comments

Comments

@Tortar
Copy link
Contributor

Tortar commented Apr 4, 2024

I was reading the thread at https://discourse.julialang.org/t/julia-slower-than-python-for-finding-pangrams/112481 and I found a very strange behaviour, consider this code to be run on the REPL:

input = "wjjqevffkkgbcehhiqpvqutmwxawzvjnbvukmlzxyhkgfddzfjhcujnlkjbdfgghjhujkiuytghjioplkjhgfdsaqwertyujioplkjhgfdsaqwertzuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkj"

function ispangram10(input::String)
    a,A,z,Z = UInt8('a'),UInt8('A'),UInt8('z'),UInt8('Z')
    appears::UInt32 = ~((one(UInt32)<<26)-one(UInt32))
    @inbounds for b in codeunits(input)
        if A  b  Z
            appears |= one(UInt32) << shift_safe(UInt32, b-A)
        elseif a  b  z
            appears |= one(UInt32) << shift_safe(UInt32, b-a)
        end
        appears == (-1%UInt32) && return true
    end
    return false
end
           
ispangram10(input) # this will error

shift_safe(::Type{T}, s::Integer) where {T} = s & (8 * sizeof(T) - 1)

ispangram10(input)
@time ispangram10(input)

you will see that the last timing is

julia> @time ispangram10(input)
  0.000032 seconds (137 allocations: 2.141 KiB)

now if you instead run this:

input = "wjjqevffkkgbcehhiqpvqutmwxawzvjnbvukmlzxyhkgfddzfjhcujnlkjbdfgghjhujkiuytghjioplkjhgfdsaqwertyujioplkjhgfdsaqwertzuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsazxcvbnmlkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkjhgfdsaqwertyuioplkj"

function ispangram10(input::String)
    a,A,z,Z = UInt8('a'),UInt8('A'),UInt8('z'),UInt8('Z')
    appears::UInt32 = ~((one(UInt32)<<26)-one(UInt32))
    @inbounds for b in codeunits(input)
        if A  b  Z
            appears |= one(UInt32) << shift_safe(UInt32, b-A)
        elseif a  b  z
            appears |= one(UInt32) << shift_safe(UInt32, b-a)
        end
        appears == (-1%UInt32) && return true
    end
    return false
end
           
#ispangram10(input) # this is the only difference, it is commented in this version

shift_safe(::Type{T}, s::Integer) where {T} = s & (8 * sizeof(T) - 1)

ispangram10(input)
@time ispangram10(input)

it returns instead

julia> @time ispangram10(input)
  0.000002 seconds

What is it happening here? Why the error in the first version makes the function perf much worse afterwards?

julia> versioninfo()
Julia Version 1.10.2
Commit bd47eca2c8a (2024-03-01 10:14 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 12 × AMD Ryzen 5 5600H with Radeon Graphics
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, znver3)
Threads: 1 default, 0 interactive, 1 GC (on 12 virtual cores)

edit: tested also on 1.11-alpha2, same behaviour

@mbauman
Copy link
Member

mbauman commented Apr 4, 2024

If shift_safe doesn't yet exist, there's no place for ispangram10 to record its backedge, and so there's no way for it to know it needs to be invalidated. The compiler knows this limitation, so it compiles it conservatively — with a dynamic call and a type instability in it. If you have a function shift_safe end defined from the get-go then ispangram10 will be able to be properly invalidated, even if it still throws a MethodError on its first invocation.

@Tortar Tortar changed the title Performance of function totally changes if it first errors? Function first containing a non existing method is not properly invalidated when the method is added, can this be improved? Apr 4, 2024
@Tortar
Copy link
Contributor Author

Tortar commented Apr 4, 2024

thanks @mbauman for the clarification, if this can't be improved then I just learnt something :D I changed the title to properly reflect the real issue anyway

@Tortar
Copy link
Contributor Author

Tortar commented Apr 4, 2024

I'm thinking if a warning when something like this happens could be beneficial in any case

@mbauman mbauman changed the title Function first containing a non existing method is not properly invalidated when the method is added, can this be improved? Track backedges through not-yet-defined bindings? Apr 4, 2024
@vchuravy
Copy link
Member

vchuravy commented Apr 4, 2024

Essentially a duplicate of #40399 and #8870

Bindings (defined or not defined) are not tracked by the system.

@vchuravy vchuravy closed this as not planned Won't fix, can't repro, duplicate, stale Apr 4, 2024
Keno added a commit that referenced this issue Jun 2, 2024
This implements world-age partitioning of bindings as proposed in #40399.
In effect, much like methods, the global view of bindings now depends on
your currently executing world. This means that `const` bindings can now
have different values in different worlds. In principle it also means
that regular global variables could have different values in different
worlds, but there is currently no case where the system does this.

# Motivation
The reasons for this change are manifold:

1. The primary motivation is to permit Revise to redefine structs.
   This has been a feature request since the very begining of Revise
   (timholy/Revise.jl#18) and there have been
   numerous attempts over the past 7 years to address this, as well as
   countless duplicate feature request. A past attempt to implement the
   necessary julia support in #22721 failed because the consequences and
   semantics of re-defining bindings were not sufficiently worked out.
   One way to think of this implementation (at least with respect to types)
   is that it provides a well-grounded implementation of #22721.

2. A secondary motivation is to make `const`-redefinition no longer UB
   (although `const` redefinition will still have a significant performance
   penalty, so it is not recommended). See e.g. the full discussion in #54099.

3. Not currently implemented, but this mechanism can be used to re-compile code
   where bindings are introduced after the first compile, which is a common
   performance trap for new users (#53958).

4. Not currently implemented, but this mechanism can be used to clarify the semantics
   of bindings import and resolution to address issues like #14055.

# Implementation

In this PR:
 - `Binding` gets `min_world`/`max_world` fields like `CodeInstance`
 - Various lookup functions walk this linked list using the current task world_age as a key
 - Inference accumulates world bounds as it would for methods
 - Upon binding replacement, we walk all methods in the system, invalidating those whose
   uninferred IR references the replaced GlobalRef
 - One primary complication is that our IR definition permits `const` globals in value position,
   but if binding replacement is permitted, the validity of this may change after the fact.
   To address this, there is a helper in `Core.Compiler` that gets invoked in the type inference
   world and will rewrite the method source to be legal in all worlds.
 - A new `@world` macro can be used to access bindings from old world ages. This is used in printing
   for old objects.
 - The `const`-override behavior was changed to only be permitted at toplevel. The warnings about
   it being UB was removed.

Of particular note, this PR does not include any mechanism for invalidating methods whose signatures
were created using an old Binding (or types whose fields were the result of a binding evaluation).
There was some discussion among the compiler team of whether such a mechanism should exist in base,
but the consensus was that it should not. In particular, although uncommon, a pattern like:
```
f() = Any
g(::f()) = 1
f() = Int
```
Does not redefine `g`. Thus to fully address the Revise issue, additional code will be required in
Revise to track the dependency of various signatures and struct definitions on bindings.

# Demo
```
julia> struct Foo
               a::Int
       end

julia> g() = Foo(1)
g (generic function with 1 method)

julia> g()
Foo(1)

julia> f(::Foo) = 1
f (generic function with 1 method)

julia> fold = Foo(1)
Foo(1)

julia> struct Foo
               a::Int
               b::Int
       end

julia> g()
ERROR: MethodError: no method matching Foo(::Int64)
The type `Foo` exists, but no method is defined for this combination of argument types when trying to construct it.

Closest candidates are:
  Foo(::Int64, ::Int64)
   @ Main REPL[6]:2
  Foo(::Any, ::Any)
   @ Main REPL[6]:2

Stacktrace:
 [1] g()
   @ Main ./REPL[2]:1
 [2] top-level scope
   @ REPL[7]:1

julia> f(::Foo) = 2
f (generic function with 2 methods)

julia> methods(f)
# 2 methods for generic function "f" from Main:
 [1] f(::Foo)
     @ REPL[8]:1
 [2] f(::@world(Foo, 0:26898))
     @ REPL[4]:1

julia> fold
@world(Foo, 0:26898)(1)
```

# Performance consideration
On my machine, the validation required upon binding replacement for the full system image takes about 200ms.
With CedarSim loaded (I tried OmniPackage, but it's not working on master), this increases about 5x. That's
a fair bit of compute, but not the end of the world. Still, Revise may have to batch its validation. There
may also be opportunities for performance improvement by operating on the compressed representation directly.

# Semantic TODO

- [ ] Do we want to change the resolution time of bindings to (semantically) resolve them immediately?
- [ ] Do we want to introduce guard bindings when inference assumes the absence of a binding?

# Implementation TODO
- [ ] Precompile re-validation
- [ ] Various cleanups in the accessors
- [ ] Invert the order of the binding linked list to make the most recent one always the head of the list
- [ ] CodeInstances need forward edges for GlobalRefs not part of the uninferred code
- [ ] Generated function support
Keno added a commit that referenced this issue Jun 9, 2024
This implements world-age partitioning of bindings as proposed in #40399.
In effect, much like methods, the global view of bindings now depends on
your currently executing world. This means that `const` bindings can now
have different values in different worlds. In principle it also means
that regular global variables could have different values in different
worlds, but there is currently no case where the system does this.

The reasons for this change are manifold:

1. The primary motivation is to permit Revise to redefine structs.
   This has been a feature request since the very begining of Revise
   (timholy/Revise.jl#18) and there have been
   numerous attempts over the past 7 years to address this, as well as
   countless duplicate feature request. A past attempt to implement the
   necessary julia support in #22721 failed because the consequences and
   semantics of re-defining bindings were not sufficiently worked out.
   One way to think of this implementation (at least with respect to types)
   is that it provides a well-grounded implementation of #22721.

2. A secondary motivation is to make `const`-redefinition no longer UB
   (although `const` redefinition will still have a significant performance
   penalty, so it is not recommended). See e.g. the full discussion in #54099.

3. Not currently implemented, but this mechanism can be used to re-compile code
   where bindings are introduced after the first compile, which is a common
   performance trap for new users (#53958).

4. Not currently implemented, but this mechanism can be used to clarify the semantics
   of bindings import and resolution to address issues like #14055.

In this PR:
 - `Binding` gets `min_world`/`max_world` fields like `CodeInstance`
 - Various lookup functions walk this linked list using the current task world_age as a key
 - Inference accumulates world bounds as it would for methods
 - Upon binding replacement, we walk all methods in the system, invalidating those whose
   uninferred IR references the replaced GlobalRef
 - One primary complication is that our IR definition permits `const` globals in value position,
   but if binding replacement is permitted, the validity of this may change after the fact.
   To address this, there is a helper in `Core.Compiler` that gets invoked in the type inference
   world and will rewrite the method source to be legal in all worlds.
 - A new `@world` macro can be used to access bindings from old world ages. This is used in printing
   for old objects.
 - The `const`-override behavior was changed to only be permitted at toplevel. The warnings about
   it being UB was removed.

Of particular note, this PR does not include any mechanism for invalidating methods whose signatures
were created using an old Binding (or types whose fields were the result of a binding evaluation).
There was some discussion among the compiler team of whether such a mechanism should exist in base,
but the consensus was that it should not. In particular, although uncommon, a pattern like:
```
f() = Any
g(::f()) = 1
f() = Int
```
Does not redefine `g`. Thus to fully address the Revise issue, additional code will be required in
Revise to track the dependency of various signatures and struct definitions on bindings.

```
julia> struct Foo
               a::Int
       end

julia> g() = Foo(1)
g (generic function with 1 method)

julia> g()
Foo(1)

julia> f(::Foo) = 1
f (generic function with 1 method)

julia> fold = Foo(1)
Foo(1)

julia> struct Foo
               a::Int
               b::Int
       end

julia> g()
ERROR: MethodError: no method matching Foo(::Int64)
The type `Foo` exists, but no method is defined for this combination of argument types when trying to construct it.

Closest candidates are:
  Foo(::Int64, ::Int64)
   @ Main REPL[6]:2
  Foo(::Any, ::Any)
   @ Main REPL[6]:2

Stacktrace:
 [1] g()
   @ Main ./REPL[2]:1
 [2] top-level scope
   @ REPL[7]:1

julia> f(::Foo) = 2
f (generic function with 2 methods)

julia> methods(f)
 [1] f(::Foo)
     @ REPL[8]:1
 [2] f(::@world(Foo, 0:26898))
     @ REPL[4]:1

julia> fold
@world(Foo, 0:26898)(1)
```

On my machine, the validation required upon binding replacement for the full system image takes about 200ms.
With CedarSim loaded (I tried OmniPackage, but it's not working on master), this increases about 5x. That's
a fair bit of compute, but not the end of the world. Still, Revise may have to batch its validation. There
may also be opportunities for performance improvement by operating on the compressed representation directly.

- [ ] Do we want to change the resolution time of bindings to (semantically) resolve them immediately?
- [ ] Do we want to introduce guard bindings when inference assumes the absence of a binding?

- [ ] Precompile re-validation
- [ ] Various cleanups in the accessors
- [ ] Invert the order of the binding linked list to make the most recent one always the head of the list
- [ ] CodeInstances need forward edges for GlobalRefs not part of the uninferred code
- [ ] Generated function support
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

No branches or pull requests

3 participants