Skip to content

RFC: Avoid over-pessimization in apply_type_tfunc#27150

Merged
martinholters merged 1 commit intomasterfrom
mh/apply_type_tfunc
Jun 13, 2018
Merged

RFC: Avoid over-pessimization in apply_type_tfunc#27150
martinholters merged 1 commit intomasterfrom
mh/apply_type_tfunc

Conversation

@martinholters
Copy link
Copy Markdown
Member

@martinholters martinholters commented May 18, 2018

@martinholters martinholters requested a review from vtjnash May 18, 2018 11:17
@martinholters martinholters added the compiler:inference Type inference label May 18, 2018
@martinholters
Copy link
Copy Markdown
Member Author

martinholters commented May 18, 2018 •
edited
Loading

Hm, LibGit2/libgit2 segfaults on 32bit Circle and Travis and hangs on FreeBSD, so likely related. Presumable, I now produce a Union{} were I shouldn't. Will try to reproduce locally. Or is that a failure we've seen elsewhere recently?

@martinholters
Copy link
Copy Markdown
Member Author

martinholters commented May 18, 2018

Will try to reproduce locally.

Works locally with a 32bit build. Any ideas?

vtjnash
vtjnash reviewed May 18, 2018
View reviewed changes
base/compiler/tfuncs.jl Outdated
else
if !isType(ai)
return Any
if !isa(ai, Type) || ai <: Type || Type <: ai
Copy link
Copy Markdown
Member

@vtjnash vtjnash May 18, 2018

Choose a reason for hiding this comment

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

typeintersect(ai, Type) != Union{}

Copy link
Copy Markdown
Member Author

@martinholters martinholters May 18, 2018

Choose a reason for hiding this comment

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

Good catch!

base/compiler/tfuncs.jl
if !isa(ai.val, Type)
return Union{}
end
else
Copy link
Copy Markdown
Member

@vtjnash vtjnash May 18, 2018

Choose a reason for hiding this comment

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

Checking explicitly for if isa TypeVar might be necessary (And PartialTypeVar)

Copy link
Copy Markdown
Member Author

@martinholters martinholters May 18, 2018

Choose a reason for hiding this comment

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

You mean ai.val could be a TypeVar and we shouldn't return Union{} then?

Copy link
Copy Markdown
Member

@vtjnash vtjnash May 18, 2018

Choose a reason for hiding this comment

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

yes. or ai could be a PartialTypeVar

Copy link
Copy Markdown
Member Author

@martinholters martinholters May 22, 2018

Choose a reason for hiding this comment

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

ai being a PartialTypeVar should be handled below. Do you mean ai.val?

@Sacha0
Copy link
Copy Markdown
Member

Sacha0 commented May 18, 2018

Hm, LibGit2/libgit2 segfaults on 32bit Circle and Travis and hangs on FreeBSD), so likely related. Presumable, I now produce a Union{} were I shouldn't. Will try to reproduce locally. Or is that a failure we've seen elsewhere recently?

Though slightly different, LibGit2 failures recently popped up in #26997 as well. Best!

@martinholters martinholters force-pushed the mh/apply_type_tfunc branch from 2c5e9c1 to a3f4331 Compare May 22, 2018 09:12
martinholters
martinholters commented May 22, 2018
View reviewed changes
base/compiler/tfuncs.jl Outdated
if isa(ai, Const)
if !isa(ai.val, Type)
if isa(ai.val, TypeVar) || isa(ai.val, PartialTypeVar)
return hasnonType = true
Copy link
Copy Markdown
Member Author

@martinholters martinholters May 22, 2018

Choose a reason for hiding this comment

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

Did you mean like so, @vtjnash?

Copy link
Copy Markdown
Member Author

@martinholters martinholters Jun 5, 2018

Choose a reason for hiding this comment

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

Bump. I'm still not sure you meant this. A Const PartialTypeVar looks a bit funny here to me.

Copy link
Copy Markdown
Member

@vtjnash vtjnash Jun 5, 2018

Choose a reason for hiding this comment

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

isa(ai.val, PartialTypeVar) can return Union{}. But ai itself can be a PartialTypeVar

Copy link
Copy Markdown
Member Author

@martinholters martinholters Jun 6, 2018

Choose a reason for hiding this comment

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

Alright, then I've apparently misinterpreted your earlier remark. The isa(ai, PartialTypeVar) case is already handled below (in the !isType(ai) and !isa(ai, Type) branch).

@martinholters
Copy link
Copy Markdown
Member Author

martinholters commented May 23, 2018

AV 32bit got far but timed out, 64bit failed for a reason I don't really understand. Why did FreeBSD not run? Anyway, the LibGit2/libgit2 segfaults did not happen again.

@martinholters martinholters force-pushed the mh/apply_type_tfunc branch 2 times, most recently from 50a3a3e to e6ef253 Compare June 6, 2018 13:48
@martinholters
Copy link
Copy Markdown
Member Author

martinholters commented Jun 7, 2018 •
edited
Loading

Oh man, CI really doesn't like this one. AV timed out, an occasional libgit2 failure, and the rest

Worker 3 failed running test OldPkg/pkg:
Some tests did not pass: 2 passed, 0 failed, 1 errored, 0 broken.OldPkg/pkg: Error During Test at /tmp/julia/share/julia/test/testdefs.jl:19
  Got exception LoadError("/tmp/julia/share/julia/stdlib/v0.7/OldPkg/test/pkg.jl", 56, KeyError("SparseArrays")) outside of a @test
  LoadError: KeyError: key "SparseArrays" not found

or s/SparseArrays/Printf. All of these are commonly seen, and tests get pretty far before hitting them, so this is probably ok. OTOH, this is not urgent at all, so we can just rerun CI once it feels better. Meanwhile, @vtjnash, could you verify whether I've (correctly) addressed all your comments?

@martinholters martinholters reopened this Jun 11, 2018
@martinholters
Copy link
Copy Markdown
Member Author

martinholters commented Jun 11, 2018

Oh, rebasing this led to an all-green CI! As a consequence, I'm unsure whether I'll ever dare touching this again. So...good to go?

@StefanKarpinski
Copy link
Copy Markdown
Member

StefanKarpinski commented Jun 11, 2018

I would give one of @JeffBezanson, @Keno or @vtjnash 24 hours to comment and then merge :trollface:

@vtjnash vtjnash requested a review from nanosoldier June 11, 2018 19:13
@StefanKarpinski
Copy link
Copy Markdown
Member

StefanKarpinski commented Jun 11, 2018

Ooo! Requesting a review from @nanosoldier, fancy.

@martinholters
Copy link
Copy Markdown
Member Author

martinholters commented Jun 12, 2018

Fancy indeed. But apparently, doesn't trigger him, so @nanosoldier runbenchmarks(ALL, vs=":master").

@nanosoldier
Copy link
Copy Markdown
Collaborator

nanosoldier commented Jun 12, 2018

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

@martinholters
Copy link
Copy Markdown
Member Author

martinholters commented Jun 12, 2018

I'm inclined to declare regressions as well as improvements there noise and will merge soonish unless anyone objects.

@martinholters martinholters merged commit 1f3d1df into master Jun 13, 2018
@martinholters martinholters deleted the mh/apply_type_tfunc branch June 13, 2018 06:19
@martinholters martinholters mentioned this pull request Jan 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vtjnash vtjnash vtjnash approved these changes

@nanosoldier nanosoldier Awaiting requested review from nanosoldier

Assignees

No one assigned

Labels

compiler:inference Type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@martinholters @Sacha0 @StefanKarpinski @nanosoldier @vtjnash