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.

Sign up for GitHub

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

Jump to bottom

RFC: Avoid over-pessimization in apply_type_tfunc #27150

Merged
merged 1 commit into from
Jun 13, 2018
Merged

RFC: Avoid over-pessimization in apply_type_tfunc #27150

merged 1 commit into from
Jun 13, 2018

Conversation

martinholters
Copy link
Member

@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
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
Member Author

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
if !isType(ai)
return Any
if !isa(ai, Type) || ai <: Type || Type <: ai
Copy link
Member

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
Member Author

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 Outdated
if !isa(ai.val, Type)
return Union{}
end
else
Copy link
Member

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
Member Author

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
Member

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
Member Author

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
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
Member Author

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
Member Author

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
Member

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
Member Author

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
Member Author

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
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
Member Author

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
Member

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
Member

Ooo! Requesting a review from @nanosoldier, fancy.

@martinholters
Copy link
Member Author

Fancy indeed. But apparently, doesn't trigger him, so @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

@martinholters
Copy link
Member Author

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