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.

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

Faster optimized frozen dictionary creation (2/n) #87630

Merged
merged 5 commits into from
Jun 16, 2023

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Jun 15, 2023

  • let the caller of FrozenHashTable.Create provide the hashcodes, it eliminates a closure and boundary checks and one method call (7% gain)
    • in case of hash set of integers it avoids renting and copying all integers to just have a copy of them
    • this is done at a cost of duplicated code, but since it eliminates a closure I hope it's not going to have big impact on the code size
  • keys and values are needed for every strategy, lets create them up-front (2% gain)
    • in CreateLengthBucketsFrozenDictionaryIfAppropriate iterate over array rather than dictionary
    • in OrdinalStringFrozenDictionary ctor avoid creating a copy of all entries (we already have a copy of keys)
LaunchCount=9  MemoryRandomization=True
Method Job Size Mean Ratio Allocated Alloc Ratio
FrozenDictionaryOptimized this PR 512 67.22 us 0.73 74.88 KB 0.88
FrozenDictionaryOptimized #87510 512 75.02 us 0.82 74.91 KB 0.88
FrozenDictionaryOptimized before #87510 512 91.66 us 1.00 85.21 KB 1.00

…oids a closure, index boundary checks and helps with inlining (7% gain)

in case of hash set of integers it avoids renting and copying all integers to just have a copy of them
…ont (2-4% gain)

* in CreateLengthBucketsFrozenDictionaryIfAppropriate iterate over array rather than dictionary
* in OrdinalStringFrozenDictionary ctor avoid creating a copy of all entires (we already have a copy of keys)
@ghost
Copy link

ghost commented Jun 15, 2023

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

Issue Details
  • let the caller of FrozenHashTable.Create provide the hashcodes, it eliminates a closure and boundary checks and one method call (7% gain)
    • in case of hash set of integers it avoids renting and copying all integers to just have a copy of them
    • this is done at a cost of duplicated code, but since it eliminates a closure it should not have big impact on the code size
  • keys and values are needed for every strategy, lets create them up-front (2% gain)
    • in CreateLengthBucketsFrozenDictionaryIfAppropriate iterate over array rather than dictionary
    • in OrdinalStringFrozenDictionary ctor avoid creating a copy of all entries (we already have a copy of keys)
LaunchCount=9  MemoryRandomization=True
Method Job Size Mean Ratio Allocated Alloc Ratio
FrozenDictionaryOptimized this PR 512 67.22 us 0.73 74.88 KB 0.88
FrozenDictionaryOptimized #87510 512 75.02 us 0.82 74.91 KB 0.88
FrozenDictionaryOptimized before #87510 512 91.66 us 1.00 85.21 KB 1.00
Author: adamsitnik
Assignees: -
Labels:

area-System.Collections, tenet-performance

Milestone: -

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

I don't love the duplication, but I don't have a better suggestion for avoiding the delegate overhead. Nice bump. Thanks.

@stephentoub stephentoub merged commit 80d87f8 into dotnet:main Jun 16, 2023
_hashTable = FrozenHashTable.Create(
entries.Length,
index => entries[index] is T t ? Comparer.GetHashCode(t) : 0,
hashCodes,
(destIndex, srcIndex) => _items[destIndex] = entries[srcIndex],
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely familiar with the implementation, but curious why we're doing delayed initialization for the _items array?

Copy link
Member

Choose a reason for hiding this comment

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

It's not delayed, it's happening during the call to Create, which is helping to put the values in the right spot.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants