RFC: highlighting type problems

[timholy]

One of the major "services" that julia developers provide on the mailing lists is helping people solve performance problems due to type instability. It's pretty easy to make such errors (I've made more than a few in my own time), but personally, I'd like to spend less time on such support issues. As usual, my response is to say "we need easier/more powerful analysis tools." So, here's proposing a new macro,

@code_typewarn myfunction(args...)

which produces output like this:

In the course of testing this I discovered #9345, and might not have noticed it without the red highlighting.

I see two main paths for this facility: it goes in base (aka, this PR; note it's not a lot of code), or it goes in a package like TypeCheck.jl or Lint.jl. I was originally planning on submitting it to TypeCheck, but it occurred to me that if folks think this should belong in base, then I'd just be giving @astrieanna a support headache by putting it in the package and then migrating it out again. So I thought I'd start here and see what people say.

Warnings: this does nassty, wicked things to our hobbitses, erm, the printing of types. It's quite possible that using this will bork your julia session in much the same way as "convertalypse" or #9315. (It did once for me during testing, albeit before it was fully working.) I would also characterize the testing of this as "minimal," on perhaps 20 functions. Hence the RFC.

[vtjnash]

this looks amazing.

of course, regardless of where it goes, it needs documentation (e.g. "it's not useful if people don't know about it.")

[ViralBShah]

I would love to have something like this in base.

[tonyhffong]

💯 such awesomeness.

[rfourquet]

Awesome!
I played a bit with it today and noticed that sometimes a reported type instablity comes from an @inlined effectful utility function, which in my understanding don't need (for efficiency) to be stable if the result is unused in the caller. If it is difficult to identify this particular case for code_typewarn, I would suggest at least to say in its doc that "red" doesn't always mean something is wrong.
Also a minor detail: @code_warntype would be better from a purely "REPL completion" point of view (@co<Tab>w<Tab>).

[timholy]

I made a couple more tweaks. Now let's play "how fast can you spot the problem without @code_typed?" (I'm sure for many of you, it's about 2 seconds):

julia> type ArrayContainer{T}
           data::Array{T}
       end

julia> Base.getindex(A::ArrayContainer, i::Int) = A.data[i]
getindex (generic function with 166 methods)

julia> A = ArrayContainer(rand(5))
ArrayContainer{Float64}([0.33665063426543473,0.11482886957148519,0.269839408725715,0.02057927127734449,0.34632235507523146])

And the answer (no peeking if you haven't tried):

It's obviously an incredibly simple case, but it's the type of mistake that has eluded the eyes of more than one reviewer on the mailing lists. Presumably if we combine this with a doc page that lists the origin of common type problems, a lot of users would be more empowered to figure this stuff out on their own.

[johnmyleswhite]

This is really great. I think this kind of thing is the biggest source of unpredictability in performance for Julia, so literally highlighting these things will be a huge help to people.

[timholy]

@rfourquet, I totally agree and will change the name to @code_warntype. Great suggestion.

Can you give me a concrete example of the spurious warning you're describing? It's definitely the case that this will warn about things that are only minimally a problem. For example:

f() = ntuple(2, i->1)::(Int,Int)
g(x) = f()[2]+x

in a sense, does not have a real type instability problem, but there's a transient type instability because the output of ntuple can't be inferred. The declaration of the output type is something that gets added on, and fixes problems for later code, but it's not incorrect to say that there's still a point here where the type is unknown. I suspect we'll just have to live with this, and solve it (as you say) through documentation.

[timholy]

OK, I've done the name change and added some fairly extensive documentation. One thing I couldn't figure out was how to get reST to do the red highlighting, so I used all-caps. Any gurus out there know how to do better?

[jakebolewski]

This is really great, but I feel functionality like this should be in a package rather than in Base. I'm probably in the minority with that opinion though. The number of features getting packed into Base seems to be growing rapidly and seems counter to the goal of .#5155.

[StefanKarpinski]

+1

[StefanKarpinski]

Sorry, that was missing several zeros.

[timholy]

The one thing I worry about with putting it in a package is #265---I'm pretty sure that when I define this outside of julia, I sometimes get WarnType{Float64} printed directly via the "old" definition of show_expr_type. But I haven't tested recently, maybe it is just due to bugs in my out-of-tree version.

If true, we could keep just that portion in base and put the rest in a package.

[timholy]

OK, I experimented again with taking it out of base. It seems to work fine, so presumably my original problems were due to transient bugs.

The explicit votes are 1 in favor of having it in base, and 1 opposed. Everyone else who has commented seems to like the functionality, but hasn't expressed an opinion about where it should live. I suspect it's best to count those as mild votes in favor of keeping this in a package. I'll wait a bit to give folks a chance to weigh in on this specific topic; if no feedback, I'll go the package route.

[tknopp]

Hey Tim, I have not tested your patch but +1000 for having such functionality in base. Type stability is one of the most crucial things in Julia and IMHO Julias Achilles heel...

[jiahao]

I have to say, the visual effect is very nice.

For the md.jl benchmark in my parallel-bench branch (a toy molecular dynamics code deliberately written naively), I get immediate feedback on where the type instability lies. For example, this little snippet integrating the state forward in time using velocity Verlet reveals type instability due to calling preduce that is hard to fathom immediately:

 @code_warntype update!(particles, 1.e-3, velocityverlet)

[timholy]

Nice example, @jiahao!

[timholy]

Although I see at least one thing that needs fixing. I expect we'll find quite a few such things in the early days.

[timholy]

That red annotation on preduce is not the call itself; it's the scoping expression Base.preduce. That supposedly should be handled by ismodulecall in 312837b, but clearly it isn't always working. I try examples like these:

julia> f(x,p) = Base.power_by_squaring(x, p)
f (generic function with 2 methods)

julia> module TestMe

       export fpower

       function fpower(x, p)
           Base.power_by_squaring(x, p)
       end

       end

and it all works fine. So I don't understand.

[ivarne]

I'm a strong proponent for #5155, but I don't think putting this (temporary?) in base will be problematic.

This will be used in interactive sessions and I can't really see when someone will write a program that uses this. Therefore we can deprecate this at any time and give instructions to users how to get the new tools.

Adding a package is a barrier for some people, and this is one of the first things we should recommend everyone with a performance problem to use.

[JeffBezanson]

It feels a bit too complicated to implement this by copying the AST. show for expressions could just check a flag and show certain types in red. Technically that is a hack, and the approach here is more composable, but I feel like it would save such a large amount of code that it would make sense.

[quinnj]

Looking forward to playing with this. +1 to having it in Base.

[rfourquet]

The example I mentioned which seems safe to me is:

maybe_fill!(b, A) = b && fill!(A, 0) # type unstable, used locally only inside myfunc
function myfunc()
    ...
    b, A = ...
    maybe_fill!(b, A)
    ...
    return true
end

So the return value of maybe_fill is never used, but is assigned (as per code_typed/code_typewarn) to a unused variable like _var1 (I think I read yesterday itself that unused vars are indeed optimized away).
And +1 for @code_warntype in base.

[timholy]

There was a comment that appeared in my email in this thread that perhaps got deleted. But the comment raised some good points about "will this really be useful if I don't know how to interpret the output?"

For the record, this is a very reasonable question; interpreting the output of @code_typed is not remotely straightforward. Many of us who have been using Julia for a long time have learned how to do it, and so most of us chatting here just take it for granted.

But this will not be the case for most people using julia, nor is it reasonable to expect that will be the case. My first attempt at a solution to this was documentation; it's probably not as easy to read in the commits as it will be when formatted on the web page, but among them there's a lookup table "if you see X, it means Y, fix it by doing Z."

But partially prompted by your comment, I agree we should think about doing even better. I just haven't even begun to think about that particular problem yet. I suspect @tonyhffong and @astrieanna might have some good ideas, since their packages already serve this niche.

[sbromberger]

I confess to having written the now-deleted comment. I typed it and then realized three things: first, I didn't want to bring up an issue for which I didn't have a proposed solution; second, I didn't want my name associated with something that looked like a complaint or criticism when I'm just a week into this great language; and finally, I did what I should've done before typing: I browsed the commit source and found that wonderful documentation (as an aside, docstrings can't come soon enough). It gave me some insight, as a new Julia user, how I might use the macro (though I confess the whole type thing is still very confusing to me, since I got lazy with Python).

I'm very pleased that someone saw and took the comment in the spirit in which it was intended. I'm still not sure I'm smart enough for this language, but I'm doing my best with it, and this macro looks like an great addition to the suite of tools I can use to write better (I'll settle for "working" at this point, though) code.

Thanks for letting me get my $0.02 in.

[quinnj]

@sbromberger, I thought it was an extremely relevant comment. As @timholy said, a lot of us have gotten used to the @code_typed output, and so the output here is (mostly) interpretable, but it's certainly not going to be something very user-friendly for newcomer's, in part who is hoped to benefit the most from this.

[rfourquet]

"even if"

[JeffBezanson]

@sbromberger thank you for your perspective on this. I don't think this is a tool that we should encourage everybody to use all the time. Like most automated tools, it has a certain false-positive rate. It shows a lot of stuff you don't really need to know, and identifies "problems" that are not necessarily problems.

[StefanKarpinski]

I accidentally pushed an unrelated commit to this branch and then force pushed it away, hopefully I didn't mess anything up here :-\

[timholy]

No worries, I'll be doing some force-pushing anyway when I get time to address the review comments. But thanks for letting me know, I really appreciate it.

[timholy]

OK, I've updated this to incorporate all the review comments. Here's the new form of the output.

[vchuravy]

💯 oh sweet

[joehuchette]

This is fantastic!