Remove all @inbounds annotations
#2005
Conversation
Julia is quite good at eliminating bounds checks on its own, and incorrect use of `@inbounds` can lead to hard-to-diagnose bugs. There are also cases where adding it actively hurts performance (e.g., JuliaLang/julia#48245). It's much better to allow the sophisticated analysis performed by inference to make this decision and it is to reflexively add `@inbounds` everywhere. Fixes JuliaStats#1265.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2005 +/- ##
==========================================
+ Coverage 86.28% 86.35% +0.07%
==========================================
Files 146 146
Lines 8787 8782 -5
==========================================
+ Hits 7582 7584 +2
+ Misses 1205 1198 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This is maybe a bit extreme to fix #1265 (which arguably is not an actual problem for most users and could be addressed by ensuring one-based indices when not operating with generic indices). AFAIK the benchmarks in this package haven't been updated in a long time, and also seem to cover only a few samplers and a few evaluations for mixture models. All other distributions are not covered by the benchmarks at all. So IMO,
In all other cases (safe Currently, my impression is that this PR removes |
I think in 2025 the burden-of-proof should go the other way: you should keep them only if you can show them to be necessary. Julia is not the same language it was back in 2014 and the compiler is much better at this than we are. Constant-propagation is an extremely useful optimization, one likely to be useful for this package, and I can't remember the last time I annotated a line with |
|
OK, I'm convinced 🙂 I just didn't trust the reasoning based on benchmarking in #1265 (comment) as it only covers a tiny subset of the package (which makes the performance checks a bit questionable...). If we/users see regressions, we can always add |
3 participants
Julia is quite good at eliminating bounds checks on its own, and incorrect use of
@inboundscan lead to hard-to-diagnose bugs. There are also cases where adding it actively hurts performance (e.g., JuliaLang/julia#48245). It's much better to allow the sophisticated analysis performed by inference to make this decision and it is to reflexively add@inboundseverywhere.Fixes #1265.