[RFC] [performance] {.inline.} guarantees, `--stacktrace:inline`, and `--ignoreInline`

[timotheecour]

some of these (especially P1) will result in a large speedup for builds with --stacktrace:off (eg debug builds)

links

• originally discussed here fix operators containing percent for VM usage Nim#13536 (comment)
• https://clang.llvm.org/docs/AttributeReference.html#gnu-inline describing gnu_inline semantics
• https://stackoverflow.com/questions/25602813/force-a-function-to-be-inline-in-clang-llvm
• https://gustedt.wordpress.com/2010/11/29/myth-and-reality-about-inline-in-c99/
• D20200303T145713

RFC proposals

• P1 an {.inline.} proc should by default NOT generate nimfr_ + popFrame (regardless of -d:release or not) even with --stacktrace:on. Otherwise --stacktrace:on can be very slow

• P2 a new option should be introduced --stacktrace:inline (implies --stacktrace:on) to generate nimfr_ + popFrame even for {.inline.}

• P3 currently proc fun2x(a: int): int {.inline.} generates:

static N_INLINE(NI, fun2x__rxUfcN4ZvcApCx0waSVH8At10303b)(NI a) {...

regardless of -d:danger or not
according to https://stackoverflow.com/questions/25602813/force-a-function-to-be-inline-in-clang-llvm it seems static should maybe not be used:

So in order to guarantee that the function is inlined:
Don’t use static inline.
Make sure not to compile with GNU89.

we should investigate to make sure it works as intended, and wheter __attribute__((always_inline)) is more appropriate (cf hints vs guarantees)

• P4 #define N_INLINE_PTR(rettype, name) rettype (*name) is dead code: no use of N_INLINE_PTR (and no idea what it's for)

• P5 we should have a compiler option --ignoreInline way to skip inlining (for debug and maybe even release builds, it's and orthogonal decision), helps w debugging esp stacktraces
  => this would only control nim code that cgen's inline, and would NOT be conflated with C-specific flags that controls inlining, so as not to interfere with other libraries; user can always pass --passC:XXX in addition to that if needed

• P6 --ignoreInline shall be usable in cmdline as well as in {.push.}/{.pop.} sections just like --rangeChecks + friends

• P7 an imported {.inline.} proc currently gets its C source code duplicated in every module it's used, which could potentially slow down c code compilation (but no need to fix if benchmarking reveals it's not a bottleneck). So it's some kind of double inlining since we already have the N_INLINE macro static N_INLINE(NI, fun2x__rxUfcN4ZvcApCx0waSVH8At10303b)(NI a) {...}
  Instead we can use what's recommended here https://gustedt.wordpress.com/2010/11/29/myth-and-reality-about-inline-in-c99/ and here https://stackoverflow.com/a/43442230/1426932 and introduce the inline proc definition in a header file, inline fun2x__rxUfcN4ZvcApCx0waSVH8At10303b(NI a) { ... } imported by all clients of it, and then use extern in a single location in a source file

// in inline_definitions.h
inline double dabs(double x) {  return x < 0.0 ? -x : x;}
// in exactly one compilation unit inline_definitions.c
extern inline double dabs(double x);

Nowadays, where the C99 standard still isn’t completely implemented in many compilers, many of them still get it wrong. Gcc corrected its first approach and now (since version 4.2.1) works with the method above. Others that mimic gcc like opencc or icc are still stuck on the old model that gcc had once invented. The include file p99_compiler.h of P99 helps you make sure that your code will work even with these compilers

[Clyybber]

So we make debugging harder, introduce a bunch of new arguments/pragmas that make everything unneccessarily more complex, instead of just using the already existing {.push: stacktraces: off.} when you really need it.

[timotheecour]

so your solution when a program (eg, nim itself) compiled with --stacktrace:on is too slow is to:

• find all the bottlenecks (that's really hard, it depends on use cases, and nimprof introduces bias)
• for each bottleneck, edit sources and wrap those procs inside {.push: stacktraces: off.} {.push: stacktraces: on.}
• recompile
• at the end of debug session, remember to undo those edits

You can't just permanently add {.push: stacktraces: off.} in each suspected location where it could be a bottleneck, that would affect debugability of those locations with no easy way to get traces (unless again, you edit the sources).

I don't see how that's practical, and that's obviously much more complex for users than what I've proposed and implemented: simply compile with --stacktrace:on/off/inline depending on what stacktraces you want, this should cover the vast majority of cases because inline procs happen to be the performance sensitive ones (eg very basic things like math operations etc).
{.push: stacktraces: on.} is always an option beyond tha (profile based using PGO could also be an option, but that's out of scope for this discussion)

So we make debugging harder

no, there's a mode that preserves pre-existing semantics; it's just a question of whether to default to old mode or to new mode.

[Araq]

All that's really needed IMHO is that we map Nim's .inline to C++'s always_inline.

[timotheecour]

well I've updated nim-lang/Nim#13582 so that existing semantics are unchanged, instead the new --stacktrace:noinline can give you large speedups while typically producing identical stracktrace, ie, a good compromise bw speed and debuggability, see nim-lang/Nim#13582 (comment) for details

[timotheecour]

I'd be happier with templates instead of inline procs as inline procs are currently not inlined in debug builds (from nim-lang/Nim#13536 (comment))
All that's really needed IMHO is that we map Nim's .inline to C++'s always_inline.

I did some further investigation, here are the conclusions using clang on OSX (should mostly generalize to gcc on linux):

• the main performance issue with debug builds vs non-debug builds is unrelated to how nim compiler treats {.inline.} (the codegen doesn't care whether it's a debug build or -d:release or -d:danger as far as {.inline.} is concerned); it's instead related to lack of c compiler optimization (eg via --passC:-O or --opt:speed); we may want to consider whether standard debug build (nim c -r main.nim) implies --opt:speed by default, but that should be discussed elsewhere)
• changing N_INLINE from inline to __attribute__((__always_inline__)) would not improve speed with standard debug build, and in some cases could make things worse (as also noted in other articles eg https://aras-p.info/blog/2017/10/09/Forced-Inlining-Might-Be-Slow/), eg:

./bin/nim c --skipParentCfg --skipUserCfg --hints:off -o:bin/nim_temp1 --stacktrace:on -f compiler/nim.nim
bin/nim_temp1 c --skipParentCfg --skipUserCfg --hints:off -o:bin/nim_temp2 -f --stacktrace:off compiler/nim.nim  => 55 seconds

# change N_INLINE from `inline` to `__attribute__((__always_inline__))` 
./bin/nim c --skipParentCfg --skipUserCfg --hints:off -o:bin/nim_temp1 --stacktrace:on -f compiler/nim.nim
bin/nim_temp1 c --skipParentCfg --skipUserCfg --hints:off -o:bin/nim_temp2 -f --stacktrace:off compiler/nim.nim  => 113.9 seconds

• however, in combination with --passC:-Og (equivalent to --passC:-O1), __attribute__((__always_inline__)) does do inlining whereas inline does not.

• passing --passC:-O or --opt:speed greatly improves speed of debug builds (builds without -d:release or -d:danger)
  ./bin/nim c --skipParentCfg --skipUserCfg --hints:off -o:bin/nim_temp1 --stacktrace:on --opt:speed -f compiler/nim.nim
  bin/nim_temp1 c --skipParentCfg --skipUserCfg --hints:off -o:bin/nim_temp2 -f --stacktrace:off compiler/nim.nim => 19 seconds

here's a minimal but illustrative example:

when defined case5:
  import times
  when defined case_normal:
    proc fun[T](x, i: T): T = x + i
  when defined case_inline:
    proc fun[T](x, i: T): T {.inline.} = x + i
  when defined case_noinline:
    proc fun[T](x, i: T): T {.noinline.} = x + i
  when defined case_alwaysinline:
    # modify `N_INLINE` in nimbase.h and replace `inline` by `__attribute__((__always_inline__))`
    proc fun[T](x, i: T): T {.inline.} = x + i
  when defined case_template:
    template fun[T](x, i: T): T = x + i

  proc main() =
    var x: uint = 1
    let n: uint = 1_000_000_000
    var i = uint(0)
    while true:
      x = fun(x,i)
      if i == n: break
      i = i + 1
    doAssert x != 1234
  let t = cpuTime()
  main()
  echo cpuTime() - t

when defined case5_runner:
  import os,strformat
  proc fun(opt: string) =
    const nim = getCurrentCompilerExe()
    let input = currentSourcePath
    let cmd = fmt"{nim} c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f {opt} {input}"
    echo cmd
    doAssert execShellCmd(cmd) == 0

  for opt2 in ["", "--passC:-Og", "--passC:-O", "-d:danger"]:
    echo "\nopt2: ", opt2
    fun fmt"-d:case_normal {opt2}"
    fun fmt"-d:case_inline {opt2}"
    fun fmt"-d:case_noinline {opt2}"
    fun fmt"-d:case_template {opt2}"
    # fun fmt"-d:case_alwaysinline {opt2}"

performance numbers for debug builds

as you can see:

• with --passC:-O, {.inline.}, {..} and template have all the same performance, while {.noinline.} is predictable much slower in this example
• with --passC:-Og, template is the only thing that actually inlines, but if we change __attribute__((__always_inline__)), {.inline.}
• without --passC:-O, {.inline.}, {..} and {.noinline.} have all the same (bad) performance, while template has less bad performance
• the only difference we'd make with __attribute__((__always_inline__)) instead of inline in N_INLINE in nimbase.h would be for {.inline.} in combination with --passC:-Og, which inlines only for __attribute__((__always_inline__))

opt2:
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_normal  /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
3.428681
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_inline  /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
3.402358
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_noinline  /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
3.359203
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_template  /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
1.759311

opt2: --passC:-Og
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_normal --passC:-Og /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
0.979641
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_inline --passC:-Og /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
1.172885
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_noinline --passC:-Og /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
0.964913
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_template --passC:-Og /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
9.999999999992654e-07

opt2: --passC:-O
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_normal --passC:-O /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
9.999999999992654e-07
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_inline --passC:-O /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
1.000000000000133e-06
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_noinline --passC:-O /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
0.978355
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_template --passC:-O /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
2.000000000000265e-06

opt2: -d:danger
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_normal -d:danger /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
1.000000000000133e-06
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_inline -d:danger /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
1.000000000000133e-06
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_noinline -d:danger /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
0.940134
/Users/timothee/git_clone/nim/Nim_prs/bin/nim.devel c -r --skipParentCfg --skipUserCfg --hints:off -d:case5 --stacktrace:off -f -d:case_template -d:danger /Users/timothee/git_clone/nim/timn/tests/nim/all/t10303.nim
2.000000000000265e-06

• none of this affects the validity of [RFC] --stacktrace:noinline: up to 3X speedups with typically same stacktraces Nim#13582, since [RFC] --stacktrace:noinline: up to 3X speedups with typically same stacktraces Nim#13582 is about improving speed of code with stacktraces even for optimized builds (it improves speed in all optimization modes)

links

• https://indico.cern.ch/event/386232/sessions/159923/attachments/771039/1057534/always_inline_performance.pdf
• https://aras-p.info/blog/2017/10/09/Forced-Inlining-Might-Be-Slow/

[Araq]

It's not about speed really, .inline should inline. If it results in slower code than we misapplied the .inline pragma. It's about being more predictable.

[timotheecour]

It's not about speed really, .inline should inline. If it results in slower code than we misapplied the .inline pragma. It's about being more predictable.

this would be bad for performance. It's not as simple as that; source code for a proc declaration has very limited context to decide on whether to inline, unlike C compiler which has much more relevant context (compile flags, params resolved to concrete types, candidate insertion inside a inlining context); hence those articles I linked in previous post which show that force_inline may be slower than inline.

In fact, the same inline proc can end up being inlined or not in different translation units (perhaps even within 1 translation unit) depending on context, and that's a good thing for performance.

I actually started with a branch (which i decided not to submit as PR) where I introduced another pragma {.alwaysInline.} with the semantics:

• nim {.inline.} => C inline (same semantics, it's just a hint; compiler is free to ignore depending on context, flags)
• nim {.alwaysInline.} => C attribute((always_inline))`` (forces inline except in specific conditions eg recursive calls etc)

but I realized it's really not the main thing at play here, the main thing was passing --opt:speed (or --passC:-O or even --passC:-Og) made all the difference.

Speed should be the main criterion here, correctness is already guaranteed by calling convention regardless of whether it ends up being inlined or not (eg, there won't be link or multiple definition errors regardless of whether it gets inlined or not).

[Araq]

I have nothing to add really. inline should inline, period. It's a way to override the compiler's optimizer by saying "I do know better, trust me". There are no other reasons for inline's existance.