Skip to content

Commit

Permalink
Fixing hashing issue in dictionary resize optimization (#11)
Browse files Browse the repository at this point in the history 
* Reproduced issue in unit tests: a dictionary size of 4-7 causes failed lookups.

* Fixes issue #10

* Updating possibly-affected benchmark results.
  • Loading branch information
@jtmueller
jtmueller committed Jan 24, 2019
1 parent a868a4a commit 106748f
Show file tree
Hide file tree
Showing 16 changed files with 144 additions and 134 deletions.
3 changes: 3 additions & 0 deletions Collections.Pooled.Tests/TestBase.NonGeneric.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ public static IEnumerable<object[]> ValidCollectionSizes()
{
yield return new object[] { 0 };
yield return new object[] { 1 };
//for (int i = 2; i < 11; i++)
// yield return new object[] { i };
yield return new object[] { 5 };
yield return new object[] { 75 };
}

Expand Down
8 changes: 4 additions & 4 deletions Collections.Pooled/Collections.Pooled.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@
<PackageProjectUrl>https://github.com/jtmueller/Collections.Pooled</PackageProjectUrl>
<RepositoryUrl>https://github.com/jtmueller/Collections.Pooled.git</RepositoryUrl>
<RepositoryType>Git</RepositoryType>
<AssemblyVersion>1.0.4.0</AssemblyVersion>
<FileVersion>1.0.4.0</FileVersion>
<Version>1.0.4</Version>
<PackageReleaseNotes>Using ThrowHelper for improved performance</PackageReleaseNotes>
<AssemblyVersion>1.0.0.0</AssemblyVersion>
<FileVersion>1.0.0.0</FileVersion>
<Version>1.0.0</Version>
<PackageReleaseNotes>Bug fix in a PooledDictionary performance optimization</PackageReleaseNotes>
</PropertyGroup>

<PropertyGroup Condition="'$(Configuration)'=='Release'">
Expand Down
41 changes: 24 additions & 17 deletions Collections.Pooled/PooledDictionary.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -599,14 +599,13 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
}

var entries = _entries;
var buckets = _buckets;
var comparer = _comparer;
var size = _size;

int hashCode = ((comparer == null) ? key.GetHashCode() : comparer.GetHashCode(key)) & 0x7FFFFFFF;

int collisionCount = 0;
ref int bucket = ref buckets[hashCode % size];
ref int bucket = ref _buckets[hashCode % size];
// Value in _buckets is 1-based
int i = bucket - 1;

Expand Down Expand Up @@ -749,9 +748,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
{
Resize();
size = _size;
buckets = _buckets;
entries = _entries;
bucket = ref buckets[hashCode % size];
bucket = ref _buckets[hashCode % size];
}
index = count;
_count = count + 1;
Expand Down Expand Up @@ -839,20 +836,27 @@ private void Resize(int newSize, bool forceNewHashCodes)
Debug.Assert(!forceNewHashCodes || default(TKey) == null);
Debug.Assert(newSize >= _size);

int[] buckets;
Entry[] entries;
bool replaceArrays;
int count = _count;

// Because ArrayPool might give us larger arrays than we asked for, see if we can
// use the existing capacity without actually resizing.
if (_buckets.Length >= newSize && _entries.Length >= newSize)
{
_size = newSize;
return;
buckets = _buckets;
entries = _entries;
replaceArrays = false;
}
else
{
buckets = s_bucketPool.Rent(newSize);
Array.Clear(buckets, 0, buckets.Length);
entries = s_entryPool.Rent(newSize);
Array.Copy(_entries, 0, entries, 0, count);
replaceArrays = true;
}

int[] buckets = s_bucketPool.Rent(newSize);
Array.Clear(buckets, 0, buckets.Length);
Entry[] entries = s_entryPool.Rent(newSize);

int count = _count;
Array.Copy(_entries, 0, entries, 0, count);

if (default(TKey) == null && forceNewHashCodes)
{
Expand All @@ -878,9 +882,12 @@ private void Resize(int newSize, bool forceNewHashCodes)
}
}

ReturnArrays();
_buckets = buckets;
_entries = entries;
if (replaceArrays)
{
ReturnArrays();
_buckets = buckets;
_entries = entries;
}
_size = newSize;
}

Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Collections.Pooled [![NuGet Version](https://img.shields.io/nuget/v/Collections.Pooled.svg?style=flat)](https://www.nuget.org/packages/Collections.Pooled/)
# Collections.Pooled [![NuGet Version](https://img.shields.io/nuget/v/Collections.Pooled.svg?style=flat)](https://www.nuget.org/packages/Collections.Pooled/) [![Build status](https://ci.appveyor.com/api/projects/status/vb6j2jon32ia5qq4/branch/master?svg=true)](https://ci.appveyor.com/project/jtmueller/collections-pooled/branch/master)

This library is based on classes from `System.Collections.Generic` that have been altered
to take advantage of the new `System.Span<T>` and `System.Buffers.ArrayPool<T>` libraries
Expand Down
20 changes: 10 additions & 10 deletions docs/benchmarks/net472/Dict_Add-report-github.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ Job=Clr Runtime=Clr InvocationCount=1
UnrollFactor=1

```
| Method | N | Mean | Error | StdDev | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|---------- |------- |----------:|----------:|----------:|------:|--------:|------------:|------------:|------------:|--------------------:|
| **DictAdd** | **1000** | **5.760 ms** | **0.1141 ms** | **0.2529 ms** | **1.00** | **0.00** | **1000.0000** | **1000.0000** | **1000.0000** | **7204928 B** |
| PooledAdd | 1000 | 4.580 ms | 0.1003 ms | 0.2941 ms | 0.80 | 0.06 | - | - | - | - |
| | | | | | | | | | | |
| **DictAdd** | **10000** | **9.425 ms** | **0.1863 ms** | **0.2612 ms** | **1.00** | **0.00** | **-** | **-** | **-** | **14645904 B** |
| PooledAdd | 10000 | 7.284 ms | 0.1509 ms | 0.3528 ms | 0.78 | 0.05 | - | - | - | - |
| | | | | | | | | | | |
| **DictAdd** | **100000** | **10.779 ms** | **0.2119 ms** | **0.3422 ms** | **1.00** | **0.00** | **-** | **-** | **-** | **13850984 B** |
| PooledAdd | 100000 | 8.081 ms | 0.1680 ms | 0.4794 ms | 0.75 | 0.04 | - | - | - | - |
| Method | N | Mean | Error | StdDev | Median | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|---------- |------- |----------:|----------:|----------:|----------:|------:|--------:|------------:|------------:|------------:|--------------------:|
| **DictAdd** | **1000** | **6.343 ms** | **0.1999 ms** | **0.5863 ms** | **6.201 ms** | **1.00** | **0.00** | **1000.0000** | **1000.0000** | **1000.0000** | **7204928 B** |
| PooledAdd | 1000 | 4.714 ms | 0.1454 ms | 0.4287 ms | 4.559 ms | 0.75 | 0.09 | - | - | - | - |
| | | | | | | | | | | | |
| **DictAdd** | **10000** | **10.453 ms** | **0.2397 ms** | **0.7031 ms** | **10.267 ms** | **1.00** | **0.00** | **-** | **-** | **-** | **14645904 B** |
| PooledAdd | 10000 | 8.025 ms | 0.2383 ms | 0.6988 ms | 7.869 ms | 0.77 | 0.08 | - | - | - | - |
| | | | | | | | | | | | |
| **DictAdd** | **100000** | **11.857 ms** | **0.3133 ms** | **0.9089 ms** | **11.626 ms** | **1.00** | **0.00** | **-** | **-** | **-** | **13850984 B** |
| PooledAdd | 100000 | 9.068 ms | 0.2694 ms | 0.7858 ms | 8.888 ms | 0.77 | 0.09 | - | - | - | - |
14 changes: 7 additions & 7 deletions docs/benchmarks/net472/Dict_Add-report.csv
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Method,Job,AnalyzeLaunchVariance,EvaluateOverhead,MaxAbsoluteError,MaxRelativeError,MinInvokeCount,MinIterationTime,OutlierMode,Affinity,EnvironmentVariables,Jit,Platform,Runtime,AllowVeryLargeObjects,Concurrent,CpuGroups,Force,HeapAffinitizeMask,HeapCount,NoAffinitize,RetainVm,Server,Arguments,BuildConfiguration,Clock,EngineFactory,NuGetReferences,Toolchain,IsMutator,InvocationCount,IterationCount,IterationTime,LaunchCount,MaxIterationCount,MaxWarmupIterationCount,MinIterationCount,MinWarmupIterationCount,RunStrategy,UnrollFactor,WarmupCount,N,Mean,Error,StdDev,Ratio,RatioSD,Gen 0/1k Op,Gen 1/1k Op,Gen 2/1k Op,Allocated Memory/Op
DictAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,1000,5.760 ms,0.1141 ms,0.2529 ms,1.00,0.00,1000.0000,1000.0000,1000.0000,7204928 B
PooledAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,1000,4.580 ms,0.1003 ms,0.2941 ms,0.80,0.06,-,-,-,-
DictAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,10000,9.425 ms,0.1863 ms,0.2612 ms,1.00,0.00,-,-,-,14645904 B
PooledAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,10000,7.284 ms,0.1509 ms,0.3528 ms,0.78,0.05,-,-,-,-
DictAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,100000,10.779 ms,0.2119 ms,0.3422 ms,1.00,0.00,-,-,-,13850984 B
PooledAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,100000,8.081 ms,0.1680 ms,0.4794 ms,0.75,0.04,-,-,-,-
Method,Job,AnalyzeLaunchVariance,EvaluateOverhead,MaxAbsoluteError,MaxRelativeError,MinInvokeCount,MinIterationTime,OutlierMode,Affinity,EnvironmentVariables,Jit,Platform,Runtime,AllowVeryLargeObjects,Concurrent,CpuGroups,Force,HeapAffinitizeMask,HeapCount,NoAffinitize,RetainVm,Server,Arguments,BuildConfiguration,Clock,EngineFactory,NuGetReferences,Toolchain,IsMutator,InvocationCount,IterationCount,IterationTime,LaunchCount,MaxIterationCount,MaxWarmupIterationCount,MinIterationCount,MinWarmupIterationCount,RunStrategy,UnrollFactor,WarmupCount,N,Mean,Error,StdDev,Median,Ratio,RatioSD,Gen 0/1k Op,Gen 1/1k Op,Gen 2/1k Op,Allocated Memory/Op
DictAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,1000,6.343 ms,0.1999 ms,0.5863 ms,6.201 ms,1.00,0.00,1000.0000,1000.0000,1000.0000,7204928 B
PooledAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,1000,4.714 ms,0.1454 ms,0.4287 ms,4.559 ms,0.75,0.09,-,-,-,-
DictAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,10000,10.453 ms,0.2397 ms,0.7031 ms,10.267 ms,1.00,0.00,-,-,-,14645904 B
PooledAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,10000,8.025 ms,0.2383 ms,0.6988 ms,7.869 ms,0.77,0.08,-,-,-,-
DictAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,100000,11.857 ms,0.3133 ms,0.9089 ms,11.626 ms,1.00,0.00,-,-,-,13850984 B
PooledAdd,Clr,False,Default,Default,Default,Default,Default,Default,11111111,Empty,RyuJit,X64,Clr,False,True,False,True,Default,Default,False,False,False,Default,Default,Default,Default,Default,Default,Default,1,Default,Default,Default,Default,Default,Default,Default,Default,1,Default,100000,9.068 ms,0.2694 ms,0.7858 ms,8.888 ms,0.77,0.09,-,-,-,-
26 changes: 13 additions & 13 deletions docs/benchmarks/net472/Dict_Constructors-report-github.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,16 @@ Intel Core i7-6700HQ CPU 2.60GHz (Skylake), 1 CPU, 8 logical and 4 physical core
Job=Clr Runtime=Clr

```
| Method | N | Mean | Error | StdDev | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|------------ |------ |--------------:|--------------:|--------------:|------:|--------:|------------:|------------:|------------:|--------------------:|
| **Dict_Ctor** | **0** | **62.91 us** | **3.6667 us** | **4.6372 us** | **1.00** | **0.00** | **114.6240** | **-** | **-** | **352.27 KB** |
| Pooled_Ctor | 0 | 70.15 us | 0.3320 us | 0.3106 us | 1.10 | 0.09 | 126.0986 | - | - | 387.5 KB |
| | | | | | | | | | | |
| **Dict_Ctor** | **1024** | **8,364.50 us** | **53.8764 us** | **47.7600 us** | **1.00** | **0.00** | **44203.1250** | **-** | **-** | **136580.3 KB** |
| Pooled_Ctor | 1024 | 54,229.73 us | 187.0308 us | 165.7980 us | 6.48 | 0.05 | 100.0000 | - | - | 388.01 KB |
| | | | | | | | | | | |
| **Dict_Ctor** | **4096** | **73,634.70 us** | **321.7213 us** | **285.1976 us** | **1.00** | **0.00** | **166857.1429** | **166857.1429** | **166857.1429** | **600997.77 KB** |
| Pooled_Ctor | 4096 | 212,708.27 us | 714.2509 us | 668.1107 us | 2.89 | 0.01 | - | - | - | 389.33 KB |
| | | | | | | | | | | |
| **Dict_Ctor** | **16384** | **205,060.94 us** | **989.2105 us** | **876.9094 us** | **1.00** | **0.00** | **563333.3333** | **563333.3333** | **563333.3333** | **2164813.5 KB** |
| Pooled_Ctor | 16384 | 847,360.81 us | 2,238.4208 us | 1,747.6117 us | 4.13 | 0.02 | - | - | - | 392 KB |
| Method | N | Mean | Error | StdDev | Median | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|------------ |------ |--------------:|--------------:|--------------:|--------------:|------:|--------:|------------:|------------:|------------:|--------------------:|
| **Dict_Ctor** | **0** | **86.03 us** | **0.7433 us** | **0.6953 us** | **85.85 us** | **1.00** | **0.00** | **114.6240** | **-** | **-** | **352.27 KB** |
| Pooled_Ctor | 0 | 105.50 us | 2.3862 us | 6.9983 us | 101.75 us | 1.29 | 0.06 | 126.0986 | - | - | 387.5 KB |
| | | | | | | | | | | | |
| **Dict_Ctor** | **1024** | **14,918.49 us** | **296.7426 us** | **317.5112 us** | **14,771.63 us** | **1.00** | **0.00** | **44187.5000** | **-** | **-** | **136580.3 KB** |
| Pooled_Ctor | 1024 | 63,548.47 us | 543.3831 us | 508.2809 us | 63,426.85 us | 4.26 | 0.11 | 125.0000 | - | - | 388.01 KB |
| | | | | | | | | | | | |
| **Dict_Ctor** | **4096** | **107,675.96 us** | **2,754.7319 us** | **5,307.4331 us** | **105,625.37 us** | **1.00** | **0.00** | **166800.0000** | **166800.0000** | **166800.0000** | **600997.77 KB** |
| Pooled_Ctor | 4096 | 249,759.02 us | 2,340.6100 us | 2,189.4081 us | 249,023.80 us | 2.26 | 0.11 | - | - | - | 389.33 KB |
| | | | | | | | | | | | |
| **Dict_Ctor** | **16384** | **270,678.81 us** | **3,036.8583 us** | **2,692.0958 us** | **270,155.03 us** | **1.00** | **0.00** | **563500.0000** | **563500.0000** | **563500.0000** | **2164816.17 KB** |
| Pooled_Ctor | 16384 | 986,135.33 us | 4,769.7619 us | 4,461.6383 us | 986,559.50 us | 3.64 | 0.05 | - | - | - | 392 KB |
Loading

0 comments on commit 106748f

Please sign in to comment.