Skip to content

Commit

Permalink
Resolve some concurrency issues (#1200)
Browse files Browse the repository at this point in the history
* Rework the assembly fixtures
* Fix concurrency issue with handle registration
* Save several ms and useless allocations
* Make sure to correctly own objects
* Native objects may be disposed at any time
* Keep the bitmap alive when just using the pixmap
* Cache the "static" font styles
  • Loading branch information
mattleibow authored Apr 3, 2020
1 parent 234c93a commit 14813f8
Show file tree
Hide file tree
Showing 22 changed files with 821 additions and 200 deletions.
215 changes: 215 additions & 0 deletions binding/Binding/HandleDictionary.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Reflection;
using System.Threading;

namespace SkiaSharp
{
internal static class HandleDictionary
{
private static readonly Type IntPtrType = typeof (IntPtr);
private static readonly Type BoolType = typeof (bool);

#if THROW_OBJECT_EXCEPTIONS
internal static readonly ConcurrentBag<Exception> exceptions = new ConcurrentBag<Exception> ();
#endif

internal static readonly ConcurrentDictionary<Type, ConstructorInfo> constructors = new ConcurrentDictionary<Type, ConstructorInfo> ();
internal static readonly Dictionary<IntPtr, WeakReference> instances = new Dictionary<IntPtr, WeakReference> ();

internal static readonly ReaderWriterLockSlim instancesLock = new ReaderWriterLockSlim ();

/// <summary>
/// Retrieve the living instance if there is one, or null if not.
/// </summary>
/// <returns>The instance if it is alive, or null if there is none.</returns>
internal static bool GetInstance<TSkiaObject> (IntPtr handle, out TSkiaObject instance)
where TSkiaObject : SKObject
{
if (handle == IntPtr.Zero) {
instance = null;
return false;
}

instancesLock.EnterReadLock ();
try {
return GetInstanceNoLocks (handle, out instance);
} finally {
instancesLock.ExitReadLock ();
}
}

/// <summary>
/// Retrieve or create an instance for the native handle.
/// </summary>
/// <returns>The instance, or null if the handle was null.</returns>
internal static TSkiaObject GetObject<TSkiaObject, TSkiaImplementation> (IntPtr handle, bool owns = true, bool unrefExisting = true, bool refNew = false)
where TSkiaObject : SKObject
where TSkiaImplementation : SKObject, TSkiaObject
{
if (handle == IntPtr.Zero)
return null;

instancesLock.EnterUpgradeableReadLock ();
try {
if (GetInstanceNoLocks<TSkiaObject> (handle, out var instance)) {
// some object get automatically referenced on the native side,
// but managed code just has the same reference
if (unrefExisting && instance is ISKReferenceCounted refcnt) {
#if THROW_OBJECT_EXCEPTIONS
if (refcnt.GetReferenceCount () == 1)
throw new InvalidOperationException (
$"About to unreference an object that has no references. " +
$"H: {handle.ToString ("x")} Type: {instance.GetType ()}");
#endif
refcnt.SafeUnRef ();
}

return instance;
}

var type = typeof (TSkiaImplementation);
var constructor = constructors.GetOrAdd (type, t => GetConstructor (t));

// we don't need to go into a writable here as the object will do it in the Handle property
var obj = (TSkiaObject)constructor.Invoke (new object[] { handle, owns });
if (refNew && obj is ISKReferenceCounted toRef)
toRef.SafeRef ();
return obj;
} finally {
instancesLock.ExitUpgradeableReadLock ();
}

static ConstructorInfo GetConstructor (Type type)
{
var ctors = type.GetTypeInfo ().DeclaredConstructors;

foreach (var ctor in ctors) {
var param = ctor.GetParameters ();
if (param.Length == 2 && param[0].ParameterType == IntPtrType && param[1].ParameterType == BoolType)
return ctor;
}

throw new MissingMethodException ($"No constructor found for {type.FullName}.ctor(System.IntPtr, System.Boolean)");
}
}

/// <summary>
/// Retrieve the living instance if there is one, or null if not. This does not use locks.
/// </summary>
/// <returns>The instance if it is alive, or null if there is none.</returns>
private static bool GetInstanceNoLocks<TSkiaObject> (IntPtr handle, out TSkiaObject instance)
where TSkiaObject : SKObject
{
if (instances.TryGetValue (handle, out var weak) && weak.IsAlive) {
if (weak.Target is TSkiaObject match) {
if (!match.IsDisposed) {
instance = match;
return true;
}
#if THROW_OBJECT_EXCEPTIONS
} else if (weak.Target is SKObject obj) {
if (!obj.IsDisposed && obj.OwnsHandle) {
throw new InvalidOperationException (
$"A managed object exists for the handle, but is not the expected type. " +
$"H: {handle.ToString ("x")} Type: ({obj.GetType ()}, {typeof (TSkiaObject)})");
}
} else if (weak.Target is object o) {
throw new InvalidOperationException (
$"An unknown object exists for the handle when trying to fetch an instance. " +
$"H: {handle.ToString ("x")} Type: ({o.GetType ()}, {typeof (TSkiaObject)})");
#endif
}
}

instance = null;
return false;
}

/// <summary>
/// Registers the specified instance with the dictionary.
/// </summary>
internal static void RegisterHandle (IntPtr handle, SKObject instance)
{
if (handle == IntPtr.Zero || instance == null)
return;

SKObject objectToDispose = null;

instancesLock.EnterWriteLock ();
try {
if (instances.TryGetValue (handle, out var oldValue) && oldValue.Target is SKObject obj && !obj.IsDisposed) {
#if THROW_OBJECT_EXCEPTIONS
if (obj.OwnsHandle) {
// a mostly recoverable error
// if there is a managed object, then maybe something happened and the native object is dead
throw new InvalidOperationException (
$"A managed object already exists for the specified native object. " +
$"H: {handle.ToString ("x")} Type: ({obj.GetType ()}, {instance.GetType ()})");
}
#endif
// this means the ownership was handed off to a native object, so clean up the managed side
objectToDispose = obj;
}

instances[handle] = new WeakReference (instance);
} finally {
instancesLock.ExitWriteLock ();
}

// dispose the object we just replaced
objectToDispose?.DisposeInternal ();
}

/// <summary>
/// Removes the registered instance from the dictionary.
/// </summary>
internal static void DeregisterHandle (IntPtr handle, SKObject instance)
{
if (handle == IntPtr.Zero)
return;

instancesLock.EnterWriteLock ();
try {
var existed = instances.TryGetValue (handle, out var weak);
if (existed && (!weak.IsAlive || weak.Target == instance)) {
instances.Remove (handle);
} else {
#if THROW_OBJECT_EXCEPTIONS
InvalidOperationException ex = null;
if (!existed) {
// the object may have been replaced

if (!instance.IsDisposed) {
// recoverable error
// there was no object there, but we are still alive
ex = new InvalidOperationException (
$"A managed object did not exist for the specified native object. " +
$"H: {handle.ToString ("x")} Type: {instance.GetType ()}");
}
} else if (weak.Target is SKObject o && o != instance) {
// there was an object in the dictionary, but it was NOT this object

if (!instance.IsDisposed) {
// recoverable error
// there was a new living object there, but we are still alive
ex = new InvalidOperationException (
$"Trying to remove a different object with the same native handle. " +
$"H: {handle.ToString ("x")} Type: ({o.GetType ()}, {instance.GetType ()})");
}
}
if (ex != null) {
if (instance.fromFinalizer)
exceptions.Add (ex);
else
throw ex;
}
#endif
}
} finally {
instancesLock.ExitWriteLock ();
}
}
}
}
5 changes: 4 additions & 1 deletion binding/Binding/SKBitmap.cs
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,10 @@ public bool PeekPixels (SKPixmap pixmap)
if (pixmap == null) {
throw new ArgumentNullException (nameof (pixmap));
}
return SkiaApi.sk_bitmap_peek_pixels (Handle, pixmap.Handle);
var result = SkiaApi.sk_bitmap_peek_pixels (Handle, pixmap.Handle);
if (result)
pixmap.pixelSource = this;
return result;
}

// Resize
Expand Down
4 changes: 2 additions & 2 deletions binding/Binding/SKColorSpace.cs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public bool GetNumericalTransferFunction (out SKColorSpaceTransferFn fn)
// *XyzD50

public SKMatrix44 ToXyzD50 () =>
GetObject<SKMatrix44> (SkiaApi.sk_colorspace_as_to_xyzd50 (Handle), false);
OwnedBy (GetObject<SKMatrix44> (SkiaApi.sk_colorspace_as_to_xyzd50 (Handle), false), this);

public bool ToXyzD50 (SKMatrix44 toXyzD50)
{
Expand All @@ -175,7 +175,7 @@ public bool ToXyzD50 (SKMatrix44 toXyzD50)
}

public SKMatrix44 FromXyzD50 () =>
GetObject<SKMatrix44> (SkiaApi.sk_colorspace_as_from_xyzd50 (Handle), false);
OwnedBy (GetObject<SKMatrix44> (SkiaApi.sk_colorspace_as_from_xyzd50 (Handle), false), this);

//

Expand Down
1 change: 1 addition & 0 deletions binding/Binding/SKData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ public void SaveTo (Stream target)
} finally {
ArrayPool<byte>.Shared.Return (buffer);
}
GC.KeepAlive (this);
}

//
Expand Down
4 changes: 2 additions & 2 deletions binding/Binding/SKDocument.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ public void Abort () =>
SkiaApi.sk_document_abort (Handle);

public SKCanvas BeginPage (float width, float height) =>
GetObject<SKCanvas> (SkiaApi.sk_document_begin_page (Handle, width, height, null), false);
OwnedBy (GetObject<SKCanvas> (SkiaApi.sk_document_begin_page (Handle, width, height, null), false), this);

public SKCanvas BeginPage (float width, float height, SKRect content) =>
GetObject<SKCanvas> (SkiaApi.sk_document_begin_page (Handle, width, height, &content), false);
OwnedBy (GetObject<SKCanvas> (SkiaApi.sk_document_begin_page (Handle, width, height, &content), false), this);

public void EndPage () =>
SkiaApi.sk_document_end_page (Handle);
Expand Down
35 changes: 31 additions & 4 deletions binding/Binding/SKFontStyle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,19 @@ namespace SkiaSharp
{
public class SKFontStyle : SKObject
{
private static readonly Lazy<SKFontStyle> normal;
private static readonly Lazy<SKFontStyle> bold;
private static readonly Lazy<SKFontStyle> italic;
private static readonly Lazy<SKFontStyle> boldItalic;

static SKFontStyle()
{
normal = new Lazy<SKFontStyle> (() => new SKFontStyleStatic (SKFontStyleWeight.Normal, SKFontStyleWidth.Normal, SKFontStyleSlant.Upright));
bold = new Lazy<SKFontStyle> (() => new SKFontStyleStatic (SKFontStyleWeight.Bold, SKFontStyleWidth.Normal, SKFontStyleSlant.Upright));
italic = new Lazy<SKFontStyle> (() => new SKFontStyleStatic (SKFontStyleWeight.Normal, SKFontStyleWidth.Normal, SKFontStyleSlant.Italic));
boldItalic = new Lazy<SKFontStyle> (() => new SKFontStyleStatic (SKFontStyleWeight.Bold, SKFontStyleWidth.Normal, SKFontStyleSlant.Italic));
}

[Preserve]
internal SKFontStyle (IntPtr handle, bool owns)
: base (handle, owns)
Expand Down Expand Up @@ -37,12 +50,26 @@ protected override void DisposeNative () =>

public SKFontStyleSlant Slant => SkiaApi.sk_fontstyle_get_slant (Handle);

public static SKFontStyle Normal => new SKFontStyle (SKFontStyleWeight.Normal, SKFontStyleWidth.Normal, SKFontStyleSlant.Upright);
public static SKFontStyle Normal => normal.Value;

public static SKFontStyle Bold => new SKFontStyle (SKFontStyleWeight.Bold, SKFontStyleWidth.Normal, SKFontStyleSlant.Upright);
public static SKFontStyle Bold => bold.Value;

public static SKFontStyle Italic => new SKFontStyle (SKFontStyleWeight.Normal, SKFontStyleWidth.Normal, SKFontStyleSlant.Italic);
public static SKFontStyle Italic => italic.Value;

public static SKFontStyle BoldItalic => new SKFontStyle (SKFontStyleWeight.Bold, SKFontStyleWidth.Normal, SKFontStyleSlant.Italic);
public static SKFontStyle BoldItalic => boldItalic.Value;

private sealed class SKFontStyleStatic : SKFontStyle
{
internal SKFontStyleStatic (SKFontStyleWeight weight, SKFontStyleWidth width, SKFontStyleSlant slant)
: base (weight, width, slant)
{
IgnorePublicDispose = true;
}

protected override void Dispose (bool disposing)
{
// do not dispose
}
}
}
}
6 changes: 5 additions & 1 deletion binding/Binding/SKImage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,11 @@ public bool PeekPixels (SKPixmap pixmap)
{
if (pixmap == null)
throw new ArgumentNullException (nameof (pixmap));
return SkiaApi.sk_image_peek_pixels (Handle, pixmap.Handle);

var result = SkiaApi.sk_image_peek_pixels (Handle, pixmap.Handle);
if (result)
pixmap.pixelSource = this;
return result;
}

public SKPixmap PeekPixels ()
Expand Down
Loading

0 comments on commit 14813f8

Please sign in to comment.