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.

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

Inlining cost model: don't count :invokes that throw #22732

Merged
merged 1 commit into from
Jul 10, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 10, 2017

The throw is not typically part of the standard runtime cost of the function. This should fix the performance concern with #20005, which turns out to be an inlining difference.

@@ -4649,7 +4649,12 @@ function statement_cost(ex::Expr, src::CodeInfo, mod::Module, params::InferenceP
end
return plus_saturate(argcost, params.inline_nonleaf_penalty)
elseif head == :foreigncall || ex.head == :invoke
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated but head == :invoke ? (Or ===)

The throw is not typically part of the standard runtime cost of the function.
@timholy
Copy link
Member Author

timholy commented Jul 10, 2017

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against comparison commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @jrevels

@timholy
Copy link
Member Author

timholy commented Jul 10, 2017

Something seems wrong with nanosoldier, possibly JuliaCI/BaseBenchmarks.jl#83. I've checked locally; there are 5 reproducible regressions, the worst of which is 20%. There are also 5 reproducible improvements, of which some are nearly 10x. Since 10x beats 1.2x, I'm merging.

@timholy timholy merged commit 86395fa into master Jul 10, 2017
@timholy timholy deleted the teh/inlining_throw branch July 10, 2017 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants