adding/removing pragmas like {.inline.} causes breaking changes
#18220
Comments
|
Ref: d32ab61 /cc @timotheecour |
within-module inlining works without {.inline.}, but cross-module inlininig requires {.inline.} can you try to minimize the issue (ideally with a self-contained module with no imports) ? |
|
I began trying to minimize it by first adding what seemed to be the relevant sections to alpm.nim, the bindings to libalpm, but it fails on toHashSet with any version of Nim: I also tried adding the original hash proc from hashes.nim in common.nim as an overload, and various variations, none worked. Perhaps the compiler needs the flexibility to not inline this section, because there are several possibilities what the item being operated on can take? Originally the inline was not there, the rest of the proc body is the same as before when not dealing with closures, so removing it would be restoring the original operation. IDK, it's a complicated section of code. |
|
after a painful reduction (until nimdustmite lands, refs timotheecour#703), here's a reduction that involves only stdlib modules: import sets, options, hashes
template hash*[T](optz: Option[T]): int =
optz.map(hash).get(0)
type
VersionConstraint* = tuple[
version: string,
impliedVersion: bool
]
PackageReference* = tuple[
name: string,
constraint: Option[VersionConstraint]
]
proc main =
var b: seq[PackageReference]
let c = toHashSet(b)remove {.inline.} in EDITfurther simplified: when true:
import options, hashes
proc hash*[T](optz: Option[T]): int =
map(optz, hash).get(0)
type Foo = object
foo: int
var b: Option[Foo]
let ci = hash(b)note, this would fail regardless of {.inline.}: var b: Option[int]
let ci = hash(b)EDITdown to 1 single import: when true:
import hashes
type Bar[T] = object
proc map2*[T, R](self: Bar[T], fn: proc (input: T): R): Bar[R] = discard
proc hash*[T](optz: Bar[T]): int =
let b = map2(optz, hash)
123
type Foo = object
var b: Bar[Foo]
let ci = hash(b)EDITfurther: when true:
# repro
import hashes
type Bar[T] = object
proc map2*[T](self: Bar[T], fn: proc (a: T): int) = discard
proc hash*[T](optz: Bar[T]): int =
map2(optz, hash)
123
type Foo = object
var b: Bar[Foo]
let ci = hash(b)notefurther reduction is still needed to remove stdlib modules, help welcome |
|
here's the final reduction: when true:
proc map2(a: proc(): int) = discard
# proc map2(a: proc(): int {.inline.}) = discard # uncommenting would make it work
proc fn1(): int = 1
proc fn2(): int {.inline.} = 2
map2(fn1) # ok
map2(fn2) # errorconclusion: # in options.nim
proc map*[T, R](self: Option[T], callback: proc (input: T): R): Option[R] {.inline.} =is IMO un-necessarily restrictive and that's what should be fixed instead, either in the signature or in the compiler to ignore the options.map should not care whether the callback is inline or not, this prevents valid optimizations;
The proper fix though is neither patching the compiler to ignore inline/noinline pragmas nor to use old/new concepts (refs #18225). The proper fix is examplesimport std/options
proc fn(a: int): int = a
proc fn(a: string): string {.noinline.} = a
template fn(a: float): float = a
echo some(1).map(fn) # ok
echo some("a").map(fn) # Error: type mismatch
echo some(1.5).map(fn) # Error: internal error: expr(skTemplate); unknown symbol |
|
Brilliant, thank you for looking at this. Hopefully it might get fixed before 1.6 is released. It would have taken me a lot longer to reduce this even partially I think, dustmite would certainly be helpful so long as it comes with a good manual and I could figure out how to use it! |
|
The inline pragma was wrong for objects and tuples anyway as you don't know how many object fields would be expanded. |
it doesn't matter! the C optimizer is free to inline or not inline a particular generic instantiation marked with Without here's a simplified example showing a 8X speedup simply by adding {.inline.}: # main.nim:
import std/times
import t12382b
proc main()=
let n = 100_000_0000
var c = 0
let t = cpuTime()
for i in 0..<n:
when defined(case1): c += fn1(i, c)
when defined(case2): c += fn2(i, c)
let t2 = cpuTime()
echo (t2 - t, c)
main()
# t12382b.nim:
proc fn1*[T](a: T, b: T): T = a + b
proc fn2*[T](a: T, b: T): T {.inline.} = a + b
if false:
# having these instantiated here or in another module prevents
# cross-module inlining forr fn1 which is not {.inline.}
let a1 = fn1(1, 2)
let a2 = fn2(1, 2) |
|
@Araq as I predicted (see previous comment), the hotfix #18227 for closing this issue introduces a performance regression, so the problem should be fixed differently. It affects all code involving hashes, which can be particularly performance sensitive. Surely there must be a better way that neither breaks code nor makes hashes slower than necessary (it's a general problem btw; with the same logic as #18227, we'd end up having to remove {.inline.} from all generics just because someone might be using it, not good). before #18227: => 1.6X slower # main.nim:
when true:
import std/times
import t12382b
import hashes
proc main()=
let n = 100_000_0000
var c = 0
let t = cpuTime()
for i in 0..<n:
c += hash (i, i)
let t2 = cpuTime()
echo (t2 - t, c)
main()
# t12382b.nim:
when true:
import std/hashes
let a2 = hash (1, 2) # instantiates the generic first in this module |
|
A performance regression that is caught by link time optimizations is not nearly as bad as a regression which prevented valid code from compiling. |
with this logic, we'd have to consider adding/removing proc callFun[A, B](a: A, fn: proc(a2: A): B): B = fn(a) # analog to options.map from this issue
import std/strutils
echo callFun("ab", validIdentifier)(or anything similar that has a proc argument, doesn't have to be generic)
(tested on 1.5.1 0a4858d) There has got to be a better way. linkssee also nim-lang/RFCs#198 (comment)
|
timotheecour
changed the title
{.inline.} causes breaking changes
# main.nim:
when true:
import std/times
include t12382b
import hashes
proc main()=
let n = 100_000_0000
var c = 0
let t = cpuTime()
for i in 0..<n:
c += hash (i, i)
let t2 = cpuTime()
echo (t2 - t, c)
echo a2
main()$ nim r --passC:-Ofast -d:case2 -d:danger main |
I cannot reproduce this, this probably just reflects measurement noise (take the best-of-5 for example). https://linux.die.net/man/1/gcc says:
and
|
Adding this to hashes.nim and calling inlinehash instead of hash in both files works. I suppose ideally you could have inlinehash and notinlinehash, hash would be something like |
The pre-1.0 Nim had a But anyway, "with this logic" implies that I intend to always undo inline changes because it would break code, no, I agree with you that the language could handle this better instead. I merely applied a hotfix until the better solution emerges. |
IIUC procvar avoided some of the issues of #18220 (allowing one to add inline pragma, addoptional params etc, without breaking related APIs), but it required one to annotate the proc you want to pass as param with {.procvar.} (eg #2487), forcing each API to make that decision; in particular, this wouldn't help with 3rd party procs that didn't have that annotation. It possibly also involved a performance penalty via an extra indirection but I'm not sure, please calrify. So {.procvar.} didn't solve the general of APIs not marked with {.procvar.}, nor did it help with symbol forwarding (eg iterators, templates, generics). See also #18241 |
|
|
|
this is related to #8303 ( the proper fix (with no runtime cost) is |
|
Would another fix be for the compiler to automatically generate procvar and closure thunks for procedures of other calling conventions? |
Ya I was thinking exactly on those lines as well, see nim-lang/RFCs#396 |
…ang#18220 (nim-lang#18241) * getType now works with tyInferred (concepts); refs nim-lang#18220 * avoid cast * add more docs
zqqw/pakku#12
d32ab61
https://github.com/nim-lang/Nim/blob/devel/lib/pure/hashes.nim
Building the latest nim-git today, all that is needed to fix this error is delete the {.inline.} from line 512 in lib/pure/hashes.nim
Does it need to be inline? (gcc will often inline things itself when it optimizes the code anyway.) If it does, have you any suggestions how to avoid the build error in Pakku, as I've tried various things and not found a solution yet. toHashSet is being run on an opaque object (C struct) from libalpm, so perhaps it isn't very amenable to being inlined at compile time, I'm not sure of the exact reason for the failure.
links
mapshouldn't care whether proc arg iscdecl#8303alias)alias Callable[R, T]))The text was updated successfully, but these errors were encountered: