Open timholy opened 6 years ago
The performance improvement in #29796 is mostly due to https://github.com/JuliaLang/julia/pull/29796/files#diff-2ac58dca1441c1a860eb7aea2f488dacR251 so the @inline
on the length
might be a red herring (and should perhaps have been in another PR).
I just saw that length
used @boundscheck
which is only active if the function gets inlined... So I inlined it.
I think you did the right thing. But it would be better if we didn't need these manual annotations.
In an ideal world, perhaps the right approach would be to compare estimated costs with and without each dependent function inlined---then things that, e.g., make a difference for @boundscheck
, or allow some dramatic simplification of the caller and callee, would get an automatic bonus. However, I suspect that from a compiler-performance standpoint that kind of change would be insane.
29796 contains a couple of manual
@inline
s that shouldn't be necessary. Analysis oflength(::String, ::Int, ::Int)
indicates that:invoke
s tothrow
) account for a total cost of 140. This would be fixed by implementing a real version of #23986 (a high priority of mine when I find the time)foreigncall
s are tojl_value_ptr
which gets specially handled by the compiler and thus shouldn't pay a cost of 20 units. I've not yet calculated what the right cost is, but this could save up to 100 units of the budgetOptimistically this would shave the cost of that function down to 135 units. This is still above our current threshold of 100 for inlining, so either (1) this might need further work or (2) we should raise the threshold for inlining. (The performance improvements in #29796 are quite dramatic and clear evidence that we are not doing the right thing currently.)