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 base #22019

Merged
merged 12 commits into from
May 24, 2017
Merged

improve inferability of base #22019

merged 12 commits into from
May 24, 2017

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented May 22, 2017

Avoid growing an input parameter during recursion. In general, permitting that would require that inference be potentially non-terminating (as proving that it will converge requires solving the halting problem). However, the methods in base using this pattern only actually requires the simpler pattern of tail-recursing on some element and concatenating the output.

(This is the learnings of #21933 without the policy change.)

@StefanKarpinski
Copy link
Member

Is this really safe to backport to 0.6? It seems awfully large and potentially dangerous for that. It would have to be fixing some really bad issue in order to warrant that backport.

@vtjnash
Copy link
Member Author

vtjnash commented May 22, 2017

Its somewhat large, but mostly mechanical. I probably wouldn't bother if we already had a release, but I think it's useful to keep master closer during the RC period. There's a few bug fix commits in here too (1, 3, and 7) that should be backported regardless.

@vtjnash
Copy link
Member Author

vtjnash commented May 22, 2017

sysimg also seems to be about a megabyte smaller too.

@StefanKarpinski
Copy link
Member

As always, can we have just the bug fixes in their own PRs please? Those can and should be merged as soon as they pass CI. The rest is less clear.

@vtjnash
Copy link
Member Author

vtjnash commented May 22, 2017

This is the bugfix PR (feature PR is #21933). It's just not quite as essential that we backport the correctness fixes as the assertion fixes.

@tkelman
Copy link
Contributor

tkelman commented May 22, 2017

What bugs are being fixed? Where are the tests?

_cshp(tail(dims), (out..., next), tail(shape), tail(nshape))
@inline function _cshp(ndim::Int, dims, shape, nshape)
a = shape[1]
b = nshape[1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces, not tabs

@vtjnash
Copy link
Member Author

vtjnash commented May 22, 2017

The fixes here are to ensure that inference will always eventually terminate when any of these methods enter its queue. Tests are PR #21933 which addresses that bug more broadly.

vtjnash added 12 commits May 22, 2017 17:23
would segfault when reaching the jl_datatype_nfields call
TODO: this is helping to avoid a type-system bug mis-computing sparams during intersection,
but that can already cause significant problems elsewhere too
…to be non-terminating

this same issue applied to several other similar functions

similarly, need to avoid nesting the use of functional code (like map)
too deeply inside the array code to avoid the appearance of indeterminate recursion in inference
@vtjnash vtjnash force-pushed the jn/inferrable-functions branch from 736fa71 to 4287761 Compare May 22, 2017 22:24
@mbauman
Copy link
Member

mbauman commented May 22, 2017

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

@nanosoldier
Copy link
Collaborator

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

@vtjnash vtjnash merged commit 62e8227 into master May 24, 2017
@vtjnash vtjnash deleted the jn/inferrable-functions branch May 24, 2017 18:14
@tkelman
Copy link
Contributor

tkelman commented May 24, 2017

Tests are PR #21933 which addresses that bug more broadly.

There is exactly one line of tests changed in https://github.com/JuliaLang/julia/pull/21933/files. I'm against backporting this without any tests.

@nalimilan
Copy link
Member

This broke broadcast in DataArrays, see JuliaStats/DataArrays.jl#259 (comment) and #22130.

tkelman pushed a commit that referenced this pull request Jun 17, 2017
would segfault when reaching the jl_datatype_nfields call

(cherry picked from commit 8cc5e2b)
small piece of #22019
@MikeInnes
Copy link
Member

cf0c6a1 fixes an issue for me and is trivial to backport, would like to see that in a point release.

@tkelman
Copy link
Contributor

tkelman commented Aug 21, 2017

can you submit a unit test that it fixes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants