7
\$\begingroup\$

I am in the need to cache Bitmap's in a memory-optimized way because the API I am building will need to process many colored Bitmap's in parallel and can be used in x86 or x64 compiled applications.

If the API is being used in x86 I can't just store the Bitmap's as they are but need to store them as compressed MemoryStream's otherwise the API would throw an OutOfMemoryException pretty fast.

After storing the "Bitmap's" they will be processed by multiple threads hence thread-safety is a major point.

Any feedback is welcome.

public static class ImageCache
{
    private static int currentId = 0;
    private static readonly object addImageLock = new object();
    private static readonly object releaseImageLock = new object();
    private static readonly ConcurrentDictionary<int, MemoryStream> images = new ConcurrentDictionary<int, MemoryStream>();

    /// <summary>
    /// Release an image based on its id.
    /// </summary>
    /// <param name="id"></param>
    public static void ReleaseIamge(int id)
    {
        lock (releaseImageLock)
        {
            ReleaseMemoryStream(id);
        }
    }

    private static void ReleaseMemoryStream(int id)
    {
        MemoryStream ms = null;
        if (images.TryGetValue(id, out ms) && ms != null)
        {
            images[id].Dispose();
            images[id] = null;
        }
    }

    /// <summary>
    /// Releases all Images
    /// </summary>
    public static void ReleaseAllImages()
    {
        lock (releaseImageLock)
        {
            lock (addImageLock)
            {
                foreach (var id in images.Keys)
                {
                    ReleaseMemoryStream(id);
                }
                images.Clear();
            }
        }
    }

    /// <summary>
    /// Returns a Bitmap from the cache which is identified by an id
    /// </summary>
    /// <param name="id"></param>
    /// <returns></returns>
    public static Bitmap GetBitmap(int id)
    {
        lock (releaseImageLock)
        {
            MemoryStream ms = null;
            if (images.TryGetValue(id, out ms))
            {
                if (ms != null)
                {
                    return (Bitmap)Image.FromStream(ms);
                }
            }
        }
        return null;
    }

    /// <summary>
    /// Adds an Bitmap to the cache
    /// </summary>
    /// <param name="bitmap"></param>
    /// <returns>0 if the Bitmap is null, otherwise a uique id</returns>
    public static int Add(Bitmap bitmap)
    {

        if (bitmap == null)
        {
            return 0;
        }

        var ms = new MemoryStream();
        bitmap.Save(ms, ImageFormat.Tiff);
        var id = 0;
        lock (addImageLock)
        {
            // If the dictionary is empty we can reset the currentId
            if (images.Count == 0)
            {
                currentId = 0;
                id = Interlocked.Increment(ref currentId);
                images.TryAdd(id, ms);
                return id;
            }

            // We don't know how long an application using this is running and how many
            // images having been stored but we don't want to reach int.MaxValue here
            // hence we recycle the Value of a KeyValuePair of the dictionary if the Value
            // will be null.
            id = images.Where(item => item.Value == null).FirstOrDefault().Key;
            if (id == 0)
            {
                id = Interlocked.Increment(ref currentId);
            }

            images[id] = ms;

        }
        return id;
    }
}

Clarifying comments:

Why don't you use a Guid as id and key in the dictionary?

Well the problem with this is that I need to "mimic" an API we use for our main application in such a way that I can interchange this API and the other API. The other api is a comercial imaging sdk whichs licence doesn't permits the use of it for a sdk/api.

\$\endgroup\$
  • \$\begingroup\$ Why don't you use a Guid as id and key in the dictionary? \$\endgroup\$ – Henrik Hansen Oct 4 '18 at 16:35
  • 1
    \$\begingroup\$ @HenrikHansen I have clarified this in the question right now. \$\endgroup\$ – Heslacher Oct 4 '18 at 16:46
  • 2
    \$\begingroup\$ If you are going to exchage it later... why don't you implement it via an interface to easily use another implementation with dependency injection? \$\endgroup\$ – t3chb0t Oct 4 '18 at 17:20
  • \$\begingroup\$ @t3chb0t that I will do later. \$\endgroup\$ – Heslacher Oct 5 '18 at 4:24
  • \$\begingroup\$ Since you mention images, x86 and out-of-memory concerns... are you taking precautions to deal with LOH fragmentation? \$\endgroup\$ – Pieter Witvoet Oct 5 '18 at 8:26
9
\$\begingroup\$

First I'm not a fan of recycling IDs. Too much confusion can happen. For example Thread 1 adds a Dog Image and gets ID 1. Thread 2 calls ReleaseAllImages and then adds a Cat image and gets ID 1. Now Thread 1 think it has a Dog image but instead returns back a Cat image. There are times we need to recycle ID but only you know the business requirements if it's truly needed or not.

Second It's thread safe but you are not getting the benefit of ConcurrentDictionary this way. Everything is locked. If two threads are trying to add one will be blocked until the other is free. It doesn't have to be this way.

I'm going to go down the path that you need to recycle IDs

Instead of leaving the record in the dictionary you can remove it and have a queue of ID to recycle. Then on the removing methods check if we removed all the records from the Dictionary and if so then enter the lock to see if we can reset the counter and queue. Going to use ReaderWriterLockSlim to allow multiple adds and only block when we are removing.

public static class ImageCache
{
    private static int currentId = 0;

    private static readonly ConcurrentDictionary<int, MemoryStream> images =
        new ConcurrentDictionary<int, MemoryStream>();

    // stores removed ID to reuse
    private static ConcurrentQueue<int> recycle = new ConcurrentQueue<int>();
    private static readonly ReaderWriterLockSlim recycleLocker = new ReaderWriterLockSlim();

    /// <summary>
    /// Release an image based on its id.
    /// </summary>
    /// <param name="id"></param>
    public static void ReleaseIamge(int id)
    {
        ReleaseMemoryStream(id);
        TryReset();
    }

    /// <summary>
    /// Releases all Images
    /// </summary>
    public static void ReleaseAllImages()
    {
        foreach (var id in images.Keys)
        {
            ReleaseMemoryStream(id);
        }

        TryReset();
    }

    private static void ReleaseMemoryStream(int id)
    {
        MemoryStream ms = null;
        if (images.TryRemove(id, out ms))
        {
            recycle.Enqueue(id);
            ms.Dispose();
        }
    }

    private static void TryReset()
    {
        if (!images.IsEmpty)
        {
            return;
        }

        //need to lock here
        if (recycleLocker.TryEnterWriteLock(TimeSpan.FromMilliseconds(100)))
        {
            try
            {
                // make sure another thread didn't sneak in and add another image
                if (images.IsEmpty)
                {
                    currentId = 0;
                    Interlocked.Exchange(ref recycle, new ConcurrentQueue<int>());
                }
            }
            finally
            {
                recycleLocker.ExitWriteLock();
            }
        }
    }

GetBitmap method doesn't need to check if the image is null now we can use the TryGetValue and not need to lock at all

/// <summary>
/// Returns a Bitmap from the cache which is identified by an id
/// </summary>
/// <param name="id"></param>
/// <returns></returns>
public static Bitmap GetBitmap(int id)
{
    MemoryStream ms = null;
    if (images.TryGetValue(id, out ms))
    {
        return (Bitmap) Image.FromStream(ms);
    }
    return null;
}

Add method will need to let the lock know we are reading from the recycle queue

/// <summary>
/// Adds an Bitmap to the cache
/// </summary>
/// <param name="bitmap"></param>
/// <returns>0 if the Bitmap is null, otherwise a uique id</returns>
public static int Add(Bitmap bitmap)
{
    if (bitmap == null)
    {
        return 0;
    }

    recycleLocker.EnterReadLock();

    try
    {
        var ms = new MemoryStream();
        bitmap.Save(ms, ImageFormat.Tiff);

        // Recycle Id or make new one
        int id;
        if (!recycle.TryDequeue(out id))
        {
            id = Interlocked.Increment(ref currentId);
        }

        // this should never be possible to fail
        images.TryAdd(id, ms);

        return id;
    }
    finally 
    { 
        recycleLocker.ExitReadLock();
    }
}

Using the ReaderLockerSlim will allow multiple reads but only one write. Now multiple threads could Add images and Get images without blocking each other and the code only blocks when it's resetting the counters.

\$\endgroup\$
  • \$\begingroup\$ Thanks for that. I will take a good look at it tomorrow. \$\endgroup\$ – Heslacher Oct 4 '18 at 15:43
6
\$\begingroup\$

Spell-check: ReleaseIamge should presumably be ReleaseImage; uique should be unique.


    private static void ReleaseMemoryStream(int id)
    {
        MemoryStream ms = null;
        if (images.TryGetValue(id, out ms) && ms != null)
        {
            images[id].Dispose();
            images[id] = null;
        }
    }

It's not entirely clear to me why images[id] = null; is not images.Remove(id); (or perhaps images.TryRemove(id, out _);).


            // If the dictionary is empty we can reset the currentId
            if (images.Count == 0)
            {
                currentId = 0;

This is too clever for my taste. Why not move the currentId = 0 to ReleaseAllImages()? (Obviously that is dependent on not implementing my previous suggestion or my next suggestion).


            id = images.Where(item => item.Value == null).FirstOrDefault().Key;

Ugh. Linear scan (and inside a lock, at that). This could be replaced with a constant-time operation using a linked list of IDs which are available for recycling.

\$\endgroup\$
  • \$\begingroup\$ Thanks for that. I will take a good look at it tomorrow. \$\endgroup\$ – Heslacher Oct 4 '18 at 15:43
6
\$\begingroup\$

I wrote this a few hours; hopefully I've removed everything that was overlapping too much with the other answers

Add(Bitmap bitmap)

I also really don't like this, and not just because it's inefficient:

images.Where(item => item.Value == null).FirstOrDefault().Key;

It took me a short while to release why this won't crash (because everything happens to be a struct here with a convenient default value), and it heavily obscures the intention. I'd be inclined to stuff this logic in a well-named method; or rather, see the other answers for a better idea (store recycled ids separately: then you can get rid of those annoying nulls you have to keep checking for; personally I'd use a ConcurrentStack rather than a ConcurrentQueue or linked-list).

I'd like to second Peter Taylor's suggestion that you move currentId = 0 to ReleaseAllImages: it will remove the extra check and code-duplication in Add, and hopefully encourage id somewhere tighter in the logic, so that it isn't sitting around with a made-up-meaningful value waiting to be returned by accident.

Threading

The combination of locks and ConcurrentDictionary looks like it should be safe. (though not immediately obvious, I think the ConcurrentDictionary is necessary for this implementation of ReleaseAllImages() because it relies on the specific enumerator - which doesn't complain when you change things - rather than anything threading related)

There is no reason to use Interlocked.Increment on currentId, as the locks already imply a memory barrier, but I guess it probably doesn't do any harm.

The lock names are not very helpful. Maybe change addImageLock to addAndRemoveLock (it stops you trying to add/remove from the dictionary at the same time), and removeImageLock to readLock (it stops you reading elements from the dictionary simultaneously).

You could also replace both with a single ReaderWriterLock, as already said in CharlesNRice's answer. The main benefit would be the opportunity for concurrent calls to GetImage, which may be long-running and potentially CPU-intensive (but then again, might just ruin your memory characteristics).

API

  • It's not clear to the consumer whose job it is to dispose the Bitmap objects. The external API talks about 'images': a consumer could be forgiven for thinking that the cache simply holds bitmaps and disposes them when they are 'released'. This needs documenting... somehow.

  • There is no logic to defend against looking up an already released Id (a big problem with simple id recycling like this, rather than using managed (object) handles)

  • I don't understand why this is static... ReleaseAll might make sense only for a subset of the images cached by a process, so I'd sooner this was not static. If it is to mimic an existing static API, then just throw a static instance down somewhere, and everyone will be happy.

  • I like that 0 is not a valid id; it's such a common made-up-meaningful value that making it crash violently would make me even more happy.

Misc

  • Nice to see inline doc on the public members, thought it could be more comprehensive: GetBitmap, for example, fails to mention that it will return null if the image is not-found (or indeed 0).

  • You're a bit inconsistent with your if (images.TryGetValue(id, out ms) && ms != null) checks; it would be better if they all looked the same.

\$\endgroup\$
1
+50
\$\begingroup\$

It seems that the reuse of the ids causes some problems. You write that you expect "many" entries, but do you have a more precise estimate of what "many" means in respect to application run/life time. In other words: is it even possible to reach int.MaxValue taken the expected traffic compared to the duration of a running session? For instance: if you expect 2 bitmaps per second, how many years will it take to reach max?

\$\endgroup\$
  • 1
    \$\begingroup\$ You are absolutely right. Well defensive coding should be done carefully ;-) Good catch! On my computer I can process arround 700 Images/minute (loading from file, passing as bitmap, storing as memorystream and disposing) so it would take 2130 days to reach int.MaxValue. \$\endgroup\$ – Heslacher Oct 5 '18 at 6:19

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.