Repository navigation
Conversation
The entry names #9192 and is filed under polish like the other build speedups. Signed-off-by: Jono Prest <jjprest@gmail.com>
Changelog💅 Polish
Semantic Diff4 commits · Full page Copy a dependency's signature only when it is reade710627 · JonoPrest · Nov 03, 2026, 08:47 UTC
normalize_path now only looks the unit up (keeping the .cmi load, import and CRC checks) and reserves the stamps and type ids the copy would take, so every later identifier and type node gets the same stamp and id as before. The copy is made with exactly those stamps and ids when the signature is first read (find_module, scrape_alias, fold_modules, read_signature); on a 552-module app 2,921 copies are reserved and 10 are ever made. How many stamps and ids a copy takes is learnt from the first copy of the same .cmi contents, keyed by the CRC create_cmi wrote, and kept for the process. If a reserved copy ever takes a different number, it is thrown away and made again with fresh stamps and ids, so a wrong count can change ids but never makes two nodes share one. Review
Read the signatures in reverse order in the deferred copy test80c9bb5 · JonoPrest · Nov 03, 2026, 08:53 UTC List.rev_map applies its function from the head, so the test read the signatures in the order they were reserved; it now reads the last one first, so each copy is made with stamps and ids reserved before others. Test the counter restore and the fallback of deferred signature copiesa8323b2 · JonoPrest · Nov 03, 2026, 09:02 UTC The independent review found the env.mli comment for read_signature attached to the new stats function, and the comment and changelog overstating when copies are deferred: only after an earlier copy of the same interface in the process, never for the first one or a one-shot bsc. New unit tests check that Ident.with_current_time puts the counter back when its function raises, and that a deferred copy whose counts are wrong (a forged .cmi CRC) is made again with fresh stamps. Build on OCaml 5.0 and test unread copies with runtime interfaceseb53533 · JonoPrest · Nov 03, 2026, 09:23 UTC Mutex.protect only exists from OCaml 5.1, so the count table locks with Mutex.lock and Fun.protect like Process_child.with_lock. The unread copy test asked for Belt modules, which the coverage job doesn't build; it now uses runtime interfaces only. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9192 +/- ##
==========================================
+ Coverage 81.73% 81.76% +0.03%
==========================================
Files 458 459 +1
Lines 65292 65448 +156
==========================================
+ Hits 53367 53516 +149
- Misses 11925 11932 +7
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9192 |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
5695c85 to
467b151
Compare
Why:
Env keeps each loaded compilation unit's signature as a lazy copy of the
.cmi signature with its binders renamed (Subst.signature). normalize_path
forced that copy just to learn that a compilation unit is not a module
alias, which it never is; normalize_path is reached for every value
reference such as Belt.Array.map, for abstract dependency types in
Ctype.try_expand_once, and from Includemod and Printtyp. These copies were
about 4-6 % of a clean build's CPU time on real projects (ui: 0.62 s of
13.3 s with one domain).
normalize_path now only looks the unit up (keeping the .cmi load, import and
CRC checks) and reserves the stamps and type ids the copy would take, so
every later identifier and type node gets the same stamp and id as before.
The copy is made with exactly those stamps and ids when the signature is
first read (find_module, scrape_alias, fold_modules, read_signature); on a
552-module app 2,921 copies are reserved and 10 are ever made.
How many stamps and ids a copy takes is learnt from the first copy of the
same .cmi contents, keyed by the CRC create_cmi wrote, and kept for the
process. If a reserved copy ever takes a different number, it is thrown away
and made again with fresh stamps and ids, so a wrong count can change ids but
never makes two nodes share one.
Review: Are the artifacts the same? Yes: stamps and type ids are the same by
construction. JS, .cmi, .cmj, .ast and .cmt are byte-identical to master on
the 5,398-artifact corpus (default and one domain), and a unit test checks
that deferred copies of every runtime and Belt interface marshal to the same
bytes as eager ones, with the same stamps and ids created around them.
Review: Can the copy differ when it is made later? Only if the .cmi graph
changed in between; the type checker only works on copies of it, and closed
object types' shared mutability cells, the one part it can change, do not
change what is copied.
Signed-off-by: Jono Prest <jjprest@gmail.com>
List.rev_map applies its function from the head, so the test read the signatures in the order they were reserved; it now reads the last one first, so each copy is made with stamps and ids reserved before others. Signed-off-by: Jono Prest <jjprest@gmail.com>
The independent review found the env.mli comment for read_signature attached to the new stats function, and the comment and changelog overstating when copies are deferred: only after an earlier copy of the same interface in the process, never for the first one or a one-shot bsc. New unit tests check that Ident.with_current_time puts the counter back when its function raises, and that a deferred copy whose counts are wrong (a forged .cmi CRC) is made again with fresh stamps. Signed-off-by: Jono Prest <jjprest@gmail.com>
Mutex.protect only exists from OCaml 5.1, so the count table locks with Mutex.lock and Fun.protect like Process_child.with_lock. The unread copy test asked for Belt modules, which the coverage job doesn't build; it now uses runtime interfaces only. Signed-off-by: Jono Prest <jjprest@gmail.com>
467b151 to
eb53533
Compare
When a module refers to a dependency,
Env.normalize_pathchecks whether it is a module alias, and that forces the dependency'sps_sig: a fullSubstcopy of the interface. Almost none of those copies are read afterwards. On a 552 module React app these copies take about 5% of build CPU. With this change 2,921 of them are deferred, and only 10 of those are ever read.This defers the copy without changing any output:
normalize_pathnow only loads the.cmi(imports and CRC checks still happen) and reserves the identifier stamps and type ids that the copy would take, by advancing both counters.find_module,fold_modules,read_signature) makes the copy with the counters set back to the reserved bases. Every stamp and type id is then the same as with an eager copy, so.cmi,.cmj, JS and.cmtstay byte identical..cmi. The first copy of each interface in a process (and a one shotbsc) is still made eagerly.It is independent of #8874, which caches reading the
.cmifiles rather than copying them.Clean builds, 8 vCPU Linux VM, this PR / master, 5 runs:
Testing:
ounit_deferred_signature_tests.mlcopies every runtime and Belt.cmieagerly and deferred, creating identifiers and type variables between reservations and reading the signatures in reverse order, and compares the marshalled results byte for byte. It also covers interfaces that are reserved but never read, counter restore on exceptions, and the fallback for a wrong count.belt_deferred_signature_copy_test.rescovers alias chains, functors,module type of,includeand recursive modules over dependency types, and threesuper_errorsfixtures print dependency types through aliases..cmi,.cmj,.ast,.cmtand.cmtiare byte identical to master across three production codebases and their dependencies at 1, 16 and the default number of domains.make testandmake test-rewatchpass.