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

remove additional rule that parameters must match for dispatch #23117

Merged
merged 1 commit into from
Aug 9, 2017

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Aug 3, 2017

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

@vtjnash vtjnash added the types and dispatch Types, subtyping and method dispatch label Aug 3, 2017
@vtjnash vtjnash requested a review from JeffBezanson August 3, 2017 16:42
base/dict.jl Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

Bump, @vtjnash.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@vtjnash vtjnash force-pushed the jn/dispatch-subtype branch from 22d277c to d0381be Compare August 3, 2017 19:16
@ararslan 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
@ararslan
Copy link
Member

ararslan commented Aug 3, 2017

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

# 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
Copy link
Member

Choose a reason for hiding this comment

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

T is not a method parameter here.

Copy link
Member Author

Choose a reason for hiding this comment

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

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}})

@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)
Copy link
Member

Choose a reason for hiding this comment

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

Should be in the subtyping tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

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

Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

few minor things, can be addressed later if needed

base/test.jl Outdated
@@ -1258,6 +1258,107 @@ function detect_ambiguities(mods...;
end

"""
detect_unbound_args(mod1, mod2...; imported=false)
Copy link
Contributor

Choose a reason for hiding this comment

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

should mention recursive kwarg here

base/test.jl Outdated

# 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

src/subtype.c Outdated
return sub;
}

JL_DLLEXPORT jl_value_t *jl_match_subtype(jl_value_t *type, jl_value_t *sig)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

same thing

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant that all three are the same statement

Copy link
Contributor

Choose a reason for hiding this comment

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

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

test/tuple.jl Outdated
@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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@JeffBezanson
Copy link
Member

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

@KristofferC
Copy link
Member

KristofferC commented Aug 4, 2017

src/subtype.c Outdated
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

base/tuple.jl Outdated
@@ -63,6 +63,7 @@ first(t::Tuple) = t[1]
# eltype

eltype(::Type{Tuple{}}) = Bottom
#eltype(::Type{Tuple{Vararg{E}}}) where {E} = E
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this variable should be called isdef instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

KristofferC commented Aug 4, 2017

I really think the problem benchmarks should be discussed more.

@ararslan
Copy link
Member

ararslan commented Aug 4, 2017

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

@JeffBezanson
Copy link
Member

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
Copy link
Member

KristofferC commented Aug 4, 2017

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
Copy link
Collaborator

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

@JeffBezanson
Copy link
Member

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

@tkelman
Copy link
Contributor

tkelman commented Aug 4, 2017

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

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

vtjnash commented Aug 7, 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 @ararslan

@vtjnash
Copy link
Member Author

vtjnash commented Aug 8, 2017

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

@JeffBezanson
Copy link
Member

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
@vtjnash vtjnash force-pushed the jn/dispatch-subtype branch from bb57ae4 to da29ff2 Compare August 9, 2017 02:46
@vtjnash vtjnash merged commit e6617a6 into master Aug 9, 2017
@vtjnash vtjnash deleted the jn/dispatch-subtype branch August 9, 2017 16:38
@StefanKarpinski
Copy link
Member

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

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])
Copy link
Member

Choose a reason for hiding this comment

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

Should update the TBD here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants