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

Improve inferability of shape::Dims for cat #39294

Merged
merged 2 commits into from
Jan 19, 2021
Merged

Improve inferability of shape::Dims for cat #39294

merged 2 commits into from
Jan 19, 2021

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Jan 17, 2021

cat is often called with Varargs or heterogenous inputs,
and in such cases inference almost always fails. Even when all the arrays
are of the same type, if the number of varargs isn't known
inference typically fails. The culprit is probably #36454.

This reduces the number of failures considerably, by avoiding
creation of vararg length tuples in the shape-inference pipeline.

`cat` is often called with Varargs or heterogenous inputs,
and inference almost always fails. Even when all the arrays
are of the same type, if the number of varargs isn't known
inference typically fails. The culprit is probably #36454.

This reduces the number of failures considerably, by avoiding
creation of vararg length tuples in the shape-inference pipeline.
base/abstractarray.jl Outdated Show resolved Hide resolved
@jebej
Copy link
Contributor

jebej commented Jan 18, 2021

Would this help with #21673?

@timholy
Copy link
Sponsor Member Author

timholy commented Jan 18, 2021

Seems likely that at least one of this and #39292 should help, but I haven't tested.

@timholy
Copy link
Sponsor Member Author

timholy commented Jan 19, 2021

For me, on the benchmark in #21673 master yields

julia> @btime test1(20)
  2.448 μs (34 allocations: 1.56 KiB)

whereas this branch yields

julia> @btime test1(20)
  1.052 μs (15 allocations: 864 bytes)

and the branch in #39292 yields

julia> @btime test1(20)
  1.746 μs (26 allocations: 1.14 KiB)

So they both help.

base/abstractarray.jl Outdated Show resolved Hide resolved
timholy added a commit that referenced this pull request Jan 19, 2021
The `cat` pipeline has long had poor inferrability.
Together with #39292 and #39294, this should basically put an
end to that problem.

Together, at least in simple cases these make the performance
of `cat` essentially equivalent to the manual version.
In other words, the `test1` and `test2` of #21673 benchmark
very similarly.
@timholy
Copy link
Sponsor Member Author

timholy commented Jan 19, 2021

The final fix is in #39314.

@timholy timholy merged commit 815076b into master Jan 19, 2021
@timholy timholy deleted the teh/cat_shape branch January 19, 2021 19:19
@timholy timholy added the backport 1.6 Change should be backported to release-1.6 label Jan 19, 2021
timholy added a commit that referenced this pull request Jan 20, 2021
The `cat` pipeline has long had poor inferrability.
Together with #39292 and #39294, this should basically put an
end to that problem.

Together, at least in simple cases these make the performance
of `cat` essentially equivalent to the manual version.
In other words, the `test1` and `test2` of #21673 benchmark
very similarly.
KristofferC pushed a commit that referenced this pull request Jan 20, 2021
`cat` is often called with Varargs or heterogenous inputs,
and inference almost always fails. Even when all the arrays
are of the same type, if the number of varargs isn't known
inference typically fails. The culprit is probably #36454.

This reduces the number of failures considerably, by avoiding
creation of vararg length tuples in the shape-inference pipeline.

(cherry picked from commit 815076b)
KristofferC pushed a commit that referenced this pull request Jan 20, 2021
The `cat` pipeline has long had poor inferrability.
Together with #39292 and #39294, this should basically put an
end to that problem.

Together, at least in simple cases these make the performance
of `cat` essentially equivalent to the manual version.
In other words, the `test1` and `test2` of #21673 benchmark
very similarly.

(cherry picked from commit 78d55e2)
@KristofferC KristofferC mentioned this pull request Jan 20, 2021
60 tasks
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Feb 1, 2021
KristofferC pushed a commit that referenced this pull request Feb 1, 2021
`cat` is often called with Varargs or heterogenous inputs,
and inference almost always fails. Even when all the arrays
are of the same type, if the number of varargs isn't known
inference typically fails. The culprit is probably #36454.

This reduces the number of failures considerably, by avoiding
creation of vararg length tuples in the shape-inference pipeline.

(cherry picked from commit 815076b)
KristofferC pushed a commit that referenced this pull request Feb 1, 2021
The `cat` pipeline has long had poor inferrability.
Together with #39292 and #39294, this should basically put an
end to that problem.

Together, at least in simple cases these make the performance
of `cat` essentially equivalent to the manual version.
In other words, the `test1` and `test2` of #21673 benchmark
very similarly.

(cherry picked from commit 78d55e2)
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
`cat` is often called with Varargs or heterogenous inputs,
and inference almost always fails. Even when all the arrays
are of the same type, if the number of varargs isn't known
inference typically fails. The culprit is probably JuliaLang#36454.

This reduces the number of failures considerably, by avoiding
creation of vararg length tuples in the shape-inference pipeline.
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
The `cat` pipeline has long had poor inferrability.
Together with JuliaLang#39292 and JuliaLang#39294, this should basically put an
end to that problem.

Together, at least in simple cases these make the performance
of `cat` essentially equivalent to the manual version.
In other words, the `test1` and `test2` of JuliaLang#21673 benchmark
very similarly.
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
`cat` is often called with Varargs or heterogenous inputs,
and inference almost always fails. Even when all the arrays
are of the same type, if the number of varargs isn't known
inference typically fails. The culprit is probably JuliaLang#36454.

This reduces the number of failures considerably, by avoiding
creation of vararg length tuples in the shape-inference pipeline.
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
The `cat` pipeline has long had poor inferrability.
Together with JuliaLang#39292 and JuliaLang#39294, this should basically put an
end to that problem.

Together, at least in simple cases these make the performance
of `cat` essentially equivalent to the manual version.
In other words, the `test1` and `test2` of JuliaLang#21673 benchmark
very similarly.
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
`cat` is often called with Varargs or heterogenous inputs,
and inference almost always fails. Even when all the arrays
are of the same type, if the number of varargs isn't known
inference typically fails. The culprit is probably #36454.

This reduces the number of failures considerably, by avoiding
creation of vararg length tuples in the shape-inference pipeline.

(cherry picked from commit 815076b)
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
The `cat` pipeline has long had poor inferrability.
Together with #39292 and #39294, this should basically put an
end to that problem.

Together, at least in simple cases these make the performance
of `cat` essentially equivalent to the manual version.
In other words, the `test1` and `test2` of #21673 benchmark
very similarly.

(cherry picked from commit 78d55e2)
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.

4 participants