mirror of
https://github.com/nim-lang/Nim.git
synced 2026-08-04 14:38:38 +00:00
bugfix
This commit is contained in:
@@ -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
|
||||
|
||||
69
doc/ic.md
69
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
|
||||
========================
|
||||
|
||||
|
||||
Reference in New Issue
Block a user