From 54dffbd09ca7a518f654db0b6e4cc9b415b22a27 Mon Sep 17 00:00:00 2001 From: Araq Date: Thu, 2 Jul 2026 20:33:51 +0200 Subject: [PATCH] bugfix --- compiler/nifbackend.nim | 38 ++++++++++++----------- doc/ic.md | 69 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 18 deletions(-) diff --git a/compiler/nifbackend.nim b/compiler/nifbackend.nim index 0dc218fc07..b77f3cdbd3 100644 --- a/compiler/nifbackend.nim +++ b/compiler/nifbackend.nim @@ -748,31 +748,33 @@ proc generateEmitStage(g: ModuleGraph; mainFileIdx: FileIndex) = let mainSuffix = cachedModuleSuffix(g.config, mainFileIdx) let targetIsMain = g.config.icBackendModule.len == 0 or g.config.icBackendModule == mainSuffix - var modules: seq[PrecompiledModule] - var precompSys: PrecompiledModule - var target: PrecompiledModule - if targetIsMain: - var nifFiles: seq[string] - (modules, precompSys, nifFiles) = loadBackendModules(g, mainFileIdx) - if modules.len == 0: - rawMessage(g.config, errGenerated, - "Cannot load NIF file for main module: " & toFullPath(g.config, mainFileIdx)) - return - target = findTargetModule(g, modules, precompSys, g.config.icBackendModule) - else: - (modules, precompSys, target) = loadDepClosure(g, g.config.icBackendModule) - if target.module == nil: + # emit renders a module's final `.c` PURELY from its own `.c.nif` and the merge + # decision (see `renderCFromArtifact` — text filtering, no AST is touched). It + # used to load the target's whole transitive import closure as BModules solely + # to reach `getCFile(bmod)` for the output path. Under the fire-all-every-edit + # merge barrier (every `emit` re-fires whenever `merge` bumps the decision's + # mtime — deliberate insurance so a decision change re-renders all `.c` + # consistently) that per-process `loadDepClosure` was the bulk of a warm + # rebuild's cost: 240 processes each re-parsing a module closure only to filter + # a handful of `.c.nif`s whose bytes are usually unchanged. Derive the `.c` + # path directly instead — the SAME pure computation `deps.nim.backendCFile` + # uses to DECLARE this stage's output (`getCFile` == that formula) — so an emit + # process loads nothing and the fire-all costs process-startup, not a graph load. + let cfilename = + if targetIsMain: AbsoluteFile toFullPath(g.config, mainFileIdx) + else: AbsoluteFile g.config.icBackendModule + let cfile = changeFileExt(completeCfilePath(g.config, + mangleModuleName(g.config, cfilename).AbsoluteFile), ".nim.c").string + let artifact = cfile & ".nif" + if not fileExists(artifact): rawMessage(g.config, errGenerated, - "per-module emit: module not found for suffix: " & g.config.icBackendModule) + "per-module emit: missing .c.nif artifact for suffix: " & g.config.icBackendModule) return let decision = readMergeDecision(getNimcacheDir(g.config).string / MergeDecisionFile) if decision.broken: rawMessage(g.config, errGenerated, "per-module emit: missing or unparsable merge decision " & MergeDecisionFile) return - let bmod = BModuleList(g.backend).mods[target.module.position] - let cfile = getCFile(bmod).string - let artifact = cfile & ".nif" var dropped = 0 let code = renderCFromArtifact(artifact, decision, extractFilename(artifact), dropped) # Write the `.c` content-stably. `merge` re-runs on any edit and bumps the diff --git a/doc/ic.md b/doc/ic.md index 278ad8a858..a2e6cd0592 100644 --- a/doc/ic.md +++ b/doc/ic.md @@ -288,6 +288,75 @@ Validation bar (held on every change): `koch bootic` must reach its byte-identic fixed point, and binary size must not regress (DCE parity), across the external-package CI set. +Further possible improvements +============================= + +A warm-edit profiling pass (2026-07-02, self-compiling the compiler into a +dedicated `--nimcache`, editing one private proc body — `internalErrorImpl` — in +the hub module `compiler/msgs.nim`) surfaced where a **hub-module** warm rebuild +actually spends its time. The result refines the "a body-only edit re-fires one +module" claim above: that holds for the *backend*, but the *frontend* can still +cascade. + +Measured: no-op `0.05s`; hub body edit `~15s`, split **~13s frontend / ~1.6s +backend**. Editing a body in a leaf (few importers) is fast; editing a body in a +widely-imported module is not, and the cost is almost entirely frontend re-sem. + +- **Frontend over-invalidation (the dominant hub-edit cost).** Editing *any* body + in a module — even a private routine that is only ever *called* — flips that + module's whole-module **impl cookie** (`writeImplCookie` hashes the entire + serialized module). Every module carrying a **NeedsImpl** edge on it then + re-sems, even though the symbol it actually consumed is unchanged (e.g. a + dependent that expanded the `internalError` *template* needs the template body, + which is untouched; it does **not** need `internalErrorImpl`'s body). In the + msgs edit this re-fires **57** `nim m` processes. A `.s.bif` mtime diff *hides* + this — `.s.bif` is content-stable, so a re-semmed-but-identical module keeps its + timestamp; count actual `nim m` PIDs to see the fan-out. + + The precise fix is **per-symbol NeedsImpl gating**: record which *symbols'* + bodies a dependent consumed (the recording site `modulegraphs.recordIcImplDep` + already receives the `PSym`; it currently coarsens to `module(s.itemId)`) and + gate the dependent + on only those. The obstacle is that `nifmake` gates on file mtimes, so + per-symbol granularity needs either many cookie files or a bucketing scheme, and + "which bodies are compile-time-consumable" is entangled with `getImpl` and the + CT call graph (a macro that runs a private helper at CT *does* consume its body). + A conservative narrowing — keep template/generic/macro/`sfCompileTime` bodies + (plus `getImpl` targets) in the impl cookie but drop ordinary runtime routine + bodies — captures the common "edit a private implementation proc" case, at the + cost of proving the exclusion is complete. + +- **Serial re-sem chains.** The 57 re-sems above run essentially **one at a time** + despite `--parallel`, because the core modules they belong to form a deep import + *chain* and `nifmake`'s depth-barriered scheduler runs one depth level at a time + (≈1 node per level). This is independent of the invalidation problem: even + perfect per-symbol precision leaves a serial tail whenever the re-sem set is a + chain. Mitigations live in the scheduler (content-stability already stops the + cascade at one level, but does not flatten the chain). + +- **Emit stage need not load the module graph (done).** `generateEmitStage` used + to `loadDepClosure`/`loadBackendModules` — materializing a module's whole + transitive import closure as `BModule`s — solely to reach `getCFile(bmod)` for + the output path. `renderCFromArtifact` is pure text filtering over the `.c.nif` + plus the merge decision; it needs none of that. Deriving the `.c` path directly + from the suffix (the same pure computation `deps.backendCFile` uses to *declare* + the stage's output) lets an `emit` process load nothing. Under the + fire-all-every-edit `emit` barrier (see below) this halved backend CPU + (user-time `51s → 24s` on the msgs edit); wall-clock barely moved because the + frontend dominates, but the reduced CPU/RAM contention matters when an editor is + running alongside. `koch ic` stays byte-identical. + +- **Do NOT make the merge decision content-stable.** A tempting frontend to the + above: `emit` re-fires for *every* live module whenever `merge` rewrites the + decision file's mtime (deliberate — a decision change must re-render every `.c` + consistently). Writing the decision `OnlyIfChanged` (with a stamp output so the + `merge` rule is not perpetually stale) makes a warm no-op instant, but a real + edit then fires `emit` only for the modules whose `.c.nif` changed — and that + produces **multiple-definition link errors** even when the decision is + byte-identical. Fire-all `emit` is a correctness invariant, not just insurance + (see the comment at `generateEmitStage`): partial `emit` leaves inconsistent + ownership across the `.c` set. This path was tried and reverted; do not retry. + Code, logic & debugging ========================