feat: CBOR-free scalar fast path for abi=c procs (#110)

Author: gmelodie

ffi/codegen/meta.nim

@@ -33,9 +33,9 @@ type
     abiFormat*: ABIFormat # wire format for this interaction (default Cbor)
     scalarFastPath*: bool
       ## True for an `abi = c` proc whose whole signature is scalar (see
-      ## `isScalarOnly`): it dispatches through the CBOR-free scalar fast path
-      ## and is skipped by the foreign-binding generators (no dispatch codegen
-      ## yet — the request rides inline POD args, no `_CWire`, no CBOR).
+      ## `isScalarOnly`): dispatches through the CBOR-free scalar fast path and
+      ## is skipped by the foreign-binding generators (their codegen is a
+      ## follow-up).
 
   FFIFieldMeta* = object
     name*: string # e.g. "delayMs"

ffi/ffi_thread_request.nim

@@ -30,11 +30,14 @@ type FFIThreadRequest* = object
   reqId*: cstring ## Per-proc Req type name used to look up the handler.
   data*: ptr UncheckedArray[byte] ## Owned CBOR-encoded request payload.
   dataLen*: int
+  isScalar*: bool
+    ## Set by `initScalar`: the payload rode inline in `scalarArgs` (no CBOR,
+    ## no `data` buffer). Lets `handleRes` tell a scalar 0-length return (a real
+    ## empty string) from a CBOR "no value".
   scalarArgs*: array[MaxScalarArgs, uint64]
-    ## Scalar-fast-path args inlined in the envelope (one `ffiPackScalar` value
-    ## per slot) so there's no per-call `c_malloc`. A plain array rather than a
-    ## `union` with `data`: costs a fixed 64 bytes per request but keeps the
-    ## `next` link and `deleteRequest` unaliased and branch-free.
+    ## Scalar-fast-path args inlined in the envelope so there's no per-call
+    ## `c_malloc`. A plain array rather than a `union` with `data`: costs a fixed
+    ## 64 bytes per request but keeps `deleteRequest` unaliased and branch-free.
   next*: ptr FFIThreadRequest
     ## Intrusive ingress-queue link (see `ffi_request_queue.nim`). Touched only
     ## under the queue's lock; the request doubles as its own node, so no
@@ -79,6 +82,7 @@ proc allocBaseRequest(
   ret[].reqId = reqId.alloc()
   ret[].data = nil
   ret[].dataLen = 0
+  ret[].isScalar = false
   ret[].next = nil
   ret[].responded = false
   return ret
@@ -169,17 +173,17 @@ proc initScalar*(
     "initScalar: " & $args.len & " scalar args exceed MaxScalarArgs (" & $MaxScalarArgs &
       ")"
   var ret = allocBaseRequest(callback, userData, reqId)
+  ret[].isScalar = true
   for i in 0 ..< args.len:
     ret[].scalarArgs[i] = args[i]
   ret
 
 func ffiScalarRetBytes*[T](x: T): seq[byte] =
-  ## Serializes a scalar handler result into the raw response payload the
-  ## callback carries — no CBOR envelope. A `string`/`cstring` rides as its
-  ## own UTF-8 bytes (like the error path); every other scalar rides as the
-  ## 8-byte native-endian image of `ffiPackScalar(x)`. Note: an empty string
-  ## yields a 0-length payload, which `handleRes` sends as the CBOR-null
-  ## sentinel — the foreign scalar reader (a follow-up) must special-case it.
+  ## Serializes a scalar handler result into the raw response payload — no CBOR
+  ## envelope. A `string`/`cstring` rides as its own UTF-8 bytes (like the error
+  ## path); every other scalar rides as the 8-byte native-endian image of
+  ## `ffiPackScalar(x)`. An empty string yields a 0-length payload (see
+  ## `handleRes`, which delivers it as `""` rather than the CBOR-null sentinel).
   when T is string:
     var b = newSeq[byte](x.len)
     if x.len > 0:
@@ -231,6 +235,15 @@ proc fireCallback*(res: Result[seq[byte], string], request: ptr FFIThreadRequest
         cast[csize_t](bytes.len),
         request[].userData,
       )
+    elif request[].isScalar:
+      # `isScalar` marks a scalar-fast-path request (args rode inline in
+      # `scalarArgs`, no CBOR): its result bytes come from `ffiScalarRetBytes`,
+      # not a CBOR encoder. So a 0-byte return is a real empty string, not
+      # "no value" — hand back a genuine empty buffer, not the CBOR-null sentinel.
+      var empty: byte
+      request[].callback(
+        RET_OK, cast[ptr cchar](addr empty), 0.csize_t, request[].userData
+      )
     else:
       # Always hand the callback a real buffer; CBOR null marks "no value".
       var sentinel = CborNullByte

ffi/internal/ffi_macro.nim

@@ -954,10 +954,9 @@ macro ffi*(args: varargs[untyped]): untyped =
     abiFormat: abiFormat,
   )
 
-  # Does this proc qualify for the CBOR-free scalar fast path? Only `abi = c`
-  # opts in, and only when every wire param + the return is a plain scalar
-  # (see `isScalarOnly`) and the args fit the inline slots. An `abi = c` proc
-  # that isn't scalar-only stays gated — full C-wire dispatch is a follow-up.
+  # Qualifies for the CBOR-free fast path only if `abi = c`, every wire param +
+  # return is scalar (`isScalarOnly`), and the args fit the inline slots. A
+  # non-scalar `abi = c` proc stays gated — full C-wire dispatch is a follow-up.
   let scalarEligible =
     abiFormat == ABIFormat.C and isScalarOnly(procMeta) and
     extraParamNames.len <= MaxScalarArgs
@@ -1821,6 +1820,12 @@ macro genBindings*(
     # export doesn't take the CBOR `(reqCbor, reqCborLen)` shape); drop them so
     # the generators don't emit a broken CBOR caller for them.
     let genProcs = bindableProcs(ffiProcRegistry)
+    for p in ffiProcRegistry:
+      if p.scalarFastPath:
+        hint(
+          "genBindings: skipping scalar-fast-path proc '" & p.procName &
+            "' (no foreign-binding codegen for the scalar shape yet)"
+        )
     case lang
     of "rust":
       generateRustCrate(
@@ -1832,8 +1837,7 @@ macro genBindings*(
       )
     of "c":
       generateCBindings(
-        ffiProcRegistry, ffiTypeRegistry, libName, outputDir, nimSrcRelPath,
-        ffiEventRegistry,
+        genProcs, ffiTypeRegistry, libName, outputDir, nimSrcRelPath, ffiEventRegistry
       )
     of "cddl":
       generateCddlBindings(genProcs, ffiTypeRegistry, libName, outputDir, nimSrcRelPath)

ffi/internal/ffi_scalar.nim

@@ -17,11 +17,10 @@ const scalarPodTypeNames = [
   "int", "int8", "int16", "int32", "int64", "uint", "uint8", "uint16", "uint32",
   "uint64", "byte", "float", "float32", "float64", "bool",
 ]
-  ## Fixed-width POD scalars that fit a single `uint64` slot and survive the
-  ## async hop by value — the payload the scalar fast path inlines into the
-  ## request (no heap copy). `cstring`/`string` are intentionally absent as
-  ## *params*: they are pointers to caller memory the FFI thread reads later,
-  ## so they'd need a copy, defeating the zero-alloc promise.
+  ## Fixed-width POD scalars that fit one `uint64` slot and survive the async
+  ## hop by value. `cstring`/`string` are intentionally absent as *params*:
+  ## they point to caller memory the FFI thread reads after the call returns,
+  ## so passing them inline by value would be unsafe.
 
 func isScalarParamTypeName*(name: string): bool =
   ## A param type eligible for the CBOR-free scalar fast path.
@@ -37,7 +36,7 @@ func isScalarOnly*(p: FFIProcMeta): bool =
   ## True iff `p` is a plain `{.ffi.}` method whose every wire param and return
   ## is scalar — the whole signature crosses without CBOR or `_CWire`. Handles
   ## and raw pointers are excluded (a handle needs a ctx-registry round-trip;
-  ## a pointer never crosses). Pure over the compile-time metadata.
+  ## a pointer never crosses).
   if p.kind != FFIKind.FFI:
     return false
   if p.returnIsPtr or p.returnIsHandle:
@@ -99,7 +98,7 @@ proc buildScalarPath*(
   let retValIdent = genSym(nskLet, "retVal")
   handlerBody.add quote do:
     let `retValIdent` = (await `helperCall`).valueOr:
-      return err($error)
+      return err(error)
     return ok(ffiScalarRetBytes(`retValIdent`))
 
   let seqByteResult = nnkBracketExpr.newTree(

tests/unit/test_c_codegen.nim

@@ -3,9 +3,10 @@
 ## pipeline, no files written) and asserts on the emitted text — the same
 ## approach as test_cddl_codegen.
 
-import std/strutils
+import std/[strutils, sequtils]
 import unittest2
 import ffi/codegen/[meta, c]
+import ffi/internal/ffi_scalar
 
 proc field(n, t: string): FFIFieldMeta =
   FFIFieldMeta(name: n, typeName: t)
@@ -210,6 +211,60 @@ suite "generateCLibHeader: no-event libraries stay lean":
     check "listeners_len" notin header
     check "_add_event_listener" in header # raw ABI symbol is always declared
 
+suite "generateCLibHeader: scalar-fast-path procs are excluded":
+  setup:
+    let procs = @[
+      FFIProcMeta(
+        procName: "calc_create",
+        libName: "calc",
+        kind: FFIKind.CTOR,
+        libTypeName: "Calc",
+        returnTypeName: "Calc",
+      ),
+      FFIProcMeta(
+        procName: "calc_echo",
+        libName: "calc",
+        kind: FFIKind.FFI,
+        libTypeName: "Calc",
+        extraParams: @[param("req", "EchoRequest")],
+        returnTypeName: "EchoResponse",
+      ),
+      FFIProcMeta(
+        procName: "calc_add",
+        libName: "calc",
+        kind: FFIKind.FFI,
+        libTypeName: "Calc",
+        extraParams: @[param("a", "int"), param("b", "int")],
+        returnTypeName: "int",
+        abiFormat: ABIFormat.C,
+        scalarFastPath: true,
+      ),
+    ]
+    let types = @[
+      FFITypeMeta(name: "EchoRequest", fields: @[field("m", "string")]),
+      FFITypeMeta(name: "EchoResponse", fields: @[field("echoed", "string")]),
+    ]
+
+  test "bindableProcs keeps the CBOR procs and drops the scalar one":
+    let kept = bindableProcs(procs)
+    check kept.anyIt(it.procName == "calc_create")
+    check kept.anyIt(it.procName == "calc_echo")
+    check not kept.anyIt(it.procName == "calc_add")
+
+  test "the C header emitted from the bindable set carries no scalar symbol":
+    let header = generateCLibHeader(bindableProcs(procs), types, "calc")
+    check "int calc_echo(void* ctx, FFICallback callback" in header
+    check "int calc_add(" notin header # note: calc_add_event_listener is unrelated
+
+  test "unfiltered, the generator would emit a wrong-ABI CBOR caller for it":
+    # Regression guard for the bug bindableProcs prevents: handed the raw
+    # registry, the C generator declares calc_add with the CBOR
+    # (req_cbor, req_cbor_len) prototype, which mismatches its real inline-arg
+    # export. genBindings must feed the filtered set (see the `of "c":` branch).
+    let header = generateCLibHeader(procs, types, "calc")
+    check "int calc_add(void* ctx, FFICallback callback, void* user_data, " &
+      "const uint8_t* req_cbor, size_t req_cbor_len);" in header
+
 suite "shared headers: prelude and cbor split":
   test "the prelude owns the leaf types and libc/TinyCBOR includes":
     let prelude = generateCPreludeHeader()

tests/unit/test_scalar_fastpath.nim

@@ -61,7 +61,14 @@ proc scalarfast_checked*(
     return err("negative not allowed")
   return ok(n * 2)
 
-## --- C-shape callback harness (mirrors test_ffi_context.nim) ----------------
+proc scalarfast_blank*(
+    lib: ScalarLib
+): Future[Result[string, string]] {.ffi: "abi = c".} =
+  ## Empty-string return — must ride back as a genuine 0-length RET_OK payload,
+  ## not the CBOR-null sentinel.
+  return ok("")
+
+## C-shape callback harness (mirrors test_ffi_context.nim).
 
 type CallbackData = object
   lock: Lock
@@ -122,10 +129,7 @@ proc scalarStr(d: CallbackData): string =
   s
 
 proc callbackErr(d: CallbackData): string =
-  var msg = newString(d.msgLen)
-  if d.msgLen > 0:
-    copyMem(addr msg[0], unsafeAddr d.msg[0], d.msgLen)
-  msg
+  scalarStr(d)
 
 proc encodedPtr(bytes: var seq[byte]): ptr byte =
   if bytes.len == 0:
@@ -183,6 +187,22 @@ suite "scalar fast path — C export shape":
     check d.retCode == RET_OK
     check scalarStr(d) == "scalarfast v1"
 
+  test "empty string return rides back as a real 0-length RET_OK payload":
+    let ctx = makeCtx(0)
+    defer:
+      check ScalarLibFFIPool.destroyFFIContext(ctx).isOk()
+
+    var d: CallbackData
+    initCallbackData(d)
+    defer:
+      deinitCallbackData(d)
+
+    check scalarfast_blank(ctx, testCallback, addr d) == RET_OK
+    waitCallback(d)
+    check d.retCode == RET_OK
+    check d.msgLen == 0 # NOT 1 (would be the 0xf6 sentinel)
+    check scalarStr(d) == ""
+
   test "float param round-trips through the uint64 slot":
     let ctx = makeCtx(0)
     defer:
@@ -250,13 +270,17 @@ suite "scalar fast path — Nim-native shape":
     check bad.isErr()
     check bad.error == "negative not allowed"
 
+    let blank = waitFor scalarfast_blank(ScalarLib(base: 0))
+    check blank.isOk()
+    check blank.value == ""
+
 # `ffiProcRegistry` is a compile-time var, so its assertions run in a static
 # block (mirrors test_abi_format.nim). A scalar-only `abi = c` proc must be
 # flagged, recognised by `isScalarOnly`, and dropped from `bindableProcs`.
 static:
   const scalarNames = [
     "scalarfast_add", "scalarfast_version", "scalarfast_scale", "scalarfast_positive",
-    "scalarfast_checked",
+    "scalarfast_checked", "scalarfast_blank",
   ]
   var seen = 0
   for p in ffiProcRegistry: