3
\$\begingroup\$

For a game implementation, I have a very large overview map with multiple layers (namely base-map / units / highlight & info / fov-shading) of which the first is CPU-intensive to generate. I have implemented a caching mechanism in which the base layer is drawn once and held in system memory, then drawn on demand by the OnPaint method at the current scale. The subsequent layers are then drawn for the current clipping region:

private bool _isCalculating = false;

/// <inheritdoc/>
protected override async void OnPaint(PaintEventArgs e) {
  if(e==null) throw new ArgumentNullException("e");
  if (DesignMode) { e.Graphics.FillRectangle(Brushes.Gray, ClientRectangle);  return; }

  if (_isCalculating) return;

  if (BufferCache == null) {
    _isCalculating = true;
    BufferCache    = await PaintedCacheBufferAsync("Cache");
    _isCalculating = false;
    this.Refresh();

  } else {

    var mapScale = DataContext.Scales[ScaleIndex];
    var location = AutoScrollPosition + Margin.OffsetSize();

    BufferCache.Render(BufferMap, mapScale/CacheScale, location);

    BufferMap.Render(BufferUnits, mapScale, location, DataContext.Model.PaintUnits);

    BufferUnits.Render(BufferBack, mapScale, location, DataContext.Model.PaintHighlight);

    BufferShading.Render(BufferBack, mapScale, location, DataContext.Model.PaintShading);

    e.Graphics.DrawImageUnscaled(BufferBack, Point.Empty);
  }
}

Though it works and is reasonably performant, I am not completely happy with the above. In particular, comments are welcome on:

  • My obvious assumption about the thread-safety of isCalculating; and
  • The overall structure of the OnPaint method.

For completeness, here are the Extension Methods on Image and Bitmap that allow the code above to be fairly neat:

/// <summary>Renders the supplied <see cref="Image"/> <paramref name="source"/> to the specified
/// <see cref="Image"/> <paramref name="target"/>, scaled and translated.</summary>
/// <param name="source">Source <see cref="Image"/> to be rendered.</param>
/// <param name="target">Target <see cref="Graphics"/> to be rendered to.</param>
/// <param name="scale">Scale at which the source should be drawn</param>
/// <param name="point"><see cref="Point"/> at which to render the <paramref name="source"/>.</param>
public static void Render(this Image source, Image target, float scale, Point point) {
  Traces.Paint.Trace("Render source to {0}:",target.Tag);

  using (var gBuffer = Graphics.FromImage(target)) source.Render(gBuffer, scale, point);
}

/// <summary>Renders the supplied <see cref="Image"/> <paramref name="source"/> to the specified
/// <see cref="Graphics"/> <paramref name="target"/>, scaled and translated.</summary>
/// <param name="source">Source <see cref="Image"/> to be rendered.</param>
/// <param name="target">Target <see cref="Graphics"/> to be rendered to.</param>
/// <param name="scale">Scale at which the source should be drawn</param>
/// <param name="point"><see cref="Point"/> at which to render the <paramref name="source"/>.</param>
public static void Render(this Image source, Graphics target, float scale, Point point) {
  target.Clear(Color.Black);

  target.PageUnit = GraphicsUnit.Pixel;
  target.TranslateTransform(point.X, point.Y);
  target.ScaleTransform(scale, scale);

  target.InterpolationMode = InterpolationMode.HighQualityBicubic;
  target.DrawImage(source, Point.Empty);
}

/// <summary>Renders, scaled and translated, first the supplied <see cref="Image"/> <paramref name="source"/> to the specified
/// <see cref="Graphics"/> <paramref name="target"/> and then the specified <paramref name="action"/>.</summary>
/// <param name="source">Source <see cref="Image"/> to be rendered.</param>
/// <param name="target">Target <see cref="Graphics"/> to be rendered to.</param>
/// <param name="scale">Scale at which the source should be drawn</param>
/// <param name="point"><see cref="Point"/> at which to render the <paramref name="source"/>.</param>
/// <param name="action">The drawing action to be overlain to target.</param>
public static void Render(this Image source, Image target, float scale, Point point, Action<Graphics> action) {
  Traces.Paint.Trace("Render cache to {0}:",target.Tag);

  using (var gTarget = Graphics.FromImage(target)) {
    gTarget.DrawImageUnscaled(source, Point.Empty);;

    gTarget.PageUnit = GraphicsUnit.Pixel;
    gTarget.TranslateTransform(point.X, point.Y);
    gTarget.ScaleTransform(scale,scale);

    action(gTarget);
  }
}
\$\endgroup\$
3
\$\begingroup\$

Well, "thread-safety" is a fairly vague term. Read/Write access to basic data types like bool for example is guaranteed to be atomic so in that sense access to _isCalculating is thread-safe. However your method is not thread-safe in the general sense as in that if two threads call it at the same time they could very well both be passing the guard check and both be performing the expensive operation.

The call order would be:

  • Both threads check _isCalculating at the same time which is false and continue
  • Both threads check that BufferCache is null and continue
  • Both thread perform the expensive operation

In the general sense protecting entry into a function with a guard flag would have to utilize Interlocked.CompareExchange:

int _IsBusy = 0;

void ExpensiveFunction()
{
    if (Interlocked.CompareExchange(ref _IsBusy, 1, 0) == 1)
    {
        // flag is already set -> don't continue
        return;
    }
    try
    {
        // do this in a try-finally block to avoid being caught out by
        // unexpected exceptions keeping the flag set forever
        PerformExpensiveWork();
    }
    finally
    {
        Interlocked.Exchange(ref _IsBusy, 0);
    }
}

That being said, given that your method is called OnPaint I assume it's only ever going to be called from a UI thread and of that there is usually only one. So your current implementation will probably work but that's largely due to the fact that calls to OnPaint are going to be made from a single thread rather than your implementation being thread-safe.

One issue, as mentioned in the above example code, is that should the expensive operation (PaintedCacheBufferAsync in your case) throw in any way then you have locked yourself out because the _isCalculating flag will never be reset.

\$\endgroup\$
  • \$\begingroup\$ Ha! I forgot about the self-deadlock; thank you. I will look up the InterlockedExchange object also. \$\endgroup\$ – Pieter Geerkens Jan 21 '15 at 0:53
  • \$\begingroup\$ Thank you again; I did need to meld your suggestions with the original double-guard to be reliable, as shown in my alternate answer. \$\endgroup\$ – Pieter Geerkens Jan 21 '15 at 2:46
  • \$\begingroup\$ Found my error - improved solution posted below. \$\endgroup\$ – Pieter Geerkens Jan 21 '15 at 4:55
1
\$\begingroup\$

ChrisWue raised two valid points, but the implementation suggested in his answer is faulty - simply swirling the mouse over the expensive control as it painted the cache caused an InvalidOperationException in Image.cs. The following variation on his implementation works and repeatedly survives the swirl test: private int _isBusy = 0;

/// <inheritdoc/>
protected override async void OnPaint(PaintEventArgs e) {
  if(e==null) throw new ArgumentNullException("e");
  if (DesignMode) { e.Graphics.FillRectangle(Brushes.Gray, ClientRectangle);  return; }

#if FIRST_WAY  // both of these work fine
  if(Interlocked.CompareExchange(ref _isBusy, 1, 1) == 1) return;
#else
  if(_isBusy == 1) return;
#endif

  if (BufferCache == null) {
    try {
      Interlocked.Exchange(ref _isBusy, 1);
      BufferCache  = await PaintedCacheBufferAsync("Cache");
    } finally { Interlocked.Exchange(ref _isBusy,0); }
    this.Refresh();
  } else {
    var mapScale = DataContext.Scales[ScaleIndex];
    var location = AutoScrollPosition + Margin.OffsetSize();

    BufferCache.Render(BufferMap, mapScale/CacheScale, location);

    BufferMap.Render(BufferUnits, mapScale, location, DataContext.Model.PaintUnits);

    BufferUnits.Render(BufferBack, mapScale, location, DataContext.Model.PaintHighlight);

    BufferShading.Render(BufferBack, mapScale, location, DataContext.Model.PaintShading);

    e.Graphics.DrawImageUnscaled(BufferBack, Point.Empty);
  }
}

Update - this implementation is better:

private int _isBusy = 0;

/// <inheritdoc/>
protected override async void OnPaint(PaintEventArgs e) {
  if(e==null) throw new ArgumentNullException("e");
  if (DesignMode) { e.Graphics.FillRectangle(Brushes.Gray, ClientRectangle);  return; }

  if (BufferCache != null) {
    var mapScale = DataContext.Scales[ScaleIndex];
    var location = AutoScrollPosition + Margin.OffsetSize();

    BufferCache.Render(BufferMap, mapScale/CacheScale, location);

    BufferMap.Render(BufferUnits, mapScale, location, DataContext.Model.PaintUnits);

    BufferUnits.Render(BufferBack, mapScale, location, DataContext.Model.PaintHighlight);

    BufferShading.Render(BufferBack, mapScale, location, DataContext.Model.PaintShading);

    e.Graphics.DrawImageUnscaled(BufferBack, Point.Empty);
  } else if (Interlocked.CompareExchange(ref _isBusy, 1, 0) == 0) {
    try     { BufferCache  = await PaintedCacheBufferAsync("Cache"); }
    finally { Interlocked.Exchange(ref _isBusy, 0); }
    this.Refresh();
  }
}

This passes the swirl test. There is still a potential race condition between this code fragment and the one location where BufferCache is set (to null, when scale changes), but currently that only occurs in the control constructor. Addressing it fully would require full access locking I believe.

\$\endgroup\$
  • 1
    \$\begingroup\$ This seems weird. Your CompareExchange doesn't actually do anything. It will replace the value of _isBusy with 1 if the current value is 1. That if should be functionally equivalent to if (_isBusy == 1) return; as it stands. So effectively your current code is pretty similar to the original one except it's using Interlocked and protects against exceptions. \$\endgroup\$ – ChrisWue Jan 21 '15 at 3:37
  • \$\begingroup\$ @ChrisWue: Yes, it is sufficient to replace it with if(_isBusy == 1) return;. However the test if (BufferCache == null) { must follow that (verified empirically). \$\endgroup\$ – Pieter Geerkens Jan 21 '15 at 3:41
  • \$\begingroup\$ @ChrisWue: The two guards, _isBusy and BufferCache == null are inter-related, but I haven't wrapped my mind around just how yet; other than that this implementation works. \$\endgroup\$ – Pieter Geerkens Jan 21 '15 at 3:45
  • \$\begingroup\$ Proof again that lock-free programming is hard. \$\endgroup\$ – ChrisWue Jan 21 '15 at 8:47
  • \$\begingroup\$ Assign the BufferCache value to a local variable, proof that against null and if not work with the local stored reference. Now your race condition has gone \$\endgroup\$ – Sir Rufo Jun 17 '16 at 18:30

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.