when widening tuple types in tmerge, only widen the complex parts #50929
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oscardssmith
added
performance
oscardssmith
force-pushed
the
tmerge-tuple-widen-slower
branch
from
0e0b1a7
to
972025f
Compare
vtjnash
approved these changes
Aug 15, 2023
JeffBezanson
approved these changes
Aug 15, 2023
oscardssmith
force-pushed
the
tmerge-tuple-widen-slower
branch
from
972025f
to
fb7edce
Compare
|
Unsurprisingly, getting this exactly right is fairly fiddly. That said, this version now passes the inference tests locally at least. If this passes CI, it definitely needs a runtests (and possibly a pkgeval) to make sure it doesn't cause major regressions. That said, from my initial testing, this seems promising. |
oscardssmith
added
the
needs nanosoldier run
|
@nanosoldier |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
While there are a lot of changes in that run, the only ones I could consistently measure were the string improvements |
|
yeah. I think this is actually ready to merge. |
oscardssmith
removed
the
needs nanosoldier run
vtjnash
pushed a commit
to vtjnash/julia
that referenced
this pull request
Aug 18, 2023
…liaLang#50929) This is the part of JuliaLang#50927 required to fix JuliaLang#49249. Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION, Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels bad intuitively because giving the compiler more type information led it to forget type information that it already knew about, and is especially damaging because it led to unnecessary type instability when iterating tuples with complex element types (because the iterator state should be inferrable as an `Int` even if you have no idea what the tuple type is). This is tagged for backport to 1.10 since it is a relatively unobtrusive change and it fixes the string regression in a more proper way.
vtjnash
pushed a commit
to vtjnash/julia
that referenced
this pull request
Aug 18, 2023
…liaLang#50929) This is the part of JuliaLang#50927 required to fix JuliaLang#49249. Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION, Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels bad intuitively because giving the compiler more type information led it to forget type information that it already knew about, and is especially damaging because it led to unnecessary type instability when iterating tuples with complex element types (because the iterator state should be inferrable as an `Int` even if you have no idea what the tuple type is). This is tagged for backport to 1.10 since it is a relatively unobtrusive change and it fixes the string regression in a more proper way.
IanButterworth
pushed a commit
that referenced
this pull request
Aug 19, 2023
…0929) This is the part of #50927 required to fix #49249. Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION, Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels bad intuitively because giving the compiler more type information led it to forget type information that it already knew about, and is especially damaging because it led to unnecessary type instability when iterating tuples with complex element types (because the iterator state should be inferrable as an `Int` even if you have no idea what the tuple type is). This is tagged for backport to 1.10 since it is a relatively unobtrusive change and it fixes the string regression in a more proper way. (cherry picked from commit 6e2e6d0)
58 tasks
KristofferC
added a commit
that referenced
this pull request
Oct 2, 2023
Backported PRs: - [x] #48625 <!-- add replace(io, str, patterns...) --> - [x] #48387 <!-- Improve documentation of sort-related functions --> - [x] #48363 <!-- Revise sort.md and docstrings in sort.jl --> - [x] #48977 <!-- Update SparseArrays.jl stdlib for SuiteSparse 7 --> - [x] #50719 <!-- fix `CyclePadding(::DataType)` --> - [x] #50694 <!-- inference: permit recursive type traits --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50594 <!-- Disallow non-index Integer types in isassigned --> - [x] #50802 <!-- Makes IntrusiveLinkedListSynchronized mutable to avoid allocation on poptask --> - [x] #50858 <!-- Add a `threadpool` parameter to `Channel` constructor --> - [x] #50874 <!-- Restrict COFF to a single thread when symbol count is high --> - [x] #50822 <!-- Add default method for setmodifiers! --> - [x] #50730 <!-- Fix integer overflow in `isapprox` --> - [x] #50850 <!-- Remove weird Rational dispatch and add pi functions to list --> - [x] #50809 <!-- Limit type-printing in MethodError --> - [x] #50915 <!-- Add note the `Task` about sticky bit --> - [x] #50929 <!-- when widening tuple types in tmerge, only widen the complex parts --> - [x] #50928 <!-- Bump JuliaSyntax to 0.4.6 --> - [x] #50959 <!-- Update libssh2 patches --> - [x] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [x] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [x] #50912 <!-- Separate foreign threads into a :foreign threadpool --> - [x] #51010 <!-- Add ORIGIN to SuiteSparse rpath on Linux/FreeBSD --> - [x] #50753 <!-- cat: remove unused promote_eltype methods that confuse inference --> - [x] #51027 <!-- Implement realloc accounting correctly --> - [x] #51019 <!-- fix a case of potentially use of undefined variable when handling error in distributed message processing --> - [x] #51039 <!-- Revert "Optimize findall(f, ::AbstractArray{Bool}) (#42202)" --> - [x] #51036 <!-- add missing invoke edge for nospecialize targets --> - [x] #51042 <!-- inference: fix return_type_tfunc modeling of concrete functions --> - [x] #51114 <!-- Workaround upstream FreeBSD issue #272992 --> - [x] #50892 <!-- Add `JL_DLLIMPORT` to `small_typeof` declaration --> - [x] #51154 <!-- broadcast: use recursion rather than ntuple to map over a tuple --> - [x] #51153 <!-- fix debug typo in "add missing invoke edge for nospecialize targets (#51036)" --> - [x] #51222 <!-- Check again if the tty is open inside the IO lock --> - [x] #51236 <!-- Add lock around uv_unref during init --> - [x] #51243 <!-- GMP: Gracefully handle more overflows. --> - [x] #51254 <!-- Ryu: make sure adding zeros does not overwrite trailing dot --> - [x] #51175 <!-- shorten stale_age for cachefile lock --> - [x] #51300 <!-- fix method definition error for bad vararg --> - [x] #51307 <!-- fix force-throw ctrl-C on Windows --> - [x] #51303 <!-- ensure revising structs is safe --> - [x] #51393 - [x] #51403 Need manual backport: - [x] #51009 <!-- fix #50562, regression in `in` of tuple of Symbols --> - [x] #51053 <!-- Bump Statistics stdlib --> - [x] #51013 <!-- fix #50709, type bound error due to free typevar in sparam env --> - [x] #51305 <!-- reduce test time for rounding and floatfuncs --> Contains multiple commits, manual intervention needed: - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [ ] #51092 <!-- inference: fix bad effects for recursion --> - [x] #51247 <!-- Check if malloc has succeeded before incrementing gc stats --> - [x] #51294 <!-- use LibGit2_jll for LibGit2 library --> Non-merged PRs with backport label: - [ ] #51132 <!-- Handle `AbstractQ` in concatenation --> - [x] #51029 <!-- testdefs: make sure that if a test set changes the active project, they change it back when they're done --> - [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib" check regardless of the value of `ispath(f)` --> - [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions --> - [x] #50385 <!-- Precompile pidlocks: add to NEWS and docs --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
KristofferC
removed
the
backport 1.10
nalimilan
pushed a commit
that referenced
this pull request
Nov 5, 2023
…0929) This is the part of #50927 required to fix #49249. Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION, Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels bad intuitively because giving the compiler more type information led it to forget type information that it already knew about, and is especially damaging because it led to unnecessary type instability when iterating tuples with complex element types (because the iterator state should be inferrable as an `Int` even if you have no idea what the tuple type is). This is tagged for backport to 1.10 since it is a relatively unobtrusive change and it fixes the string regression in a more proper way. (cherry picked from commit 6e2e6d0)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the part of #50927 required to fix #49249. Specifically, before this change
tmerge(Tuple{Any, Int}, Nothing)would beUnion{Nothing, Tuple{Any, Int}}buttmerge(Tuple{BIG_UNION, Int}, Nothing)would beUnion{Nothing, Tuple{Any, Any}}. This feels bad intuitively because giving the compiler more type information led it to forget type information that it already knew about, and is especially damaging because it led to unnecessary type instability when iterating tuples with complex element types (because the iterator state should be inferrable as anInteven if you have no idea what the tuple type is).This is tagged for backport to 1.10 since it is a relatively unobtrusive change and it fixes the string regression in a more proper way.