Repository navigation
perf: SortedMap.exists and forall walk a map with a reference type through the interface - #47
Open
xperiandri wants to merge 1 commit into
Open
xperiandri wants to merge 1 commit into
xperiandri wants to merge 1 commit into
Conversation
…e through the interface The code that all reference types share needs a lookup of the exact type at run time for each call of a member of a generic struct, and the struct enumerator of `ImmutableSortedDictionary<K, V>` is made of such calls. A walk through `IEnumerable<KeyValuePair<K, V>>` took 0.56 to 0.62 times as long for strings of 100 to 100 000 bindings (16 against 29 ns for a binding) and 1.3 to 1.6 times as long for integers, so `typeof` of the key and of the value picks the enumerator: the struct one when both are value types, the interface when either is a reference type. Compiled into the library and measured against a copy of the old loop in the same process, `exists` and `forall` took 0.55 to 0.84 times as long for string keys and values (the largest map is the noisiest cell), 0.60 to 0.69 for a string next to an integer in either place, and the same time for integer keys and values (0.90 to 1.13). The cost is the boxed enumerator, 56 bytes per call, which the comment of the functions already counted. The contracts run on integers, so strings, and a string next to an integer, get tests of their own: the results of `Map`, and the stop at the first match and at the first failure. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
4 of 6 tasks
There was a problem hiding this comment.
🟡 Changes recommended
The value-type branches of exists and forall lack tests covering empty maps, matching and nonmatching predicates, and short-circuiting call counts.
1 open issue
What changed in this PR
Optimizes SortedMap.exists and SortedMap.forall traversal for reference-type keys or values while preserving public signatures and results.
Changes:
- Uses interface enumeration when either type is a reference type, retaining struct enumeration when both are value types.
- Documents measured performance and allocation tradeoffs.
- Adds string and mixed-type tests for result parity with F#
Map, plus string short-circuit tests.
| File | Description |
|---|---|
| tests/FSharp.Collections.Immutable.Tests/SortedMap/SortedMapTests.fs | Adds reference-type and mixed-type predicate tests. |
| src/FSharp.Collections.Immutable/SortedMap/SortedMap.fs | Selects traversal by key/value type and documents performance measurements. |
Review effort: Balanced
|
|
||
| try | ||
| let mutable found = false | ||
| if typeof<'Key>.IsValueType && typeof<'Value>.IsValueType then |
xperiandri
force-pushed
the
perf/sorted-map
branch
from
November 8, 2026 02:41
60c5bb1 to
9c335ec
Compare
xperiandri
force-pushed
the
feat/sorted-map
branch
from
November 8, 2026 02:41
b66dd42 to
185b0a9
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
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.
Proposed Changes
Builds on #37.
SortedMap.existsandSortedMap.forallwalk a map throughIEnumerable<KeyValuePair<K, V>>when the key or the value is a reference type, and through the struct enumerator ofImmutableSortedDictionary<K, V>when both are value types. In the code that all reference types share, each call of a member of a generic struct needs a lookup of the exact type at run time, and the struct enumerator ofImmutableSortedDictionary<K, V>is made of such calls. No public signature changes.existsandforallof the library against a copy of the old loop, 100 to 100 000 bindings, two runsRatios are the time relative to the code before; below 1 is faster. The cost of the interface is the boxed enumerator, 56 bytes per call, which the comment of the functions already counted. The base build read 0.90 to 1.10 against the same copy of the old loop, which is the check of the script.
The other collections were measured for the same pattern too; only
SortedSetandSortedMapgain (the method, and the tables of every collection and cell, are in the fifth comment of #45: #45 (comment)).The contracts run on integers, so the commit adds tests with strings and with a string next to an integer: the results of
Map, and the stop at the first match and at the first failure.Types of changes
A performance change that keeps every result and every signature.
Checklist
existsandforallcarries the measured numbers, as the other comments of the module do🤖 Generated with Claude Code