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

inference: (slightly) improve type stability of capturing closures #56532

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

Conversation

aviatesk
Copy link
Member

As an idea to improve type stability for capturing closures, such as in #31909, I tried this idea of propagating the closure object as a PartialStruct whose fields include captured variables of which types are (partially) known. By performing const-prop on this closure::PartialStruct, we can achieve a certain level of type stability.
Specifically, I made some modifications to abstract_eval_new to allow creating PartialStruct even for :new objects that are !isconcretedispatch (since PartialStruct can now represent abstract elements). I also adjusted const_prop_argument_heuristic to perform aggressive constant propagation using such closure::PartialStruct.

As a result, the following code now achieves type stability:

julia> Base.infer_return_type((Bool,Int,)) do b, y
           x = b ? 1 : missing
           inner = y -> x + y
           return inner(y)
       end
Any                   # master
Union{Missing, Int64} # this commit

However, this alone was not enough to fully resolve #31909. The call graph of map is extremely complex, and simply applying constant propagation everywhere does not achieve the type safety requested in the issue.

Nevertheless this commit alone would still improve type stability for some cases, so I will go ahead and submit it as a PR.


@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk aviatesk force-pushed the avi/abstract-partial-struct branch 2 times, most recently from 72be4a1 to 56de49c Compare November 13, 2024 06:45
@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

aviatesk added a commit that referenced this pull request Nov 15, 2024
- removed unnecessary `Core.Box` allocation
- made the type of the closure that is passed to `Future` concrete

That said, it doesn’t seem ideal to require this sort of manual
optimizations.. The value of using closures cannot be denied in this
code base, and I feel that it would be better to work towards optimizing
closures more (as we do with #56532)?
aviatesk added a commit that referenced this pull request Nov 20, 2024
This is an alternative to #56532 and can resolve #31909.
Currently `apply_type_tfunc` is unable to handle `Union`-argtypes with
any precision. With this change, `apply_type_tfunc` now performs
union-splitting on `Union`-argtypes and returns the merged result of
the splits.
While this can improve inference precision, we might need to be cautious
about potential inference time bloat.
aviatesk added a commit that referenced this pull request Nov 20, 2024
This is an alternative to #56532 and can resolve #31909.
Currently `apply_type_tfunc` is unable to handle `Union`-argtypes with
any precision. With this change, `apply_type_tfunc` now performs
union-splitting on `Union`-argtypes and returns the merged result of
the splits.
While this can improve inference precision, we might need to be cautious
about potential inference time bloat.
aviatesk added a commit that referenced this pull request Nov 21, 2024
This is an alternative to #56532 and can resolve #31909.
Currently `apply_type_tfunc` is unable to handle `Union`-argtypes with
any precision. With this change, `apply_type_tfunc` now performs
union-splitting on `Union`-argtypes and returns the merged result of
the splits.
While this can improve inference precision, we might need to be cautious
about potential inference time bloat.
aviatesk added a commit that referenced this pull request Nov 21, 2024
This is an alternative to #56532 and can resolve #31909.
Currently `apply_type_tfunc` is unable to handle `Union`-argtypes with
any precision. With this change, `apply_type_tfunc` now performs
union-splitting on `Union`-argtypes and returns the merged result of
the splits.
While this can improve inference precision, we might need to be cautious
about potential inference time bloat.
aviatesk added a commit that referenced this pull request Nov 21, 2024
This is an alternative to #56532 and can resolve #31909.
Currently `apply_type_tfunc` is unable to handle `Union`-argtypes with
any precision. With this change, `apply_type_tfunc` now performs
union-splitting on `Union`-argtypes and returns the merged result of
the splits.
While this can improve inference precision, we might need to be cautious
about potential inference time bloat.
aviatesk added a commit that referenced this pull request Nov 22, 2024
This is an alternative to #56532 and can resolve #31909.
Currently `apply_type_tfunc` is unable to handle `Union`-argtypes with
any precision. With this change, `apply_type_tfunc` now performs
union-splitting on `Union`-argtypes and returns the merged result of
the splits.
While this can improve inference precision, we might need to be cautious
about potential inference time bloat.
aviatesk added a commit that referenced this pull request Nov 22, 2024
This is an alternative to #56532 and can resolve #31909.
Currently `apply_type_tfunc` is unable to handle `Union`-argtypes with
any precision. With this change, `apply_type_tfunc` now performs
union-splitting on `Union`-argtypes and returns the merged result of the
splits.
While this can improve inference precision, we might need to be cautious
about potential inference time bloat.

---------

Co-authored-by: Jameson Nash <[email protected]>
As an idea to improve type stability for capturing closures, such as in
#31909, I tried this idea of propagating the closure
object as a `PartialStruct` whose `fields` include captured variables
of which types are (partially) known. By performing const-prop on this
`closure::PartialStruct`, we can achieve a certain level of type
stability.
Specifically, I made some modifications to `abstract_eval_new` to allow
creating `PartialStruct` even for `:new` objects that are
`!isconcretedispatch` (since `PartialStruct` can now represent abstract
elements). I also adjusted `const_prop_argument_heuristic` to perform
aggressive constant propagation using such `closure::PartialStruct`.

As a result, the following code now achieves type stability:
```julia
julia> Base.infer_return_type((Bool,Int,)) do b, y
           x = b ? 1 : missing
           inner = y -> x + y
           return inner(y)
       end
Any                   # master
Union{Missing, Int64} # this commit
```

However, this alone was not enough to fully resolve #31909.
The call graph of `map` is extremely complex, and simply applying
constant propagation everywhere does not achieve the type safety
requested in the issue.

Nevertheless this commit alone would still improve type stability for
some cases, so I will go ahead and submit it as a PR.
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.

Suboptimal performance of captured small unions
2 participants