Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add more specialized frozen collection types. #79794

Merged
merged 2 commits into from
Jan 9, 2023
Merged

Add more specialized frozen collection types. #79794

merged 2 commits into from
Jan 9, 2023

Conversation

geeknoid
Copy link
Member

  • Add SmallFrozenDictionary/Set which don't do any hashing and simply iterate through arrays, testing each element

  • Add ClosedRangeInt32FrozenDictionary/Set which are used when the keys are in a closed integer range.

  • Add SparseRangeInt32FrozenSet which uses a bit vector for storage.

FrozenDictionary/Set each have some constants defined to drive the heuristics that decide when to use the small and sparse collection types. I did some preliminary tuning of these, but this could use a detailed benchmark to pick the right values. I'll leave the fine-tuning to a separate PR.

@ghost ghost assigned geeknoid Dec 18, 2022
@ghost
Copy link

ghost commented Dec 18, 2022

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

  • Add SmallFrozenDictionary/Set which don't do any hashing and simply iterate through arrays, testing each element

  • Add ClosedRangeInt32FrozenDictionary/Set which are used when the keys are in a closed integer range.

  • Add SparseRangeInt32FrozenSet which uses a bit vector for storage.

FrozenDictionary/Set each have some constants defined to drive the heuristics that decide when to use the small and sparse collection types. I did some preliminary tuning of these, but this could use a detailed benchmark to pick the right values. I'll leave the fine-tuning to a separate PR.

Author: geeknoid
Assignees: -
Labels:

area-System.Collections
Milestone: -

gfoidl
gfoidl reviewed Dec 18, 2022
View reviewed changes
...stem.Collections.Immutable/src/System/Collections/Frozen/ClosedRangeInt32FrozenDictionary.cs Outdated Show resolved Hide resolved
...stem.Collections.Immutable/src/System/Collections/Frozen/ClosedRangeInt32FrozenDictionary.cs Outdated Show resolved Hide resolved
...ries/System.Collections.Immutable/src/System/Collections/Frozen/ClosedRangeInt32FrozenSet.cs Outdated Show resolved Hide resolved
...ries/System.Collections.Immutable/src/System/Collections/Frozen/ClosedRangeInt32FrozenSet.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallInt32FrozenSet.cs Outdated Show resolved Hide resolved
...ries/System.Collections.Immutable/src/System/Collections/Frozen/SparseRangeInt32FrozenSet.cs Outdated Show resolved Hide resolved
...ries/System.Collections.Immutable/src/System/Collections/Frozen/SparseRangeInt32FrozenSet.cs Outdated Show resolved Hide resolved
...ries/System.Collections.Immutable/src/System/Collections/Frozen/SparseRangeInt32FrozenSet.cs Outdated Show resolved Hide resolved
...ries/System.Collections.Immutable/src/System/Collections/Frozen/SparseRangeInt32FrozenSet.cs Outdated Show resolved Hide resolved
@geeknoid geeknoid changed the title Add 5 more specialized frozen collection types. Add 7 more specialized frozen collection types. Dec 18, 2022
@danmoseley
Copy link
Member

Is there an issue approving the API - or is this intended to start discussion...

@geeknoid
Copy link
Member Author

@danmoseley There is no API change here, all these types are internal to the implementation of the existing frozen set and dictionary.

@geeknoid geeknoid force-pushed the main branch 2 times, most recently from 78abf93 to 27abcb3 Compare December 22, 2022 05:29
@build-analysis build-analysis bot mentioned this pull request Dec 22, 2022
Closed
@geeknoid geeknoid force-pushed the main branch 2 times, most recently from de2ac98 to 9a0bb73 Compare December 22, 2022 18:15
stephentoub
stephentoub reviewed Jan 3, 2023
View reviewed changes
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs Outdated Show resolved Hide resolved
....Collections.Immutable/src/System/Collections/Frozen/Integer/SmallIntegerFrozenDictionary.cs Outdated Show resolved Hide resolved
.../System.Collections.Immutable/src/System/Collections/Frozen/Integer/SmallIntegerFrozenSet.cs Outdated Show resolved Hide resolved
...ibraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallFrozenDictionary.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallFrozenSet.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/tests/Frozen/FrozenSetTests.cs Show resolved Hide resolved
Add 7 more specialized frozen collection types.
a8cfa79 
- Add SmallFrozenDictionary/Set which don't do any hashing and simply iterate through arrays,
testing each element

- Add ClosedRangeInt32FrozenDictionary/Set which are used when the keys are in a
closed integer range.

- Add SparseRangeInt32FrozenSet which uses a bit vector for storage.

FrozenDictionary/Set each have some constants defined to drive the heuristics that
decide when to use the small and sparse collection types. I did some preliminary tuning
of these, but this could use a detailed benchmark to pick the right values. I'll leave
the fine-tuning to a separate PR.
@geeknoid geeknoid force-pushed the main branch 8 times, most recently from e2ef0a9 to 31712f6 Compare January 4, 2023 23:22
@geeknoid geeknoid changed the title Add 7 more specialized frozen collection types. Add more specialized frozen collection types. Jan 4, 2023
@geeknoid geeknoid force-pushed the main branch 2 times, most recently from dad8b1a to 2c9fc66 Compare January 5, 2023 02:42
This was referenced Jan 5, 2023
Closed
Closed
stephentoub
stephentoub approved these changes Jan 8, 2023
View reviewed changes
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Constants.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs Outdated Show resolved Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenSet.cs Outdated Show resolved Hide resolved
...ollections.Immutable/src/System/Collections/Frozen/Int32/ClosedRangeInt32FrozenDictionary.cs Outdated Show resolved Hide resolved
...ollections.Immutable/src/System/Collections/Frozen/Int32/ClosedRangeInt32FrozenDictionary.cs Outdated Show resolved Hide resolved
...es/System.Collections.Immutable/src/System/Collections/Frozen/Int32/Int32FrozenDictionary.cs Show resolved Hide resolved
...libraries/System.Collections.Immutable/src/System/Collections/Frozen/Int32/Int32FrozenSet.cs Show resolved Hide resolved
...ystem.Collections.Immutable/src/System/Collections/Frozen/Int32/SparseRangeInt32FrozenSet.cs Outdated Show resolved Hide resolved
...m.Collections.Immutable/src/System/Collections/Frozen/Integer/SparseRangeIntegerFrozenSet.cs Outdated Show resolved Hide resolved
@geeknoid geeknoid merged commit dcda1e0 into dotnet:main Jan 9, 2023
ViktorHofer
ViktorHofer reviewed Jan 10, 2023
View reviewed changes
src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj
Comment on lines +47 to +49
<Compile Include="System\Collections\Frozen\Int32\SmallInt32FrozenDictionary.cs" Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net7.0'))" />
<Compile Include="System\Collections\Frozen\Int32\SmallInt32FrozenSet.cs" Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net7.0'))" />
<Compile Include="System\Collections\Frozen\Int32\SparseRangeInt32FrozenSet.cs" Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net7.0'))" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These msbuild functions aren't cost-free and we usually try to not have them per compile item. Can you please move them into a separate ItemGroup? Same for the ones above (L39-43).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIll do.

@lewing lewing mentioned this pull request Jan 29, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Feb 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@teo-tsirpanis teo-tsirpanis teo-tsirpanis left review comments

@ViktorHofer ViktorHofer ViktorHofer left review comments

@gfoidl gfoidl gfoidl left review comments

@stephentoub stephentoub stephentoub approved these changes

@grbell-ms grbell-ms grbell-ms approved these changes

Assignees

@geeknoid geeknoid

Labels
area-System.Collections
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@geeknoid @danmoseley @stephentoub @gfoidl @ViktorHofer @teo-tsirpanis @grbell-ms