Repository navigation
Conversation
JonoPrest
added a commit
that referenced
this pull request
Nov 3, 2026
The entry names #9194. Signed-off-by: Jono Prest <jjprest@gmail.com>
Contributor
Author
|
@codex review |
Changelog💅 Polish
Semantic Diff2 commits · Full page Copy a dependency's values when they are read6d73a8e · JonoPrest · Nov 03, 2026, 11:26 UTC
For the components of a .cmi signature as it was read, values are now table cells that make the copy on first read. Type ids are allocated in order and end up in .cmt files and in which nodes the backtracking trail logs, so the copies keep their ids: once the process has built the components of the same .cmi contents (same CRC) eagerly, it knows how many type ids each value takes, reserves them when the components are built and makes the copy with exactly those ids when the value is read. A copy that takes a different number is made again with fresh ids, like the deferred signature copies, and counted per domain for the tests. One-shot bsc and the first use of an interface in a build still build eagerly. Types, constructors, labels, extension constructors, module types, submodules and components of anything else (aliases, functor applications, local modules) are built as before. The tables of an open take the cells too: an open keeps a function giving the binding of each component, and the bindings Env.copy_types replaces through an open in a table checked before the components. Review
Allocate a value's copy closure only when the copy is deferredb9df7dd · JonoPrest · Nov 09, 2026, 14:34 UTC
Measured with scripts/compilerBenchmarks.js against the previous commit: allocated words -0.15% in total, -0.25% in the project builds (where components are built again in one process and their counts are known), -0.04% for the files compiled one by one. On a 552 module React app (clean build, one domain) the build allocates 0.2% fewer words. |
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: Didn't find any major issues. Breezy! 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 @@
## jono/defer-signature-copy #9194 +/- ##
=============================================================
+ Coverage 82.01% 82.07% +0.06%
=============================================================
Files 457 458 +1
Lines 65827 66174 +347
=============================================================
+ Hits 53985 54315 +330
- Misses 11842 11859 +17
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
JonoPrest
marked this pull request as ready for review
November 3, 2026 11:56
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 3, 2026 19:51
8810538 to
67eb3bc
Compare
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 3, 2026 21:51
67eb3bc to
94bd206
Compare
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 4, 2026 06:15
94bd206 to
c342131
Compare
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 4, 2026 17:14
c342131 to
87d814f
Compare
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 9, 2026 07:18
87d814f to
f8f3ba1
Compare
🎉 Compiler benchmarksAllocated wordsIn total: 680,168,100 → 675,146,415 (-0.74%). 🎉 Improvement: 2 benchmarks allocate more than 1% less.
221 more changes
InstructionsIn total: 9,664,627,709 → 9,667,238,667 (+0.03%).
Output sizeIn total: 25,570,910 → 25,572,544 (+0.01%). None of the 13 benchmarks changed by 0.1% or more. Pull request b9df7dd compared with base 462c00f in this run. |
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 9, 2026 07:34
f8f3ba1 to
1ddb5e1
Compare
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 9, 2026 12:01
1ddb5e1 to
31cc3ad
Compare
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 9, 2026 12:41
31cc3ad to
787e4e0
Compare
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 9, 2026 13:05
787e4e0 to
3f453f9
Compare
4 tasks done
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 9, 2026 22:04
b9df7dd to
737b3cf
Compare
Changelog💅 Polish
Semantic Diff3 commits · Full page Copy a dependency's values when they are readf77df90 · JonoPrest · Nov 03, 2026, 11:26 UTC
For the components of a .cmi signature as it was read, values are now table cells that make the copy on first read. Type ids are allocated in order and end up in .cmt files and in which nodes the backtracking trail logs, so the copies keep their ids: once the process has built the components of the same .cmi contents (same CRC) eagerly, it knows how many type ids each value takes, reserves them when the components are built and makes the copy with exactly those ids when the value is read. A copy that takes a different number is made again with fresh ids, like the deferred signature copies, and counted per domain for the tests. One-shot bsc and the first use of an interface in a build still build eagerly. Types, constructors, labels, extension constructors, module types, submodules and components of anything else (aliases, functor applications, local modules) are built as before. The tables of an open take the cells too: an open keeps a function giving the binding of each component, and the bindings Env.copy_types replaces through an open in a table checked before the components. Review
Allocate a value's copy closure only when the copy is deferred879474f · JonoPrest · Nov 09, 2026, 14:34 UTC
Measured with scripts/compilerBenchmarks.js against the previous commit: allocated words -0.15% in total, -0.25% in the project builds (where components are built again in one process and their counts are known), -0.04% for the files compiled one by one. On a 552 module React app (clean build, one domain) the build allocates 0.2% fewer words. Build the path of a component found through an open with its binding4b6c1e4 · JonoPrest · Nov 10, 2026, 11:40 UTC
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
|
🎉 Compiler benchmarksAllocated wordsIn total: 637,171,344 → 631,941,561 (-0.82%). Generated projects: 629,737,665 → 548,300,925 (-12.93%). 🎉 Improvement: 5 benchmarks allocate more than 1% less.
223 more changes
InstructionsIn total: 9,212,082,829 → 9,215,995,461 (+0.04%).
5 more changes
Output sizeIn total: 25,844,591 → 25,846,458 (+0.01%). None of the 13 benchmarks changed by 0.1% or more. Pull request 4b6c1e4 compared with base 1140711 in this run. |
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 10, 2026 06:14
737b3cf to
6aaa334
Compare
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
3 times, most recently
from
November 10, 2026 13:40
0c22d07 to
71f4b13
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
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-dependency-value-copies
branch
from
November 10, 2026 14:44
911b4e0 to
1d62ccc
Compare
JonoPrest
force-pushed
the
jono/defer-dependency-value-copies
branch
from
November 10, 2026 16:14
1d62ccc to
9017d3f
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
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-dependency-value-copies
branch
from
November 10, 2026 16:24
9017d3f to
b87ee5c
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-dependency-value-copies
branch
from
November 10, 2026 17:24
b87ee5c to
64186ad
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-dependency-value-copies
branch
from
November 10, 2026 18:14
64186ad to
f1672f7
Compare
Why:
Forcing a dependency's components copied every value of its interface
with the module path substitution, although a dependent reads only a
few of them: on a 552-module app, about 5 % of the values of .cmi
components are ever read.
For the components of a .cmi signature as it was read, values are now
table cells that make the copy on first read. Type ids are allocated in
order and end up in .cmt files and in which nodes the backtracking trail
logs, so the copies keep their ids: once the process has built the
components of the same .cmi contents (same CRC) eagerly, it knows how many
type ids each value takes, reserves them when the components are built and
makes the copy with exactly those ids when the value is read. A copy that
takes a different number is made again with fresh ids, like the deferred
signature copies, and counted per domain for the tests. One-shot bsc and
the first use of an interface in a build still build eagerly. Types,
constructors, labels, extension constructors, module types, submodules and
components of anything else (aliases, functor applications, local modules)
are built as before.
The tables of an open take the cells too: an open keeps a function giving
the binding of each component, and the bindings Env.copy_types replaces
through an open in a table checked before the components.
Review: Can a deferred copy differ from the eager one apart from when it is
made? The copied values are the unmarshalled .cmi signature, which the type
checker never sees (it only gets copies), and the copy takes no stamps; the
type ids are the eager ones, so every later stamp and id, and every
operation on the copied nodes, is the same. The new unit tests compare
marshalled results of eager and deferred components over every runtime
interface, and whole requests (outputs, .cmt and error messages) using
functors, aliases, include, module type of, recursive modules, shadowing
and first-class modules, also on several domains at once.
Review: Are the cells shared between requests or domains? No: components
belong to the persistent structures of one request; only the count table is
shared, behind a mutex, and its arrays are not changed once recorded.
Signed-off-by: Jono Prest <jjprest@gmail.com>
Why:
`components_of_module_maker` built a closure for the copy of every
value it added, also when it then copied the value at once (the first
time the process builds a .cmi's components, and for every module
that is not part of a .cmi), and checked recorded counts against the
signature by converting the count array to a list.
`make_cell` now takes the substitution and the declaration and builds a
closure only for a deferred copy, and the counts are checked by walking
the signature with an index. Which values are deferred, the type ids
they take and the components built are unchanged.
Measured with scripts/compilerBenchmarks.js against the previous commit:
allocated words -0.15% in total, -0.25% in the project builds (where
components are built again in one process and their counts are known),
-0.04% for the files compiled one by one. On a 552 module React app
(clean build, one domain) the build allocates 0.2% fewer words.
Signed-off-by: Jono Prest <jjprest@gmail.com>
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-dependency-value-copies
branch
from
November 10, 2026 18:52
f1672f7 to
4b6c1e4
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.
Stacked on #9192.
When a module first uses a dependency,
Envbuilds that dependency's components and copies every value of its interface with the module path substitution. Dependents read only a few of them: on a 552 module React app about 5% of those values are ever read.For the components of a
.cmisignature, values are now table cells that make the copy on first read. This uses the same approach as #9192 to keep output identical:.cmi,.cmj, JS and.cmt, stays the same.bscand the first use of an interface in a build are unchanged.opentables take the cells too: an open keeps a function giving each component's binding, andEnv.copy_typesoverrides go in a small table checked first.The gain is real but modest, and this adds a fair amount of code to
env.ml(about 300 lines). Paired benchmark, 20 interleaved clean builds per project, against #9192, user CPU with bootstrap 95% CI (for the full version, deferring types and labels too; this values only version measured the same head to head, with about 30% less code):Wall time at default domains is within noise on two of the three projects. Allocation drops about 2% on the React app. So this is a few percent of CPU, and only in the long running rewatch process. Happy to drop it if that isn't worth the extra machinery in
env.ml.Testing:
ounit_deferred_components_tests.ml: eager vs deferred components of every runtime interface compared as marshalled results, unread values still reserving their ids, the fallback for a wrong count, a copy that raises,Env.copy_typesthrough an open, and whole compiler requests (outputs,.cmtand error messages with functors, aliases, include,module type of, recursive modules, shadowing, first class modules) eager vs deferred, also on 4 domains at once..cmi,.cmj,.ast,.cmtand.cmtiare byte identical to master across three production codebases and their dependencies, at 1 and the default number of domains.make testandmake test-rewatchpass.