-
Notifications
You must be signed in to change notification settings - Fork 546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve some concurrency issues #1200
Changes from 3 commits
739c18b
77778ec
23deee9
e123a9a
14a4215
a61f0b2
598adca
ae2bb04
c8ac92e
2e0358a
850d205
37ea41a
86d19c2
e59e5b7
260028e
65bdd97
b01ff5e
c2f6614
053f3dc
7e05532
a564f54
c0df9bb
5e56e76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 (); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of pure locking and concurrent dictionaries, just use a reader/writer lock as that is what we actually want. And, when reading and we find that there is no handle, then we can upgrade in place. |
||
|
||
/// <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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anonymous delegates are cached rather than direct method references. |
||
|
||
// 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) { | ||
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 (); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caching on .NET Core is a major perf boost. Major as in 3-4x, but still in ns.