10-70x faster $float, roundtrip + correct rounding via dragonbox

changelog.md

@@ -72,6 +72,10 @@
 
 - `json` and `jsonutils` now serialize NaN, Inf, -Inf as strings, so that
   `%[NaN, -Inf]` is the string `["nan","-inf"]` instead of `[nan,-inf]` which was invalid json.
+- `system.addFloat` now uses dragonbox algorithm, which ensures roundtrip guarantee, minimum length,
+  and correct rounding. Use `-d:nimLegacyAddFloat` for a transition period.
+
+- `system/formatfloat` (an internal module) was deprecated; use the new module `std/strfloats` instead.
 
 - `strformat` is now part of `include std/prelude`.
 

compiler/ast.nim

@@ -695,7 +695,7 @@ type
     mInstantiationInfo, mGetTypeInfo, mGetTypeInfoV2,
     mNimvm, mIntDefine, mStrDefine, mBoolDefine, mRunnableExamples,
     mException, mBuiltinType, mSymOwner, mUncheckedArray, mGetImplTransf,
-    mSymIsInstantiationOf, mNodeId, mPrivateAccess
+    mSymIsInstantiationOf, mNodeId, mPrivateAccess, mAddDependency
 
 
 # things that we can evaluate safely at compile time, even if not asked for it:

compiler/builddeps.nim

@@ -0,0 +1,44 @@
+#[
+## TODO
+* this will show `CC: dragonbox`:
+`nim r -f --hint:cc --filenames:abs --processing:filenames --hint:conf nonexistant`
+we could refine the logic to delay compilation until cgen phase instead.
+(see also `tests/osproc/treadlines.nim`)
+* allow some form of reporting so that caller can tell whether a dependency
+  doesn't exist, or was already built, or builds with error, or builds successfully, etc.
+]#
+
+import std/[osproc, os, strutils]
+import msgs, options, ast, lineinfos, extccomp, pathutils
+
+const prefix = "__" # prevent clashing with user files
+
+proc addDependency*(conf: ConfigRef, name: string, info: TLineInfo) =
+  case name
+  of "dragonbox":
+    if name notin conf.dependencies:
+      conf.dependencies.add name
+      # xxx we could also build this under $nimb/build/
+      let dir = conf.getNimcacheDir().string
+      createDir dir
+      let objFile = dir / ("$1nimdragonbox.o" % prefix)
+      if optForceFullMake in conf.globalOptions or not objFile.fileExists:
+        # consider using instead: `getCompilerExe(conf, ...)`
+        when defined(osx):
+          let cppExe = "clang++"
+        else:
+          let cppExe = "g++"
+        when defined(linux):
+          let options = "-fPIE" # avoids: `relocation R_X86_64_32S against `.rodata' can not be used when making a PIE object`
+        else:
+          let options = ""
+        let inputFile = conf.libpath.string / "vendor/drachennest/dragonbox.cc"
+        let cmd = "$# -c -std=c++11 $# -O3 -o $# $#" % [cppExe.quoteShell, options, objFile.quoteShell, inputFile.quoteShell]
+        # xxx use md5 hash to recompile if needed
+        writePrettyCmdsStderr displayProgressCC(conf, inputFile, cmd)
+        let (outp, status) = execCmdEx(cmd)
+        if status != 0:
+          localError(conf, info, "building '$#' failed: cmd: $#\noutput:\n$#" % [name, cmd, outp])
+      conf.addExternalFileToLink(objFile.AbsoluteFile)
+  else:
+    localError(conf, info, "expected: 'dragonbox', got: '$1'" % name)

compiler/condsyms.nim

@@ -134,3 +134,4 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimHasUnifiedTuple")
   defineSymbol("nimHasIterable")
   defineSymbol("nimHasTypeofVoid")
+  defineSymbol("nimHasDragonbox")

compiler/extccomp.nim

@@ -281,7 +281,7 @@ const
 
   hExt* = ".h"
 
-template writePrettyCmdsStderr(cmd) =
+template writePrettyCmdsStderr*(cmd) =
   if cmd.len > 0:
     flushDot(conf)
     stderr.writeLine(cmd)
@@ -835,7 +835,7 @@ proc hcrLinkTargetName(conf: ConfigRef, objFile: string, isMain = false): Absolu
                    else: platform.OS[conf.target.targetOS].dllFrmt % basename
   result = conf.getNimcacheDir / RelativeFile(targetName)
 
-proc displayProgressCC(conf: ConfigRef, path, compileCmd: string): string =
+proc displayProgressCC*(conf: ConfigRef, path, compileCmd: string): string =
   if conf.hasHint(hintCC):
     if optListCmd in conf.globalOptions or conf.verbosity > 1:
       result = MsgKindToStr[hintCC] % (demanglePackageName(path.splitFile.name) & ": " & compileCmd)

compiler/ic/replayer.nim

@@ -12,7 +12,7 @@
 ## support.
 
 import ".." / [ast, modulegraphs, trees, extccomp, btrees,
-  msgs, lineinfos, pathutils, options, cgmeth]
+  msgs, lineinfos, pathutils, options, cgmeth, builddeps]
 
 import tables
 
@@ -32,6 +32,7 @@ proc replayStateChanges*(module: PSym; g: ModuleGraph) =
       of "hint": message(g.config, n.info, hintUser, n[1].strVal)
       of "warning": message(g.config, n.info, warnUser, n[1].strVal)
       of "error": localError(g.config, n.info, errUser, n[1].strVal)
+      of "addDependency": addDependency(g.config, n[1].strVal, n.info)
       of "compile":
         internalAssert g.config, n.len == 4 and n[2].kind == nkStrLit
         let cname = AbsoluteFile n[1].strVal

compiler/options.nim

@@ -359,6 +359,7 @@ type
 
     externalToLink*: seq[string]  # files to link in addition to the file
                                   # we compiled (*)
+    dependencies*: seq[string]     # dependencies
     linkOptionsCmd*: string
     compileOptionsCmd*: seq[string]
     linkOptions*: string          # (*)
@@ -398,7 +399,7 @@ proc setNote*(conf: ConfigRef, note: TNoteKind, enabled = true) =
 proc hasHint*(conf: ConfigRef, note: TNoteKind): bool =
   # ternary states instead of binary states would simplify logic
   if optHints notin conf.options: false
-  elif note in {hintConf, hintProcessing}:
+  elif note in {hintConf, hintProcessing, hintCC}:
     # could add here other special notes like hintSource
     # these notes apply globally.
     note in conf.mainPackageNotes

compiler/pragmas.nim

@@ -96,12 +96,6 @@ proc getPragmaVal*(procAst: PNode; name: TSpecialWord): PNode =
 proc pragma*(c: PContext, sym: PSym, n: PNode, validPragmas: TSpecialWords;
             isStatement: bool = false)
 
-proc recordPragma(c: PContext; n: PNode; args: varargs[string]) =
-  var recorded = newNodeI(nkReplayAction, n.info)
-  for i in 0..args.high:
-    recorded.add newStrNode(args[i], n.info)
-  addPragmaComputation(c, recorded)
-
 const
   errStringLiteralExpected = "string literal expected"
   errIntLiteralExpected = "integer literal expected"

compiler/semdata.nim

@@ -332,6 +332,13 @@ proc addPragmaComputation*(c: PContext; n: PNode) =
   if c.config.symbolFiles != disabledSf:
     addPragmaComputation(c.encoder, c.packedRepr, n)
 
+proc recordPragma*(c: PContext; n: PNode; args: varargs[string]) =
+  # xxx rename to `recordAction`, since it can record things other than pragmas
+  var recorded = newNodeI(nkReplayAction, n.info)
+  for i in 0..args.high:
+    recorded.add newStrNode(args[i], n.info)
+  addPragmaComputation(c, recorded)
+
 proc inclSym(sq: var seq[PSym], s: PSym): bool =
   for i in 0..<sq.len:
     if sq[i].id == s.id: return false

compiler/semmagic.nim

@@ -9,7 +9,7 @@
 
 # This include file implements the semantic checking for magics.
 # included from sem.nim
-
+from builddeps import addDependency
 proc semAddrArg(c: PContext; n: PNode; isUnsafeAddr = false): PNode =
   let x = semExprWithType(c, n)
   if x.kind == nkSym:
@@ -573,6 +573,13 @@ proc magicsAfterOverloadResolution(c: PContext, n: PNode,
     if n[1].typ.skipTypes(abstractInst).kind in {tyUInt..tyUInt64}:
       n[0].sym.magic = mSubU
     result = n
+  of mAddDependency:
+    let x = c.semConstExpr(c, n[1])
+    case x.kind
+    of nkStrKinds: addDependency(c.config, x.strVal, n.info)
+    else: localError(c.config, n.info, "cannot evaluate at compile time")
+    recordPragma(c, n, "addDependency", x.strVal)
+    result = newNodeIT(nkEmpty, n.info, getSysType(c.graph, n.info, tyVoid))
   of mPrivateAccess:
     var t = n[1].typ[0]
     if t.kind in {tyRef, tyPtr}: t = t[0]

compiler/vmhooks.nim

@@ -36,30 +36,32 @@ proc setResult*(a: VmArgs; v: seq[string]) =
   for x in v: n.add newStrNode(nkStrLit, x)
   a.slots[a.ra].node = n
 
-template getX(k, field) {.dirty.} =
+template getReg(a, i): untyped =
   doAssert i < a.rc-1
-  doAssert a.slots[i+a.rb+1].kind == k
-  result = a.slots[i+a.rb+1].field
+  a.slots[i+a.rb+1].unsafeAddr
+
+template getX(k, field): untyped {.dirty.} =
+  let p = getReg(a, i)
+  doAssert p.kind == k, $p.kind
+  p.field
 
 proc numArgs*(a: VmArgs): int =
   result = a.rc-1
 
 proc getInt*(a: VmArgs; i: Natural): BiggestInt = getX(rkInt, intVal)
 proc getBool*(a: VmArgs; i: Natural): bool = getInt(a, i) != 0
 proc getFloat*(a: VmArgs; i: Natural): BiggestFloat = getX(rkFloat, floatVal)
-proc getString*(a: VmArgs; i: Natural): string =
-  doAssert i < a.rc-1
-  doAssert a.slots[i+a.rb+1].kind == rkNode
-  result = a.slots[i+a.rb+1].node.strVal
-
-proc getNode*(a: VmArgs; i: Natural): PNode =
-  doAssert i < a.rc-1
-  doAssert a.slots[i+a.rb+1].kind == rkNode
-  result = a.slots[i+a.rb+1].node
+proc getNode*(a: VmArgs; i: Natural): PNode = getX(rkNode, node)
+proc getString*(a: VmArgs; i: Natural): string = getX(rkNode, node).strVal
+proc getVar*(a: VmArgs; i: Natural): PNode =
+  let p = getReg(a, i)
+  # depending on whether we come from top-level or proc scope, we need to consider 2 cases
+  case p.kind
+  of rkRegisterAddr: result = p.regAddr.node
+  of rkNodeAddr: result = p.nodeAddr[]
+  else: doAssert false, $p.kind
 
 proc getNodeAddr*(a: VmArgs; i: Natural): PNode =
-  doAssert i < a.rc-1
-  doAssert a.slots[i+a.rb+1].kind == rkNodeAddr
-  let nodeAddr = a.slots[i+a.rb+1].nodeAddr
+  let nodeAddr = getX(rkNodeAddr, nodeAddr)
   doAssert nodeAddr != nil
   result = nodeAddr[]

compiler/vmops.nim

@@ -234,18 +234,19 @@ proc registerAdditionalOps*(c: PCtx) =
   registerCallback c, "stdlib.os.getCurrentCompilerExe", proc (a: VmArgs) {.nimcall.} =
     setResult(a, getAppFilename())
 
+  proc stackTrace2(msg: string, n: PNode) =
+    stackTrace(c, PStackFrame(prc: c.prc.sym, comesFrom: 0, next: nil), c.exceptionInstr, msg, n.info)
+
   registerCallback c, "stdlib.macros.symBodyHash", proc (a: VmArgs) =
     let n = getNode(a, 0)
     if n.kind != nkSym:
-      stackTrace(c, PStackFrame(prc: c.prc.sym, comesFrom: 0, next: nil), c.exceptionInstr,
-                  "symBodyHash() requires a symbol. '" & $n & "' is of kind '" & $n.kind & "'", n.info)
+      stackTrace2("symBodyHash() requires a symbol. '" & $n & "' is of kind '" & $n.kind & "'", n)
     setResult(a, $symBodyDigest(c.graph, n.sym))
 
   registerCallback c, "stdlib.macros.isExported", proc(a: VmArgs) =
     let n = getNode(a, 0)
     if n.kind != nkSym:
-      stackTrace(c, PStackFrame(prc: c.prc.sym, comesFrom: 0, next: nil), c.exceptionInstr,
-                  "isExported() requires a symbol. '" & $n & "' is of kind '" & $n.kind & "'", n.info)
+      stackTrace2("isExported() requires a symbol. '" & $n & "' is of kind '" & $n.kind & "'", n)
     setResult(a, sfExported in n.sym.flags)
 
   proc hashVmImpl(a: VmArgs) =
@@ -320,3 +321,8 @@ proc registerAdditionalOps*(c: PCtx) =
   registerCallback c, "stdlib.typetraits.hasClosureImpl", proc (a: VmArgs) =
     let fn = getNode(a, 0)
     setResult(a, fn.kind == nkClosure or (fn.typ != nil and fn.typ.callConv == ccClosure))
+
+  registerCallback c, "stdlib.strfloats.addFloat", proc(a: VmArgs) =
+    let p = a.getVar(0)
+    let x = a.getFloat(1)
+    addFloat(p.strVal, x)

lib/std/private/deputils.nim

@@ -0,0 +1,8 @@
+##[
+Experimental, subject to change
+]##
+
+proc addDependency*(name: string) {.magic: "AddDependency".} =
+  ## Adds a dependency on `name`; currently only `dragonbox` is supported.
+  # a pragma would be possible but cause more boilerplate, and also would be less flexible
+  # in case we want to also return something about the depenedncy

lib/std/strfloats.nim

@@ -0,0 +1,120 @@
+#
+#
+#            Nim's Runtime Library
+#        (c) Copyright 2019 Nim contributors
+#
+#    See the file "copying.txt", included in this
+#    distribution, for details about the copyright.
+#
+
+#[
+## TODO
+* support more efficient dragonbox API for float32
+* support more rounding modes and other options, see https://github.com/jk-jeon/dragonbox
+]#
+
+
+const useDragonbox = defined(nimHasDragonbox) and not defined(nimLegacyAddFloat) and not defined(nimscript)
+
+const dragonboxBufLen = 64
+  # see justification for 64 here: https://github.com/abolz/Drachennest/blob/master/src/dragonbox.h
+
+const strFloatBufLen* = dragonboxBufLen
+
+when not defined(nimscript): # eg for `tests/stdlib/tlwip.nim`
+  from system/memory import nimCopyMem
+
+proc addCharsN*(result: var string, buf: ptr char; n: int) = # PRTEMP MOVE
+  let oldLen = result.len
+  result.setLen oldLen + n
+  when declared(nimCopyMem):
+    nimCopyMem(result[oldLen].addr, buf, n)
+  else:
+    doAssert false
+
+proc addCstring(result: var array[strFloatBufLen, char], buf: openArray[char]) {.inline.} =
+  for i in 0..<buf.len:
+    result[i] = buf[i]
+
+proc toStringSprintf*(buf: var array[strFloatBufLen, char]; value: BiggestFloat): int =
+  ## This is the old implementation to format floats in the Nim
+  ## programming language.
+  ##
+  ## * `buf` - A buffer to write into. The buffer does not need to be initialized.
+  proc c_sprintf(buf, frmt: cstring): cint {.header: "<stdio.h>", importc: "sprintf", varargs, noSideEffect.}
+  let n: int = c_sprintf(addr buf, "%.16g", value)
+  var hasDot = false
+  for i in 0..<n:
+    # compensate for some `LC_NUMERIC` values, refs https://linux.die.net/man/3/sprintf
+    # xxx incorrect in this edge case: "1.234.567,89" in the da_DK locale.
+    if buf[i] == ',':
+      buf[i] = '.'
+      hasDot = true
+    elif buf[i] in {'a'..'z', 'A'..'Z', '.'}:
+      hasDot = true
+  if not hasDot: # 12 => 12.0
+    buf[n] = '.'
+    buf[n+1] = '0'
+    result = n + 2
+  else:
+    result = n
+  # On Windows nice numbers like '1.#INF', '-1.#INF' or '1.#NAN' or 'nan(ind)'
+  # of '-1.#IND' are produced.
+  # We want to get rid of these here:
+  if buf[n-1] in {'n', 'N', 'D', 'd', ')'}:
+    addCstring(buf, "nan")
+    result = 3
+  elif buf[n-1] == 'F':
+    if buf[0] == '-':
+      addCstring(buf, "-inf")
+      result = 4
+    else:
+      addCstring(buf, "inf")
+      result = 3
+
+when useDragonbox:
+  import private/deputils
+  addDependency("dragonbox")
+  proc dragonboxToString(buf: ptr char, value: cdouble): ptr char {.importc: "nim_dragonbox_Dtoa".}
+
+  proc toStringDragonbox*(buf: var array[strFloatBufLen, char]; value: BiggestFloat): int {.inline.} =
+    let first = buf[0].addr
+    let ret = dragonboxToString(first, value)
+    result = cast[int](ret) - cast[int](first)
+    if buf[result-1] in {'f', 'n'}: # inf, -inf, nan
+      return result
+    for i in 0..<result: # 12 => 12.0
+      if buf[i] in {'.', 'e'}: # if needed, we could make 1e2 print as 1.0e2
+        return result
+    buf[result] = '.'
+    buf[result+1] = '0'
+    result += 2
+
+  template toString*(buf: var array[strFloatBufLen, char]; value: BiggestFloat): int =
+    toStringDragonbox(buf, value)
+else:
+  template toString*(buf: var array[strFloatBufLen, char]; value: BiggestFloat): int =
+    toStringSprintf(buf, value)
+
+proc addFloat*(result: var string; x: float) =
+  ## Converts `x` to its string representation and appends it to `result`.
+  ##
+  ## The algorithm is implementation defined, but currently uses dragonbox algorithm,
+  ## which ensures roundtrip guarantee, minimum length, and correct rounding.
+  runnableExamples:
+    var a = "prefix:"
+    a.addFloat(0.1)
+    assert a == "prefix:0.1"
+
+    a.setLen 0
+    var b = 0.1
+    var c = b + 0.2
+    a.addFloat(c)
+    assert a == "0.30000000000000004"
+    assert c != 0.3 # indeed, binary representation is not exact
+  when nimvm: # also a vmops, after bootstrap
+    result.add $x
+  else:
+    var buf {.noinit.}: array[strFloatBufLen, char]
+    let n = toString(buf, x)
+    result.addCharsN(buf[0].addr, n)