remove additional rule that parameters must match for dispatch #23117

vtjnash · 2017-08-03T16:42:47Z

matching a method signature is a simple subtyping test,
with no additional rule to ensure that all parameters have a value

and adds a Test for possibly unbound parameters,
which allows detecting methods definitions that this change affects

kmsquire · 2017-08-03T17:30:15Z

base/dict.jl

@@ -135,10 +135,10 @@ copy(d::Dict) = Dict(d)

 const AnyDict = Dict{Any,Any}

+Dict(ps::Pair...) = Dict{Any,Any}(ps) # define first, since method sort order is wrong for the following


Just curious, can you explain why the sort order is wrong when this is below?

Presumably, DataStructures.jl will need to make similar changes (e.g., at https://github.com/JuliaCollections/DataStructures.jl/blob/master/src/default_dict.jl#L88-L96)

Bump, @vtjnash.

Probably fixed now—Jeff has made a lot of changes to method sort order since then

ararslan · 2017-08-03T21:56:20Z

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

JeffBezanson · 2017-08-03T17:54:57Z

base/essentials.jl

+# I think they may be faster / more efficient for inference,
+# if we could enable them, but are they?
+# TODO: These currently can't be used (#21026) since with
+# z(::Type{Tuple{Val{T}} where T}) = T


T is not a method parameter here.

Indeed, I seem to have been tired when I tried to write the MRE. ref #23017

On master, this bug in the subtyping environment exhibits itself as a MethodError:

julia> z2(::Type{<:Tuple{Vararg{T}}}) where {T} = T julia> z2(Tuple{Val{T}} where T) Val{T} julia> z2(Tuple{Val}) ERROR: MethodError: no method matching z2(::Type{Tuple{Val}})

JeffBezanson · 2017-08-03T18:00:21Z

test/tuple.jl

+@test (NTuple{T, Int32} where T) <: Tuple{Vararg{Int32}}
+@test !((NTuple{T, Int32} where T) <: Tuple{Int32, Vararg{Int32}})
+@test Tuple{Vararg{Int32}} <: (NTuple{T, Int32} where T)
+@test Tuple{Int32, Vararg{Int32}} <: (NTuple{T, Int32} where T)


Should be in the subtyping tests.

I'm not entirely sure why these tests weren't there. I assume there's already some in there? I just moved this block over from core.jl

nanosoldier · 2017-08-04T01:26:27Z

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

tkelman

few minor things, can be addressed later if needed

tkelman · 2017-08-04T05:09:11Z

base/test.jl

@@ -1258,6 +1258,107 @@ function detect_ambiguities(mods...;
 end

 """
+ detect_unbound_args(mod1, mod2...; imported=false)


should mention recursive kwarg here

tkelman · 2017-08-04T11:48:29Z

base/test.jl

+
+# find if var will be constrained to have a definite value
+# in any concrete leaftype subtype of typ
+function constrains_param(var::TypeVar, @nospecialize(typ), cov::Bool)


would be a little clearer to write this argument name out as covariant if that's what it is

tkelman · 2017-08-04T12:28:26Z

src/subtype.c

+ return sub;
+}
+
+JL_DLLEXPORT jl_value_t *jl_match_subtype(jl_value_t *type, jl_value_t *sig)


is this called anywhere, does it need to be exported?

tkelman · 2017-08-04T12:32:07Z

src/typemap.c

@@ -683,9 +649,9 @@ static jl_typemap_entry_t *jl_typemap_lookup_by_type_(jl_typemap_entry_t *ml, jl


 // this is the general entry point for looking up a type in the cache
-// (as a subtype, or with typeseq)
+// as a subtype, or with type_equal


is this now referring to jl_types_equal instead of the Julia Base.typeseq ?

sure but now the comment isn't saying either of those

I meant that all three are the same statement

this is the only place in the entire repo that "type_equal" appears

tkelman · 2017-08-04T12:48:01Z

test/tuple.jl

+ @test convert(Tuple{Int, Vararg{Int}}, (1, 2, 3)) == (1, 2, 3)
+ @test convert(Tuple{Vararg{T}} where T<:Integer, (1, 2, 3)) == (1, 2, 3)
+ @test convert(Tuple{T, Vararg{T}} where T<:Integer, (1, 2, 3)) == (1, 2, 3)
+ @test convert(Tuple{Int, Int, Float64}, (1, 2, 3)) == (1, 2, 3.0)


this (and the rest here too?) should probably be using ===

JeffBezanson · 2017-08-04T18:40:46Z

Benchmarks look generally good. Why the huge speedup in "splatting"?

KristofferC · 2017-08-04T18:44:41Z

Benchmarks look generally good

Hmm, 6 of the problem benchmarks which are quite large show regressions. I think these benchmarks looks quite bad...

For reference, here are graphs of these regressed problem:

ziggurat: http://kristofferc.bitnamiapp.com/timeline/#/?exe=7,6,8&ben=problem-ziggurat-ziggurat&env=3&revs=1000&equid=off&quarts=on&extr=on
raytrace: http://kristofferc.bitnamiapp.com/timeline/#/?exe=7,6,8&ben=problem-raytrace-raytrace&env=3&revs=1000&equid=off&quarts=on&extr=on
stockcorr: http://kristofferc.bitnamiapp.com/timeline/#/?exe=7,6,8&ben=problem-stockcorr-stockcorr&env=3&revs=1000&equid=off&quarts=on&extr=on
imdb: http://kristofferc.bitnamiapp.com/timeline/#/?exe=7,6,8&ben=problem-imdb-centrality&env=3&revs=1000&equid=off&quarts=on&extr=on
grigoris: http://kristofferc.bitnamiapp.com/timeline/#/?exe=7,6,8&ben=problem-grigoriadis khachiyan-grigoriadis_khachiyan&env=3&revs=1000&equid=off&quarts=on&extr=on

JeffBezanson · 2017-08-04T18:52:53Z

src/subtype.c

@@ -2583,6 +2585,46 @@ JL_DLLEXPORT int jl_type_morespecific_no_subtype(jl_value_t *a, jl_value_t *b)
 return type_morespecific_(a, b, 0, NULL);
 }

+JL_DLLEXPORT jl_svec_t *jl_match_method(jl_value_t *a, jl_value_t *b)


This function is identical to jl_env_from_type_intersection; let's remove it. (jl_env_from_type_intersection looks like it's missing a GC root though)

And since it returns a pair it should be renamed e.g. jl_type_intersection_with_env.

JeffBezanson · 2017-08-04T18:55:00Z

base/tuple.jl

@@ -63,6 +63,7 @@ first(t::Tuple) = t[1]
 # eltype

 eltype(::Type{Tuple{}}) = Bottom
+#eltype(::Type{Tuple{Vararg{E}}}) where {E} = E


This could use a comment or TODO note, or delete it.

JeffBezanson · 2017-08-04T18:56:45Z

src/codegen.cpp

@@ -3064,7 +3064,12 @@ static jl_cgval_t emit_sparam(jl_codectx_t &ctx, size_t i)
 T_prjlvalue,
 ctx.spvals_ptr,
 i + sizeof(jl_svec_t) / sizeof(jl_value_t*));
- return mark_julia_type(ctx, tbaa_decorate(tbaa_const, ctx.builder.CreateLoad(bp)), true, jl_any_type, false);
+ Value *sp = tbaa_decorate(tbaa_const, ctx.builder.CreateLoad(bp));
+ Value *isnull = ctx.builder.CreateICmpNE(emit_typeof(ctx, sp),


Looks like this variable should be called isdef instead?

That would probably have been a more logical choice. Currently, I'm just maintaining compatibility with the handful of other places we got this backwards, and leaving fixing the names to a future PR author.

KristofferC · 2017-08-04T19:44:06Z

I really think the problem benchmarks should be discussed more.

ararslan · 2017-08-04T19:53:47Z

We can check if they persist in another Nanosoldier run: @nanosoldier runbenchmarks(ALL, vs=":master")

JeffBezanson · 2017-08-04T20:11:06Z

Here's what I see in the graphs:

ziggurat: Gradually got worse since Jan 1.
raytrace: big spike early July
stockcorr: Got worse starting around Jan 1, got better in May.
imdb: Has been improving
grigoriadis: Very similar curve to ziggurat

It definitely seems worth investigating what happened starting in the Jan-Mar timeframe, but it's not clear to me if any of this relates to this PR.

KristofferC · 2017-08-04T20:12:45Z

It relates to the PR in that a 20% bump for some of these problem-benchmarks would make them right now the slowest they have ever been since we started recording it.

nanosoldier · 2017-08-04T23:10:58Z

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

JeffBezanson · 2017-08-04T23:29:31Z

Looks like raytracing still shows a slowdown but the other slowdowns in the "problem" benchmarks went away.

tkelman · 2017-08-04T23:31:00Z

nullablearray and one of the "random" benchmarks also show a consistent more-than-2x slowdown

vtjnash · 2017-08-07T23:48:26Z

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

nanosoldier · 2017-08-08T03:27:01Z

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

vtjnash · 2017-08-08T22:04:06Z

Looks like everything is good here now. OK to merge?

JeffBezanson · 2017-08-08T22:11:33Z

Yep, ok by me.

matching a method signature is a simple subtyping test, with no additional rule to ensure that all parameters have a value and adds a Test for possibly unbound parameters, which allows detecting methods definitions that this change affects

StefanKarpinski · 2017-08-09T22:41:26Z

I'm more excited about this change than I have any right to be.

ararslan · 2017-08-09T22:45:30Z

NEWS.md

+ Linting in package tests is recommended to confirm that the set of methods
+ which might throw `UndefVarError` when accessing the static parameters
+ (`need_to_handle_undef_sparam = Set{Any}(m.sig for m in Test.detect_unbound_args(Base, recursive=true))`)
+ is equal (`==`) to some known set (`expected = Set()`). ([#TBD])


Should update the TBD here

vtjnash added the domain:types and dispatch Types, subtyping and method dispatch label Aug 3, 2017

vtjnash requested a review from JeffBezanson August 3, 2017 16:42

kmsquire reviewed Aug 3, 2017

View reviewed changes

vtjnash force-pushed the jn/dispatch-subtype branch from 22d277c to d0381be Compare August 3, 2017 19:16

ararslan changed the title ~~remove additional rule that parameters must match for dispath~~ remove additional rule that parameters must match for dispatch Aug 3, 2017

JeffBezanson reviewed Aug 3, 2017

View reviewed changes

tkelman reviewed Aug 4, 2017

View reviewed changes

JeffBezanson reviewed Aug 4, 2017

View reviewed changes

JeffBezanson approved these changes Aug 4, 2017

View reviewed changes

vtjnash force-pushed the jn/dispatch-subtype branch 2 times, most recently from ec169f1 to bb57ae4 Compare August 7, 2017 22:13

vtjnash force-pushed the jn/dispatch-subtype branch from bb57ae4 to da29ff2 Compare August 9, 2017 02:46

vtjnash merged commit e6617a6 into master Aug 9, 2017

vtjnash deleted the jn/dispatch-subtype branch August 9, 2017 16:38

ararslan reviewed Aug 9, 2017

View reviewed changes

Sacha0 added a commit to Sacha0/julia that referenced this pull request Aug 10, 2017

Touch up news entry for JuliaLang#23117 and add link to PR.

3beee61

JeffBezanson mentioned this pull request Aug 10, 2017

drop dispatch rule that all method type parameters must be captured #21026

Closed

ararslan pushed a commit that referenced this pull request Aug 10, 2017

Touch up news entry for #23117 and add link to PR. (#23196)

8c7f01e

fredrikekre mentioned this pull request Aug 24, 2017

hvcat error for large, mixed-type matrix literals #23430

Closed

This was referenced Sep 10, 2017

Confusing MethodError for missing parameter #17319

Closed

Partial parameter match leads to confusing MethodError #18110

Closed

Remove warning for non occurring static parameter in method definition #23651

Merged

yuyichao mentioned this pull request Oct 14, 2017

Method specificity bug with Union #24124

Closed

rdeits mentioned this pull request Jan 26, 2018

foreach broken on Julia master due to method matching changes tkoolen/TypeSortedCollections.jl#19

Closed

ExpandingMan mentioned this pull request Feb 23, 2018

Overhauled to Arrow Back-End and Better Memory Safety JuliaData/Feather.jl#78

Merged

HarrisonGrodin mentioned this pull request Jul 26, 2018

Parameterized vararg called with no arguments #28278

Closed

JeffBezanson mentioned this pull request Oct 24, 2018

type parameter UndefVarError when dispatch matches UnionAll #29788

Closed

		@@ -135,10 +135,10 @@ copy(d::Dict) = Dict(d)

		const AnyDict = Dict{Any,Any}

		Dict(ps::Pair...) = Dict{Any,Any}(ps) # define first, since method sort order is wrong for the following

remove additional rule that parameters must match for dispatch #23117

remove additional rule that parameters must match for dispatch #23117

Conversation

vtjnash commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented Aug 4, 2017

tkelman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Aug 4, 2017

KristofferC commented Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC commented Aug 4, 2017 • edited Loading

ararslan commented Aug 4, 2017

JeffBezanson commented Aug 4, 2017

KristofferC commented Aug 4, 2017 • edited Loading

nanosoldier commented Aug 4, 2017

JeffBezanson commented Aug 4, 2017

tkelman commented Aug 4, 2017

vtjnash commented Aug 7, 2017

nanosoldier commented Aug 8, 2017

vtjnash commented Aug 8, 2017

JeffBezanson commented Aug 8, 2017

StefanKarpinski commented Aug 9, 2017

Choose a reason for hiding this comment

KristofferC commented Aug 4, 2017 •

edited

Loading

KristofferC commented Aug 4, 2017 •

edited

Loading

KristofferC commented Aug 4, 2017 •

edited

Loading