Skip to content

Commit e805fd6

Browse files
vtjnashvtjnash
committedApr 19, 2014
don't inline more than a reasonable amount -- our inliner was good enough now that it could generate enormous 1-liners. fix #6566. close #6569
·
v1.11.4v0.3.0-rc1
1 parent 5636ec5 commit e805fd6

File tree

1 file changed

+1
-5
lines changed

1 file changed

+1
-5
lines changed
 

‎base/inference.jl

+1-5
Original file line numberDiff line numberDiff line change
@@ -2239,15 +2239,11 @@ function inlineable(f, e::Expr, atypes, sv, enclosing_ast)
22392239
end
22402240

22412241
function inline_worthy(body::Expr)
2242-
# see if body is only "return <expr>", or is otherwise considered worth inlining
2243-
if length(body.args) == 1
2244-
return true
2245-
end
22462242
# if isa(body.args[1],QuoteNode) && (body.args[1]::QuoteNode).value === :inline
22472243
# shift!(body.args)
22482244
# return true
22492245
# end
2250-
if length(body.args) < 4 && occurs_more(body, e->true, 50) < 50
2246+
if length(body.args) < 6 && occurs_more(body, e->true, 40) < 40
22512247
return true
22522248
end
22532249
return false

10 commit comments

Comments
 (10)

JeffBezanson commented on Apr 19, 2014

@JeffBezanson
JeffBezanson
on Apr 19, 2014
Member

Why did it hang? Surely "enormous" is not "infinite"?

JeffBezanson commented on Apr 19, 2014

@JeffBezanson
JeffBezanson
on Apr 19, 2014
Member

Ah, looking at the issue I think I see.

vtjnash commented on Apr 19, 2014

@vtjnash
vtjnash
on Apr 19, 2014
MemberAuthor

not infinite, but I think it may have been exponential in the number of terms. i think we need have the inliner consider the size of an argument against the number of copies that it is going to make, to decide whether it is better to make a local variable or a copy

vtjnash commented on Apr 19, 2014

@vtjnash
vtjnash
on Apr 19, 2014
MemberAuthor

looking at the issue, i see you already mentioned that :P

stevengj commented on Apr 22, 2014

@stevengj
stevengj
on Apr 22, 2014
Member

Looks like this commit may have hurt my Julia FFT performance (in #6193). Non-power-of-two sizes had recently gotten a lot better in performance, but after merging the commits from after April 17 it suddenly got about 50% worse (though not as bad as before). I haven't done a bisect yet, but this is the only commit that looks like it could have made such a difference.

JeffBezanson commented on Apr 22, 2014

@JeffBezanson
JeffBezanson
on Apr 22, 2014
Member

Yeah, we need to do better. For now we can probably just increase this threshold.

stevengj commented on Apr 22, 2014

@stevengj
stevengj
on Apr 22, 2014
Member

I can confirm that reverting this commit restores my earlier performance. I'll play with the thresholds to see where the point of diminishing returns occurs.

stevengj commented on Apr 23, 2014

@stevengj
stevengj
on Apr 23, 2014
Member

It seems to be sufficient to change the threshold to 100 to restore the old performance:

--- a/base/inference.jl
+++ b/base/inference.jl
@@ -2243,7 +2243,7 @@ function inline_worthy(body::Expr)
 #        shift!(body.args)
 #        return true
 #    end
-    if length(body.args) < 6 && occurs_more(body, e->true, 40) < 40
+    if length(body.args) < 6 && occurs_more(body, e->true, 100) < 100
         return true
     end
     return false

Increasing the threshold further yields no benefit for me.

ViralBShah commented on Apr 25, 2014

@ViralBShah
ViralBShah
on Apr 25, 2014
Member

Should we add the fft implementation to our benchmarks?

stevengj commented on Apr 25, 2014

@stevengj
stevengj
on Apr 25, 2014
Member

@ViralBShah, when it is merged it would probably make a good stress test to check for performance regressions. I'm hoping to have it ready by sometime early in the 0.4 cycle.

Please sign in to comment.