From 2605cd32135a7a5a492b384c0ca180e158089563 Mon Sep 17 00:00:00 2001 From: araq Date: Sun, 30 Aug 2026 23:30:30 +0200 Subject: [PATCH] IC: take the module index from the `.bif` instead of rescanning for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `moduleId` threw away the index `bif.load` hands it and rebuilt an equivalent one by walking the module's ENTIRE token stream, allocating a string per `SymbolDef` — once per module, per backend process. That was 909ms of a 10.1s cold `--ic:on` build. Taking the carried index instead: 220ms, and the build drops to 9.4s. `bif.store` already builds that index in one forward traversal at write time (`bif.buildIndex`) and writes it into the file, and `bif.load` reads it back with `pos` already a TOKEN index of the declaration's enclosing tag — the very thing the rescan recomputed. The comment claiming the file's offsets are "meaningless once the file is parsed" was true of the older byte-offset `readEmbeddedIndex`; it stopped being true when `bif` started storing token positions. The two agree BY CONSTRUCTION, and the reason is worth stating because "the file has an index" would not be enough on its own: the writer filters with `bif.isGlobalSymbol(name, dottedSuffix)`, every `storeBif` call site passes `"." & extractModuleSuffix(path)` — the same suffix the reader forms — and the visibility rule is the same test on the same token. Checked rather than argued, all the same. The old rescan stays as `rescanPosIndex` behind `-d:icIndexCheck`, which compares the two entry by entry on every module load: a full build agrees exactly (11220 entries for the system module alone), and sabotaging the comparison makes it fire, so the clean run says something. `ensureSemBuf` had the same rescan for the `.s.bif` companion; it uses the carried index too. Verified: both configurations build; `tests/ic` 40/40; 67/67 generated `.c` byte-identical to before, and cursor still identical to `PNode`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FMyRHByv7hhaQJ4Pa1bHbE --- compiler/ast2nif.nim | 63 ++++++++++++++++++++++++++++++++++---------- compiler/icprof.nim | 2 +- 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/compiler/ast2nif.nim b/compiler/ast2nif.nim index 86b02fc7eb..b24c6d2fe0 100644 --- a/compiler/ast2nif.nim +++ b/compiler/ast2nif.nim @@ -28,7 +28,7 @@ import "../dist/nimony/src/lib" / [bitabs, nifstreams, lineinfos, # `pool(c: Cursor)` accessor would shadow nifstreams' global `pool` var the writer # uses; the reader reaches pools via `symName(c)`/`strVal(c)` etc. import "../dist/nimony/src/lib/nifcore" except pool -from "../dist/nimony/src/lib" / bif import load, BifModule +from "../dist/nimony/src/lib" / bif import load, BifModule, IndexVis, ivHidden import icmodnames import "../dist/nimony/src/models" / nifindex_tags import typekeys @@ -2621,14 +2621,9 @@ proc isGlobalIndexSym(s, dottedSuffix: string): bool = if s[i] == '.': inc dots dots >= 2 -proc buildPosIndex(buf: var TokenBuf; suffix: string): Table[string, NifIndexEntry] = - ## Step 2a token-position index: scan the eagerly-parsed module `buf` for the - ## global `SymbolDef`s it OWNS and record each at the token position of its - ## enclosing tag (`(sd`/`(td`), with visibility from the marker that follows - ## the def. Replaces `readEmbeddedIndex` (whose byte offsets are meaningless - ## once the file is parsed); mirrors `bif.buildIndex` and the text writer's - ## `(.index …)`. Foreign symbols appear only as `Symbol` uses (never - ## `SymbolDef`s) so they are naturally excluded. +proc rescanPosIndex(buf: var TokenBuf; suffix: string): Table[string, NifIndexEntry] = + ## VERIFICATION ONLY (`-d:icIndexCheck`): the old full-token-stream rescan, + ## kept so `indexFromBif` can be graded against it over a whole real build. result = initTable[string, NifIndexEntry]() let dotted = "." & suffix if buf.len == 0: return @@ -2638,17 +2633,53 @@ proc buildPosIndex(buf: var TokenBuf; suffix: string): Table[string, NifIndexEnt case c.kind of TagLit: mostRecentTagPos = cursorToPosition(buf, c) - inc c # descend into the body (visit every token) + inc c of SymbolDef: let nm = symName(c) let tagPos = mostRecentTagPos - inc c # advance to the marker / next sibling + inc c if isGlobalIndexSym(nm, dotted): let vis = if c.hasMore and c.kind == DotToken: Hidden else: Exported result[nm] = NifIndexEntry(offset: tagPos, info: NoLineInfo, vis: vis) else: inc c +proc indexFromBif(m: BifModule): Table[string, NifIndexEntry] = + ## The module's name -> token-position index, taken from the index the `.bif` + ## ALREADY CARRIES rather than recomputed. + ## + ## `bif.store` builds that index in one forward traversal at write time + ## (`bif.buildIndex`) and writes it into the file; `bif.load` reads it back as + ## `BifModule.index`, with `pos` already a TOKEN index of the declaration's + ## enclosing tag — the very thing this used to rescan the whole token stream + ## to recompute, once per module per backend process. That rescan was 909ms of + ## a 10.1s cold `--ic:on` build (`-d:icBNodeProf`, `tPosIndex`). + ## + ## The two agree by construction, and it is worth saying exactly why, because + ## "the file has an index" would not be enough on its own: the writer filters + ## with `bif.isGlobalSymbol(name, dottedSuffix)` and every `storeBif` call site + ## passes `"." & extractModuleSuffix(path)`, which is the same `dottedSuffix` + ## the reader would have formed — so the two filters select the same symbols, + ## and the `vis` rule (a `DotToken` marker after the def means hidden) is the + ## same test on the same token. + result = initTable[string, NifIndexEntry](m.index.len) + for e in m.index: + result[poolSym(m.buf.pool, e.sym)] = + NifIndexEntry(offset: int(e.pos), info: NoLineInfo, + vis: (if e.vis == ivHidden: Hidden else: Exported)) + +proc indexFromBif(m: var BifModule; suffix: string): Table[string, NifIndexEntry] = + result = indexFromBif(m) + when defined(icIndexCheck): + let want = rescanPosIndex(m.buf, suffix) + doAssert result.len == want.len, + "index size differs for " & suffix & ": carried " & $result.len & + " rescanned " & $want.len + for k, v in want: + let got = result.getOrDefault(k) + doAssert got.offset == v.offset and got.vis == v.vis, + "index entry differs for " & k & " in " & suffix + proc readUnusedId(buf: var TokenBuf): int32 = ## Find the module's `(unusedid )` directive — emitted as the FIRST child ## of the top-level `(stmts ...)` by writeNifModule/writeLoweredModule — and @@ -2687,7 +2718,7 @@ proc moduleId(c: var DecodeContext; suffix: string; flags: set[LoadFlag] = {}): # This mirrors `toNifFilename` (kept in sync). `bif.load` mints FRESH per-file # pools, so the buffer's literals/tags resolve through its own # `cursorPool(n)`/`n.tags` (the reader is pool-agnostic); the token-position - # index is rebuilt name-based via `buildPosIndex`. + # index is taken from the one the file carries (`indexFromBif`). let conf = c.infos.config let useLowered = conf.cmd == cmdNifC and (conf.icBackendStage == "cg" or conf.icBackendStage == "emit") @@ -2699,8 +2730,12 @@ proc moduleId(c: var DecodeContext; suffix: string; flags: set[LoadFlag] = {}): raiseAssert "NIF file not found for module suffix '" & suffix & "': " & modFile & ". This can happen when loading a module from NIF that references another module " & "whose NIF file hasn't been written yet." + icProfStart(tBifLoad) var m = bif.load(modFile) - let index = buildPosIndex(m.buf, suffix) + icProfStop(tBifLoad) + icProfStart(tPosIndex) + let index = indexFromBif(m, suffix) + icProfStop(tPosIndex) # Seed the backend id counters ABOVE every id the file already uses, so a # freshly-minted backend sym/type (closure env, RTTI hook, temp) can never # share a `toId` with a loaded one. See `readUnusedId` / `(unusedid)`. @@ -2725,7 +2760,7 @@ proc ensureSemBuf(c: var DecodeContext; module: FileIndex) = let semFile = (getNimcacheDir(c.infos.config) / RelativeFile(m.suffix & ".s.bif")).string if not fileExists(semFile): return var sm = bif.load(semFile) - m.semIndex = buildPosIndex(sm.buf, m.suffix) + m.semIndex = indexFromBif(sm, m.suffix) m.semBuf = ensureMove sm.buf proc hasTypeOffset(c: var DecodeContext; module: FileIndex; nifName: string): bool = diff --git a/compiler/icprof.nim b/compiler/icprof.nim index 5edfaf102c..195cf0ac7f 100644 --- a/compiler/icprof.nim +++ b/compiler/icprof.nim @@ -33,7 +33,7 @@ when defined(icBNodeProf): pLastSon, pIterYield, pSym, pTyp, pTypTagLit, pOrigin, pNilType, pGenBodyCalls, pInfo TimeSlot* = enum - tLoadClosure, tModuleId, tTopLevel, tInterfTables, + tLoadClosure, tModuleId, tBifLoad, tPosIndex, tTopLevel, tInterfTables, tTransform, tHandOff, tGenBody, tAnalyses, tSym, tTyp, tInfo, tOrigin