Skip to content

Commit

Permalink
don't inline more than a reasonable amount -- our inliner was good en…
Browse files Browse the repository at this point in the history 
…ough now that it could generate enormous 1-liners. fix #6566. close #6569
  • Loading branch information
@vtjnash
vtjnash committed Apr 19, 2014
1 parent 5636ec5 commit e805fd6
Showing 1 changed file with 1 addition and 5 deletions.
6 changes: 1 addition & 5 deletions base/inference.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2239,15 +2239,11 @@ function inlineable(f, e::Expr, atypes, sv, enclosing_ast)
end

function inline_worthy(body::Expr)
# see if body is only "return <expr>", or is otherwise considered worth inlining
if length(body.args) == 1
return true
end
# if isa(body.args[1],QuoteNode) && (body.args[1]::QuoteNode).value === :inline
# shift!(body.args)
# return true
# end
if length(body.args) < 4 && occurs_more(body, e->true, 50) < 50
if length(body.args) < 6 && occurs_more(body, e->true, 40) < 40
return true
end
return false
Expand Down

10 comments on commit e805fd6

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

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

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

Ah, looking at the issue I think I see.

@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.

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@stevengj
Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Should we add the fft implementation to our benchmarks?

@stevengj
Copy link
Member

Choose a reason for hiding this comment

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

@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.