debugcsharpMinor
Fixed size dictionary to achieve performance goals
Viewed 0 times
sizeachievegoalsperformancefixeddictionary
Problem
In my open source project, I have code that uses a dictionary very intensively so a system dictionary implementation did not suit me due to performance reasons. Therefore, I decided to write my own implementation that uses unsafe code. Fortunately, I can use integers as dictionary keys and I always know the max dictionary size.
Here is the implementation of the specific collection:
```
using System.Collections;
using System.Collections.Generic;
using System.Linq;
namespace logviewer.logic.support
{
public unsafe class FixedSizeDictionary : IDictionary
{
private readonly int count;
private int[] indexes;
private T[] store;
public FixedSizeDictionary(int count)
{
this.count = count;
this.store = new T[count];
this.indexes = new int[count];
}
public IEnumerator> GetEnumerator()
{
for (var i = 0; i (i, this.store[i]);
}
}
}
IEnumerator IEnumerable.GetEnumerator()
{
return this.GetEnumerator();
}
public void Add(KeyValuePair item)
{
this.Add(item.Key, item.Value);
}
public void Clear()
{
this.store = new T[this.count];
this.indexes = new int[this.count];
}
public bool Contains(KeyValuePair item)
{
return this.indexes[item.Key] > 0 && Equals(this.store[item.Key], item.Value);
}
public void CopyTo(KeyValuePair[] array, int arrayIndex)
{
for (var i = arrayIndex; i (i, this.store[i]);
}
}
}
public bool Remove(KeyValuePair item)
{
return this.Remove(item.Key);
}
public int Count
{
get
{
var result = 0;
for (var i = 0; i false;
public bool ContainsKey(int key)
{
Here is the implementation of the specific collection:
```
using System.Collections;
using System.Collections.Generic;
using System.Linq;
namespace logviewer.logic.support
{
public unsafe class FixedSizeDictionary : IDictionary
{
private readonly int count;
private int[] indexes;
private T[] store;
public FixedSizeDictionary(int count)
{
this.count = count;
this.store = new T[count];
this.indexes = new int[count];
}
public IEnumerator> GetEnumerator()
{
for (var i = 0; i (i, this.store[i]);
}
}
}
IEnumerator IEnumerable.GetEnumerator()
{
return this.GetEnumerator();
}
public void Add(KeyValuePair item)
{
this.Add(item.Key, item.Value);
}
public void Clear()
{
this.store = new T[this.count];
this.indexes = new int[this.count];
}
public bool Contains(KeyValuePair item)
{
return this.indexes[item.Key] > 0 && Equals(this.store[item.Key], item.Value);
}
public void CopyTo(KeyValuePair[] array, int arrayIndex)
{
for (var i = arrayIndex; i (i, this.store[i]);
}
}
}
public bool Remove(KeyValuePair item)
{
return this.Remove(item.Key);
}
public int Count
{
get
{
var result = 0;
for (var i = 0; i false;
public bool ContainsKey(int key)
{
Solution
A big problem I see with either version is, that it doesn't behave like a
Examples about this point
-
Adding a
-
Accessing the
-
...
The
which is slow, or by a simple loop without the
as you see I have changed the loop condition by using the
You don't need for the
Currently your implementation is copying items until the array is full or until the end of the source array is reached, but with the sideeffect of the first point above.
Without knowing how you are using the posted dictionaries its hard to tell if the dictionaries are the bottleneck or if their usage is flawed.
Dictionary nor does it follow the documentation of the IDictionary.Examples about this point
-
Adding a
KeyValuePair twice with the same key is silently overwriting the value responding to the key. Your class should throw an ArgumentException for such a case.-
Accessing the
Item property should throw a KeyNotFoundException for any Tkey key which isn't in the dictionary.-
...
The
Count property of each version could be simplified either by using Linq likereturn this.indexes.Count(i => i > 0);which is slow, or by a simple loop without the
ContainsKeyInternal() call like sovar result = 0;
for (var i = 0; i 0)
{
result++;
}
}
return result;as you see I have changed the loop condition by using the
count field. Accessing the Length property of the array each time the condition is evaluated will take some time as well and you have the count field, so you should use it.You don't need for the
Keys and Values properties the call to the GetEnumerator() of your class and later restricting the result either to the Key or Value property of the returned KeyValuePair.
I would suggest to add two more methods IEnumerable GetKeysInternal() and IEnumerable GetValuesInternal() like so
public ICollection Keys
{
get {
return this.GetKeysInternal().ToArray();
}
}
private IEnumerable GetKeysInternal()
{
for (var i = 0; i 0)
{
yield return i;
}
}
}
public ICollection Values
{
get { return this.GetValuesInternal().ToArray(); }
}
private IEnumerable GetValuesInternal()
{
for (var i = 0; i 0)
{
yield return this.store[i];
}
}
}
Both versions have this
///
/// IMPORTANT: key out of range intentionally missed here due to performance reasons.
/// You shouldn't pass key that out of size range to avoid undefined behaviour
///
public bool TryGetValue(int key, out T value)
IMO a bool Try...() method should never ever throw an exception hence this xml doc isn't neccessary. A simple check for key >= count would be enough like so
public bool TryGetValue(int key, out T value)
{
if (key >= this.count || this.indexes[key] == 0)
{
value = default(T);
return false;
}
value = this.store[key];
return true;
}
The CopyTo() method has some more flaws.
-
If we take the CopyTo() method of the standard Dictionary<> implemented through the IDictionary<> interface we get an array returned where the found KeyValuePair's are stored in the beginning of the array. Your implementation stores the KeyValuePair's at its original positions. As a user of your class I wouldn't expect this.
-
If we pass a negativ arrayIndex we get a IndexOutOfRangeException from the private ContainsKeyInternal() method which means you are exposing implementation details of your class.
You should check the parameter and throw an ArgumentOutOfRangeException.
-
If the array isn't big enough to get all KeyValuePair's an ArgumentException` should be thrown stating that the array isn't big enough.Currently your implementation is copying items until the array is full or until the end of the source array is reached, but with the sideeffect of the first point above.
Without knowing how you are using the posted dictionaries its hard to tell if the dictionaries are the bottleneck or if their usage is flawed.
Code Snippets
return this.indexes.Count(i => i > 0);var result = 0;
for (var i = 0; i < count; i++)
{
if (this.indexes[i] > 0)
{
result++;
}
}
return result;public ICollection<int> Keys
{
get {
return this.GetKeysInternal().ToArray();
}
}
private IEnumerable<int> GetKeysInternal()
{
for (var i = 0; i < count; i++)
{
if (this.indexes[i] > 0)
{
yield return i;
}
}
}
public ICollection<T> Values
{
get { return this.GetValuesInternal().ToArray(); }
}
private IEnumerable<T> GetValuesInternal()
{
for (var i = 0; i < count; i++)
{
if (this.indexes[i] > 0)
{
yield return this.store[i];
}
}
}/// <remarks>
/// IMPORTANT: key out of range intentionally missed here due to performance reasons.
/// You shouldn't pass key that out of size range to avoid undefined behaviour
/// </remarks>
public bool TryGetValue(int key, out T value)public bool TryGetValue(int key, out T value)
{
if (key >= this.count || this.indexes[key] == 0)
{
value = default(T);
return false;
}
value = this.store[key];
return true;
}Context
StackExchange Code Review Q#158339, answer score: 2
Revisions (0)
No revisions yet.