[release-0.6] help #24383 inference and load time regression #24399

JeffBezanson · 2017-10-29T21:31:20Z

If this works, a similar version is applicable to master.

This change has two parts. First, I modified the union-splitting logic in method lookup to only infer split signatures that are concrete. This prunes a bunch of the inference tree, reducing BandedMatrices.ji from ~100MB to ~40MB.

Next there is a load time problem where jl_recache_types is O(n^2). (A lot of time is also spent inserting methods and backedges, but if the jl_recache_types change works it's an easy fix.) @vtjnash will have to review to see if this is valid. If it's not, it can be dropped from the patch and there's still a significant improvement.

With this, using BandedMatrices (including precompilation) takes ~3 minutes for me instead of ~20.

ref #24383

vtjnash · 2017-10-30T00:53:14Z

will have to review to see if this is valid

Yeah, that looks fine. In my analysis of all packages on my system, this list had ~10-1000 items originally (edit: usually around 10 originally, and now usually in the low hundreds), so I didn't worry about optimizing it (and the assertion there was pretty helpful in shaking out latent problems that it used to have).

The change to the union-splitting logic seems less advisable though (it's covering for subtyping returning incorrect type variable bounds, leading to incorrect eltype deductions), but also valid.

JeffBezanson · 2017-10-30T03:49:00Z

I'll work on that. We should really make sure the inferred static parameter values are sound no matter what.

JeffBezanson · 2017-10-30T17:58:09Z

Ok, I think this might be good now. I would say a PkgEval run is warranted however.

vtjnash · 2017-10-30T18:00:19Z

src/subtype.c

-            if (!oldval || !jl_is_typevar(oldval) || !jl_is_long(val))
+            if (oldval && !jl_egal(oldval, val))
+                e->envout[e->envidx] = (jl_value_t*)u->var;
+            else


this commit should probably be put on master first

vtjnash · 2017-10-30T18:04:35Z

test/inference.jl

+let T1 = Array{Float64}, T2 = Array{_1,2} where _1
+    inference_test_copy(a::T) where {T<:Array} = ccall(:jl_array_copy, Ref{T}, (Any,), a)
+    rt = Base.return_types(inference_test_copy, (Union{T1,T2},))[1]
+    @test rt >: T1 && rt >: T2


This should be testing for equality to something. I know you only want to test for strongly valid properties, but as written, I can't verify that this test is testing for the reported issue. And it could also break again later without noticing that this test was later rendered ineffectual.

The brokenness before was that rt >: T1 && rt >: T2 did not hold. So if it broke again the test would fail.

from #24399

…g of large precompiled packages. extracted from #24399

…g of large precompiled packages. (#24407) extracted from #24399

ViralBShah · 2017-10-31T19:53:37Z

Would be great if @ChrisRackauckas can try this out.

ChrisRackauckas · 2017-11-01T20:22:15Z

I don't have the setup to build everything right now, but my test would be to just check using BandedMatrices like @JeffBezanson did, so since that worked, 👍 .

from #24399

…g of large precompiled packages. (#24407) extracted from #24399 (cherry picked from commit b8d42d4)

ararslan · 2017-11-11T22:47:49Z

Bump @JeffBezanson. It would be great if we could get this merged so that it can go into 0.6.2.

reduce O(n^2) behavior of `jl_recache_types` improve soundness of inferred typevar bounds in subtyping these help #24383

JeffBezanson · 2017-11-14T18:04:23Z

Rebased. Merge when ready if the test failures are expected.

ararslan · 2017-11-14T19:43:18Z

Yep, they are. The release-0.6 branch doesn't have the CircleCI configuration file, any of the commits that got Julia building on FreeBSD, or the fixes for Travis that allow IPv6. (The Travis change is being backported.) Thanks Jeff!

ararslan · 2017-11-20T22:06:15Z

This appears to have broken JuliaDB (based on a git bisect). Should it be reverted from the release-0.6 branch, or would you rather submit some kind of fix for the case it's hitting?

JeffBezanson · 2017-11-20T22:21:13Z

How can I get more info on the failure?

…erence (#24399)" This reverts commit cf2ed66. It was determined to cause package regressions.

ararslan · 2017-11-20T22:27:23Z

Build the release-0.6 branch and run Pkg.test("JuliaDB"). The log from PkgEval is also available here.

…erence (#24399)" This reverts commit cf2ed66. It was determined to cause package regressions.

JeffBezanson added compiler:inference Type inference performance Must go faster compiler:precompilation Precompilation of modules labels Oct 29, 2017

ararslan requested a review from vtjnash October 29, 2017 21:42

JeffBezanson force-pushed the jb/0.6-load-time branch from 880ca90 to 2b4cb95 Compare October 29, 2017 22:54

JeffBezanson force-pushed the jb/0.6-load-time branch 2 times, most recently from c3f5f5f to 931b322 Compare October 30, 2017 17:46

vtjnash reviewed Oct 30, 2017

View reviewed changes

JeffBezanson added a commit that referenced this pull request Oct 30, 2017

fix soundness of inferred typevar bounds

5d2cf0a

from #24399

JeffBezanson mentioned this pull request Oct 30, 2017

fix soundness of inferred typevar bounds #24406

Merged

JeffBezanson added a commit that referenced this pull request Oct 30, 2017

short-circuit some iterations in jl_recache_type to speed up loadin…

9794886

…g of large precompiled packages. extracted from #24399

JeffBezanson mentioned this pull request Oct 30, 2017

short-circuit some iterations in jl_recache_type to speed up loading of large precompiled packages #24407

Merged

JeffBezanson added a commit that referenced this pull request Oct 31, 2017

short-circuit some iterations in jl_recache_type to speed up loadin…

b8d42d4

…g of large precompiled packages. (#24407) extracted from #24399

ararslan pushed a commit that referenced this pull request Nov 4, 2017

fix soundness of inferred typevar bounds

31db82a

from #24399

JeffBezanson added a commit that referenced this pull request Nov 7, 2017

fix soundness of inferred typevar bounds (#24406)

36f40d5

from #24399

ararslan pushed a commit that referenced this pull request Nov 7, 2017

short-circuit some iterations in jl_recache_type to speed up loadin…

f22390a

…g of large precompiled packages. (#24407) extracted from #24399 (cherry picked from commit b8d42d4)

ararslan pushed a commit that referenced this pull request Nov 8, 2017

short-circuit some iterations in jl_recache_type to speed up loadin…

8a709b8

…g of large precompiled packages. (#24407) extracted from #24399 (cherry picked from commit b8d42d4)

ararslan pushed a commit that referenced this pull request Nov 8, 2017

short-circuit some iterations in jl_recache_type to speed up loadin…

a05e10f

…g of large precompiled packages. (#24407) extracted from #24399 (cherry picked from commit b8d42d4)

ararslan pushed a commit that referenced this pull request Nov 8, 2017

short-circuit some iterations in jl_recache_type to speed up loadin…

a07a950

…g of large precompiled packages. (#24407) extracted from #24399 (cherry picked from commit b8d42d4)

ararslan pushed a commit that referenced this pull request Nov 8, 2017

short-circuit some iterations in jl_recache_type to speed up loadin…

cc4c67c

…g of large precompiled packages. (#24407) extracted from #24399 (cherry picked from commit b8d42d4)

ararslan pushed a commit that referenced this pull request Nov 9, 2017

short-circuit some iterations in jl_recache_type to speed up loadin…

affca5d

…g of large precompiled packages. (#24407) extracted from #24399 (cherry picked from commit b8d42d4)

ararslan mentioned this pull request Nov 13, 2017

WIP: Backports for 0.6.2 #24519

Merged

only infer concrete union-split signatures to cut down on inference

9e84757

reduce O(n^2) behavior of `jl_recache_types` improve soundness of inferred typevar bounds in subtyping these help #24383

JeffBezanson force-pushed the jb/0.6-load-time branch from 931b322 to 9e84757 Compare November 14, 2017 03:35

ararslan merged commit cf2ed66 into release-0.6 Nov 14, 2017

ararslan deleted the jb/0.6-load-time branch November 14, 2017 19:43

ararslan added a commit that referenced this pull request Nov 20, 2017

Revert "only infer concrete union-split signatures to cut down on inf…

c2dfe07

…erence (#24399)" This reverts commit cf2ed66. It was determined to cause package regressions.

ararslan added a commit that referenced this pull request Nov 21, 2017

Revert "only infer concrete union-split signatures to cut down on inf…

c9cd406

…erence (#24399)" This reverts commit cf2ed66. It was determined to cause package regressions.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release-0.6] help #24383 inference and load time regression #24399

[release-0.6] help #24383 inference and load time regression #24399

JeffBezanson commented Oct 29, 2017

vtjnash commented Oct 30, 2017 •

edited

Loading

JeffBezanson commented Oct 30, 2017

JeffBezanson commented Oct 30, 2017

vtjnash Oct 30, 2017

vtjnash Oct 30, 2017

JeffBezanson Oct 30, 2017

ViralBShah commented Oct 31, 2017

ChrisRackauckas commented Nov 1, 2017

ararslan commented Nov 11, 2017

JeffBezanson commented Nov 14, 2017

ararslan commented Nov 14, 2017

ararslan commented Nov 20, 2017

JeffBezanson commented Nov 20, 2017

ararslan commented Nov 20, 2017

[release-0.6] help #24383 inference and load time regression #24399

[release-0.6] help #24383 inference and load time regression #24399

Conversation

JeffBezanson commented Oct 29, 2017

vtjnash commented Oct 30, 2017 • edited Loading

JeffBezanson commented Oct 30, 2017

JeffBezanson commented Oct 30, 2017

vtjnash Oct 30, 2017

Choose a reason for hiding this comment

vtjnash Oct 30, 2017

Choose a reason for hiding this comment

JeffBezanson Oct 30, 2017

Choose a reason for hiding this comment

ViralBShah commented Oct 31, 2017

ChrisRackauckas commented Nov 1, 2017

ararslan commented Nov 11, 2017

JeffBezanson commented Nov 14, 2017

ararslan commented Nov 14, 2017

ararslan commented Nov 20, 2017

JeffBezanson commented Nov 20, 2017

ararslan commented Nov 20, 2017

vtjnash commented Oct 30, 2017 •

edited

Loading