HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpMinor

Fixed size dictionary to achieve performance goals

Submitted by: @import:stackexchange-codereview··
0
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)
{

Solution

A big problem I see with either version is, that it doesn't behave like a 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 like

return this.indexes.Count(i => i > 0);


which is slow, or by a simple loop without the ContainsKeyInternal() call like so

var 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.