Repository navigation
Conversation
JonoPrest
added a commit
that referenced
this pull request
Nov 3, 2026
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 readde2e2d6 · 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 test5247378 · 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 copies7d14ed3 · 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 interfaces3220de0 · 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. |
Contributor
Author
|
@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.97% 82.01% +0.03%
==========================================
Files 456 457 +1
Lines 65671 65827 +156
==========================================
+ Hits 53836 53985 +149
- Misses 11835 11842 +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 |
Contributor
Author
|
@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". |
JonoPrest
marked this pull request as ready for review
November 3, 2026 09:43
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
5 times, most recently
from
November 9, 2026 07:18
7199bfb to
4de59d8
Compare
✅ Compiler benchmarksAllocated wordsIn total: 683,553,051 → 680,175,931 (-0.49%).
67 more changes
InstructionsIn total: 9,664,671,698 → 9,664,627,235 (±0.00%). None of the 33 benchmarks changed by 0.1% or more. Output sizeIn total: 25,569,598 → 25,570,910 (+0.01%). None of the 13 benchmarks changed by 0.1% or more. Pull request 3220de0 compared with base b82d944 in this run. |
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
4 times, most recently
from
November 9, 2026 13:05
93f5ec1 to
37bc835
Compare
4 tasks done
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
2 times, most recently
from
November 9, 2026 22:04
3220de0 to
6bfcbc9
Compare
Changelog💅 Polish
Semantic Diff4 commits · Full page Copy a dependency's signature only when it is readdfb34bf · 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 test141c660 · 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 copies0f36054 · 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 interfaces189209b · 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. |
🎉 Compiler benchmarksAllocated wordsIn total: 640,764,970 → 637,168,988 (-0.56%). Generated projects: 920,848,263 → 629,733,464 (-31.61%). 🎉 Improvement: 5 benchmarks allocate more than 1% less.
72 more changes
InstructionsIn total: 9,211,172,326 → 9,212,082,355 (+0.01%).
Output sizeIn total: 25,843,198 → 25,844,591 (+0.01%). None of the 13 benchmarks changed by 0.1% or more. Pull request 189209b compared with base 00e3f40 in this run. |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9192 |
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
4 times, most recently
from
November 10, 2026 13:40
6aa529f to
8d52d81
Compare
JonoPrest
added a commit
that referenced
this pull request
Nov 10, 2026
Why:
Id_tbl.find_opened returned the binding and its position as a new
pair, which find_name and find_all took apart again to build the
path, so every lookup of a name found in the components of an open
allocated three words more than before the open tables took the
cells. In stress/RefsInLoop
that moved each statement's allocations from 46272 to 46368 bytes, a
Fibonacci number: with the minor heap the benchmarks run with, the
objects of the 300 statements in the .cmt then fall on the same slot of
the marshaller's position table (Fibonacci hashing of addresses, linear
probing), and marshalling the .cmt took about 40M more instructions
(+23% under cachegrind), though the file is byte-identical.
find_opened now gives the path and the binding, built from the pair stored
in the table; update, which only Env.copy_types calls, looks the binding up
as before. The bindings found, their order and the outputs are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -0.02% in total, stress/NestedClosures -0.13%, and
stress/RefsInLoop -5.4K words; with perf stat (s=64M as the benchmarks
run) RefsInLoop takes 456.5M instructions, against 515.0M before and 456.1M
for #9192 head.
Review: Does the instruction count of RefsInLoop now depend on luck? Yes,
as before #9194: any change of 96 bytes per statement in this benchmark
can land on a stride that hashes badly, without any work in the compiler.
The fix removes the allocation that moved it there, which is worth having
on its own.
Signed-off-by: Jono Prest <jjprest@gmail.com>
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
from
November 10, 2026 14:44
8d52d81 to
9585fd3
Compare
JonoPrest
added a commit
that referenced
this pull request
Nov 10, 2026
Why:
Id_tbl.find_opened returned the binding and its position as a new
pair, which find_name and find_all took apart again to build the
path, so every lookup of a name found in the components of an open
allocated three words more than before the open tables took the
cells. In stress/RefsInLoop
that moved each statement's allocations from 46272 to 46368 bytes, a
Fibonacci number: with the minor heap the benchmarks run with, the
objects of the 300 statements in the .cmt then fall on the same slot of
the marshaller's position table (Fibonacci hashing of addresses, linear
probing), and marshalling the .cmt took about 40M more instructions
(+23% under cachegrind), though the file is byte-identical.
find_opened now gives the path and the binding, built from the pair stored
in the table; update, which only Env.copy_types calls, looks the binding up
as before. The bindings found, their order and the outputs are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -0.02% in total, stress/NestedClosures -0.13%, and
stress/RefsInLoop -5.4K words; with perf stat (s=64M as the benchmarks
run) RefsInLoop takes 456.5M instructions, against 515.0M before and 456.1M
for #9192 head.
Review: Does the instruction count of RefsInLoop now depend on luck? Yes,
as before #9194: any change of 96 bytes per statement in this benchmark
can land on a stride that hashes badly, without any work in the compiler.
The fix removes the allocation that moved it there, which is worth having
on its own.
Signed-off-by: Jono Prest <jjprest@gmail.com>
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
from
November 10, 2026 16:14
9585fd3 to
5145a93
Compare
JonoPrest
added a commit
that referenced
this pull request
Nov 10, 2026
Why:
Id_tbl.find_opened returned the binding and its position as a new
pair, which find_name and find_all took apart again to build the
path, so every lookup of a name found in the components of an open
allocated three words more than before the open tables took the
cells. In stress/RefsInLoop
that moved each statement's allocations from 46272 to 46368 bytes, a
Fibonacci number: with the minor heap the benchmarks run with, the
objects of the 300 statements in the .cmt then fall on the same slot of
the marshaller's position table (Fibonacci hashing of addresses, linear
probing), and marshalling the .cmt took about 40M more instructions
(+23% under cachegrind), though the file is byte-identical.
find_opened now gives the path and the binding, built from the pair stored
in the table; update, which only Env.copy_types calls, looks the binding up
as before. The bindings found, their order and the outputs are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -0.02% in total, stress/NestedClosures -0.13%, and
stress/RefsInLoop -5.4K words; with perf stat (s=64M as the benchmarks
run) RefsInLoop takes 456.5M instructions, against 515.0M before and 456.1M
for #9192 head.
Review: Does the instruction count of RefsInLoop now depend on luck? Yes,
as before #9194: any change of 96 bytes per statement in this benchmark
can land on a stride that hashes badly, without any work in the compiler.
The fix removes the allocation that moved it there, which is worth having
on its own.
Signed-off-by: Jono Prest <jjprest@gmail.com>
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
from
November 10, 2026 16:24
5145a93 to
badaa5a
Compare
JonoPrest
added a commit
that referenced
this pull request
Nov 10, 2026
Why:
Id_tbl.find_opened returned the binding and its position as a new
pair, which find_name and find_all took apart again to build the
path, so every lookup of a name found in the components of an open
allocated three words more than before the open tables took the
cells. In stress/RefsInLoop
that moved each statement's allocations from 46272 to 46368 bytes, a
Fibonacci number: with the minor heap the benchmarks run with, the
objects of the 300 statements in the .cmt then fall on the same slot of
the marshaller's position table (Fibonacci hashing of addresses, linear
probing), and marshalling the .cmt took about 40M more instructions
(+23% under cachegrind), though the file is byte-identical.
find_opened now gives the path and the binding, built from the pair stored
in the table; update, which only Env.copy_types calls, looks the binding up
as before. The bindings found, their order and the outputs are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -0.02% in total, stress/NestedClosures -0.13%, and
stress/RefsInLoop -5.4K words; with perf stat (s=64M as the benchmarks
run) RefsInLoop takes 456.5M instructions, against 515.0M before and 456.1M
for #9192 head.
Review: Does the instruction count of RefsInLoop now depend on luck? Yes,
as before #9194: any change of 96 bytes per statement in this benchmark
can land on a stride that hashes badly, without any work in the compiler.
The fix removes the allocation that moved it there, which is worth having
on its own.
Signed-off-by: Jono Prest <jjprest@gmail.com>
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
from
November 10, 2026 17:24
badaa5a to
6e141e5
Compare
JonoPrest
added a commit
that referenced
this pull request
Nov 10, 2026
Why:
Id_tbl.find_opened returned the binding and its position as a new
pair, which find_name and find_all took apart again to build the
path, so every lookup of a name found in the components of an open
allocated three words more than before the open tables took the
cells. In stress/RefsInLoop
that moved each statement's allocations from 46272 to 46368 bytes, a
Fibonacci number: with the minor heap the benchmarks run with, the
objects of the 300 statements in the .cmt then fall on the same slot of
the marshaller's position table (Fibonacci hashing of addresses, linear
probing), and marshalling the .cmt took about 40M more instructions
(+23% under cachegrind), though the file is byte-identical.
find_opened now gives the path and the binding, built from the pair stored
in the table; update, which only Env.copy_types calls, looks the binding up
as before. The bindings found, their order and the outputs are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -0.02% in total, stress/NestedClosures -0.13%, and
stress/RefsInLoop -5.4K words; with perf stat (s=64M as the benchmarks
run) RefsInLoop takes 456.5M instructions, against 515.0M before and 456.1M
for #9192 head.
Review: Does the instruction count of RefsInLoop now depend on luck? Yes,
as before #9194: any change of 96 bytes per statement in this benchmark
can land on a stride that hashes badly, without any work in the compiler.
The fix removes the allocation that moved it there, which is worth having
on its own.
Signed-off-by: Jono Prest <jjprest@gmail.com>
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
from
November 10, 2026 18:14
6e141e5 to
4d78281
Compare
JonoPrest
added a commit
that referenced
this pull request
Nov 10, 2026
Why:
Id_tbl.find_opened returned the binding and its position as a new
pair, which find_name and find_all took apart again to build the
path, so every lookup of a name found in the components of an open
allocated three words more than before the open tables took the
cells. In stress/RefsInLoop
that moved each statement's allocations from 46272 to 46368 bytes, a
Fibonacci number: with the minor heap the benchmarks run with, the
objects of the 300 statements in the .cmt then fall on the same slot of
the marshaller's position table (Fibonacci hashing of addresses, linear
probing), and marshalling the .cmt took about 40M more instructions
(+23% under cachegrind), though the file is byte-identical.
find_opened now gives the path and the binding, built from the pair stored
in the table; update, which only Env.copy_types calls, looks the binding up
as before. The bindings found, their order and the outputs are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -0.02% in total, stress/NestedClosures -0.13%, and
stress/RefsInLoop -5.4K words; with perf stat (s=64M as the benchmarks
run) RefsInLoop takes 456.5M instructions, against 515.0M before and 456.1M
for #9192 head.
Review: Does the instruction count of RefsInLoop now depend on luck? Yes,
as before #9194: any change of 96 bytes per statement in this benchmark
can land on a stride that hashes badly, without any work in the compiler.
The fix removes the allocation that moved it there, which is worth having
on its own.
Signed-off-by: Jono Prest <jjprest@gmail.com>
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>
JonoPrest
added a commit
that referenced
this pull request
Nov 10, 2026
Why:
Id_tbl.find_opened returned the binding and its position as a new
pair, which find_name and find_all took apart again to build the
path, so every lookup of a name found in the components of an open
allocated three words more than before the open tables took the
cells. In stress/RefsInLoop
that moved each statement's allocations from 46272 to 46368 bytes, a
Fibonacci number: with the minor heap the benchmarks run with, the
objects of the 300 statements in the .cmt then fall on the same slot of
the marshaller's position table (Fibonacci hashing of addresses, linear
probing), and marshalling the .cmt took about 40M more instructions
(+23% under cachegrind), though the file is byte-identical.
find_opened now gives the path and the binding, built from the pair stored
in the table; update, which only Env.copy_types calls, looks the binding up
as before. The bindings found, their order and the outputs are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -0.02% in total, stress/NestedClosures -0.13%, and
stress/RefsInLoop -5.4K words; with perf stat (s=64M as the benchmarks
run) RefsInLoop takes 456.5M instructions, against 515.0M before and 456.1M
for #9192 head.
Review: Does the instruction count of RefsInLoop now depend on luck? Yes,
as before #9194: any change of 96 bytes per statement in this benchmark
can land on a stride that hashes badly, without any work in the compiler.
The fix removes the allocation that moved it there, which is worth having
on its own.
Signed-off-by: Jono Prest <jjprest@gmail.com>
JonoPrest
force-pushed
the
jono/defer-signature-copy
branch
from
November 10, 2026 18:52
4d78281 to
189209b
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.