pkg: don't recreate Pkg_rules.DB values #12872
Merged
+30
−45
Conversation
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
rgrinberg
reviewed
Dec 5, 2025
Alizter
force-pushed
the
push-rmmwsrozxnzs
branch
4 times, most recently
from
5edd17c to
274a7cd
Compare
rgrinberg
reviewed
Dec 9, 2025
Alizter
changed the title
Alizter
force-pushed
the
push-rmmwsrozxnzs
branch
2 times, most recently
from
2b8bc92 to
31e5bb2
Compare
rgrinberg
reviewed
Dec 9, 2025
Alizter
force-pushed
the
push-rmmwsrozxnzs
branch
from
31e5bb2 to
f210f64
Compare
Alizter
changed the title
Alizter
force-pushed
the
push-rmmwsrozxnzs
branch
from
f210f64 to
0d8deb5
Compare
Alizter
force-pushed
the
push-rmmwsrozxnzs
branch
from
0d8deb5 to
ef350d6
Compare
rgrinberg
approved these changes
Dec 10, 2025
Member
rgrinberg
left a comment
rgrinberg
left a comment
There was a problem hiding this comment.
The previous code was needlessly convoluted so your attempt at memoization lost sharing. I refactored things to be clearer and gained back the sharing.
This table is only created and used twice. We add an Id.t field so that it can easily be compared later when it's values are memoized in the of_ctx call. The old hash and equality functions were expensive an unnecessary. We also memoize the of_dev_tool caller Before: ``` [ocaml-cohttp]$ hyperfine "dune build" Benchmark 1: dune build Time (mean ± σ): 1.851 s ± 0.039 s [User: 1.561 s, System: 0.283 s] Range (min … max): 1.814 s … 1.943 s 10 runs ``` After: ``` [ocaml-cohttp]$ hyperfine "dune build" Benchmark 1: dune build Time (mean ± σ): 1.086 s ± 0.016 s [User: 0.803 s, System: 0.279 s] Range (min … max): 1.052 s … 1.108 s 10 runs ``` Signed-off-by: Ali Caglayan <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]> Signed-off-by: Ali Caglayan <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
The DB is already memoized Signed-off-by: Rudi Grinberg <[email protected]> Signed-off-by: Ali Caglayan <[email protected]>
Alizter
force-pushed
the
push-rmmwsrozxnzs
branch
from
14daebb to
d4a9786
Compare
Member
|
Contributes to #12693 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This table is only created and used twice. We add an Id.t field so that
it can easily be compared later when it's values are memoized in the
of_ctx call.
The old hash and equality functions were expensive an unnecessary.
Before:
After: