3
\$\begingroup\$

So i have a ConcurrentDictionary<int, string>

The "int" is merely the way i store the key, which i want more recommendations on.

Currently i do like this.

        public string memoize(Func<string, uint, string> functor, string code, uint id)
    {
        //Used for the lookup to identify if it's been inputed before
        int codeHash = code.GetHashCode();
        string functor_return;


        if (_compilerCache.TryGetValue(codeHash, out functor_return))
            return functor_return;


        functor_return = functor?.Invoke(code, id);
        _compilerCache[codeHash] = functor_return;

        return functor_return;
    }

Why do i use GetHashCode()? I don't know, it's just the only way i could figure out rather than store the string value itself.

I also save this "Cache", and hence why the compression and key is important.

    //Serialize and Compress object to file
    public static void SerializeObject<T>(string filename, T obj)
    {
        using (Stream stream = File.Open(filename, FileMode.Create))
        using (var cStream = new GZipStream(stream, CompressionLevel.Optimal))
        {
            BinaryFormatter binaryFormatter = new BinaryFormatter();
            binaryFormatter.Serialize(cStream, obj);
        }
    }

    //DeSerialize and Decompress object from file
    public static T DeSerializeObject<T>(string filename)
    {
        T objectToBeDeSerialized = default(T);
        try
        {
            using (Stream stream = File.Open(filename, FileMode.Open))
            using (var cStream = new GZipStream(stream, CompressionMode.Decompress))
            {
                BinaryFormatter binaryFormatter = new BinaryFormatter();
                objectToBeDeSerialized = (T)binaryFormatter.Deserialize(cStream);
            }

        }
        catch (Exception e)
        {
            MessageBox.Show(e.Message + " : Error DeSerializing Cache, corrupted?");
        }

        return objectToBeDeSerialized;
    }

It all works, but i would like to improve it. I don't really know if even GetHashCode is a good way to do this thing, i just took it randomly as before i just stored the value itself.

The values are usually around 10-20 characters i guess, usually very similar (which should be compression friendly i guess).

So can anything be done to improve this?

\$\endgroup\$
2
  • \$\begingroup\$ Have you experienced any memory or performance issues or why are you trying to optimize it to the last bit? About how many items do we speak? A thousand or tens of millions or more? \$\endgroup\$
    – t3chb0t
    Nov 20, 2016 at 13:41
  • \$\begingroup\$ some hundredthousands probably. \$\endgroup\$
    – Zerowalker
    Nov 20, 2016 at 21:07

2 Answers 2

3
\$\begingroup\$

GetHashCode is used internally by ConcurrentDictionary to speed up access to the real key.

But, there is no guarantee that 2 different strings will return 2 different HashCode, you may encounter collisions! Such collisions are handled by the ConcurrentDictionary.

So with your code, as the _compilerCache is a Dictionary<int,functor> (it seems to be) and not a Dictionary<string,functor>, you may encounter rare events where two different codes will give the same key. If such a collision happends, you have a subtle and rare bug.

I suggest the following :

  • stick to a Dictionary<string,functor>, use the real key

  • don't input a HashCode instead of the real key

  • you may also input your own GetHashCode in Dictionary constructor

Using code instead of int as a key has a price : memory footprint is larger, but with small strings (10-20 char), it should not be an issue.

Hope this helps !

\$\endgroup\$
4
  • \$\begingroup\$ What do you mean using code instead of int as a key? Why i use int was simply cause of the "key" right? The thing i want to memioze returns a String and inputs a String. So basically "string conversion" if you like. Feels like i am doing something wrong? \$\endgroup\$
    – Zerowalker
    Nov 20, 2016 at 17:54
  • \$\begingroup\$ Sorry I wasn't so clear in my answer. What I means by code was from your code => string code \$\endgroup\$
    – thesyndarn
    Nov 20, 2016 at 18:25
  • \$\begingroup\$ So i should just use my input string as the key? \$\endgroup\$
    – Zerowalker
    Nov 20, 2016 at 21:07
  • \$\begingroup\$ That was the idea : your input string should be the real key, the GetHashCode function may cause collisions. \$\endgroup\$
    – thesyndarn
    Nov 21, 2016 at 0:12
0
\$\begingroup\$

I only have a few comments.

Instead of using the type Func<string, uint, string>, consider defining a delegate type. This will allow you to give a name to this type as well as its parameters.

For your SerializeObject and DeSerializeObject methods:

The method names SerializeObject and DeSerializeObject are fine, but they don't tell me that they use the filesystem or data compression. If you wanted to make this fact clear, you could name the methods CompressObjectToFile and DecompressObjectFromFile.

When writing the word "deserialize" in title case, write it as "Deserialize", with a lowercase S, not as "DeSerialize", with an uppercase S. The word "deserialize" is only a single word, after all.

Consider not doing any exception handling in the DeSerializeObject. The problem with doing exception handling there is that if I'm calling DeSerializeObject, I'm not given any choice about how I want to handle errors. If you remove the exception handling, then I can handle the errors however I want.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.