Resolve some concurrency issues #1200

mattleibow · 2020-03-29T22:50:55Z

Description of Change

Resolve some concurrency issues. In some cases the construction and/or destruction overlap and this causes invalid states. This PR improves the locking to avoid this, but still preserves the read and write level of locking.

It also includes a few other improvements, such as making sure objects that require their "parent" object to not be GC'ed add a reference to it. In most cases it is simple, such as a canvas being owned by a surface. But, in the case of a pixmap, the ownership is a little different in that they are copies, but actually require the parent object to stick around. So, the ownership is "reversed".

There is also a few other tweaks to improve general GC reliability/interop performance.

Bugs Fixed

Fixes [BUG] macOS native crashes in gr_backendrendertarget_get_gl_framebufferinfo #1188
Resolves underlying issue for [BUG] Crashes in sk_canvas_restore_to_count on Mac 10.15 #1121 [BUG] Random iOS Crash GRBackendRenderTarget.GetGlFramebufferInfo() #910 [BUG] Crashes on Android #1173 [BUG] Random segfaults on Android #1168 [BUG] iOS app freezes randomly due to native crashes #1144
Resolves the underlying issue for the need for Re-work views to avoid incorrect disposal #1180

API Changes

None.

Behavioral Changes

Less crashes due to correct object management.

PR Checklist

Has tests (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Updated documentation

mattleibow · 2020-03-29T23:36:49Z

binding/Binding/HandleDictionary.cs

+		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 ();


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.

mattleibow · 2020-03-29T23:37:10Z

binding/Binding/HandleDictionary.cs

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


Caching on .NET Core is a major perf boost. Major as in 3-4x, but still in ns.

mattleibow · 2020-03-29T23:38:48Z

binding/Binding/HandleDictionary.cs

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


Anonymous delegates are cached rather than direct method references.

mattleibow · 2020-03-29T23:41:06Z

binding/Binding/SKObject.cs

+		internal ConcurrentDictionary<IntPtr, SKObject> OwnedObjects =>
+			ownedObjects ??= new ConcurrentDictionary<IntPtr, SKObject> ();


Lazy-instantiate this as it is hardly ever used. Typically for documents and some for some streams, codecs and typefaces.

mattleibow · 2020-03-29T23:42:19Z

tests/SkiaSharp.NetCore.Tests/SkiaSharp.NetCore.Tests.csproj

@@ -12,18 +12,13 @@
    <PackageReference Include="xunit" Version="2.4.1" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.4.1" />
    <PackageReference Include="XunitXml.TestLogger" Version="2.1.26" />
-    <PackageReference Include="xunit.assemblyfixture" Version="2.0.3" />


This never worked for .NET Core anyway. Use the xUnit-approved way from the xUnit samples.

ziriax

I might be missing something, but at first sight getting OwnedObjects and KeepAliveObjects is not thread-safe, and can suffer from a race condition?

You could use double checked locking:
https://en.m.wikipedia.org/wiki/Double-checked_locking

Or maybe just Interlocked.CompareExchange

I could be wrong, I just briefly looked at the code, maybe some higher level locking is performed

mattleibow · 2020-03-29T23:56:31Z

Thanks for the review!

Technically, the objects should only be referenced from a single thread at any given time. However, I will add some locking to prevent any future issues.

- Lazy<T> still allocates, so not useful - Technically not needed due to objects not being thread-safe

It is highly likely that some parent object provides a reference to another child object, but then the parent destroys and replaces the child. In this case, the child will not own the handle. If there is a managed object that owns its handle, then it should throw.

mattleibow · 2020-04-01T16:16:19Z

binding/Binding/SKBitmap.cs

-			return SkiaApi.sk_bitmap_peek_pixels (Handle, pixmap.Handle);
+			var result = SkiaApi.sk_bitmap_peek_pixels (Handle, pixmap.Handle);
+			if (result)
+				Referenced (pixmap, this);


The GC sometimes likes to collect the bitmap in use by a pixmap. This case is important because the bitmap OWNS the pixmap, and the reference is not counted. So a disposal destroys the pixmap.

mattleibow · 2020-04-01T16:16:56Z

binding/Binding/SKData.cs

@@ -251,6 +251,7 @@ public void SaveTo (Stream target)
 			} finally {
 				ArrayPool<byte>.Shared.Return (buffer);
 			}
+			GC.KeepAlive (this);


Just prevent the GC from going to town, because we are not actually using the object, but a value from one of the properties.

mattleibow · 2020-04-01T16:17:29Z

binding/Binding/SKObject.cs

-#if THROW_OBJECT_EXCEPTIONS
-		internal static readonly ConcurrentBag<Exception> exceptions = new ConcurrentBag<Exception> ();
-#endif
+		private readonly object locker = new object ();


A single locker as this doesn't need to be too complicated.

mattleibow · 2020-04-01T16:19:38Z

I think this is it. Fixed some other bugs. Concurrency due to construction and destruction, but also when the GC starts to collect things too soon.

mattleibow · 2020-04-02T08:56:34Z

I have pushed what I think is now a good version to the preview feed: https://aka.ms/skiasharp-eap/index.json
Check out the PR version: 1.68.2-pr.1200.20

Gillibald · 2020-04-02T11:25:25Z

Will have a look shortly 👍

mattleibow · 2020-04-02T20:10:26Z

@Gillibald have you been able to have a look and see if things are better? I am thinking of just merging and pushing to nuget.org so that the mapsui folks can check. But, I just want to make sure that I get your opinion/hands on the pie here.

Gillibald · 2020-04-02T20:15:40Z

Did not have time today. Could do tomorrow morning. In a few hours 😀. My current test creates and disposes millions of SKObjects. So minimal improvements should come out clearly. Mostly text related objects.

mattleibow · 2020-04-02T21:35:40Z

Ah, millions? Have you noticed any random crashes or weird behavior at all 😄

The issues that mostly come up is with concurrent execution because objects were being created with the same handle as an object being destroyed. As a result, the managed side got all mixed up as it was busy nuking things and then it is added back, or worse the reverse. Most of the cases coming now are when people use the GL views because that is usually for high frequency updates and often on separate threads.

So this would be interesting to see if you were seeing weird things, or even having to do some weird things to prevent weird things 😄

Gillibald · 2020-04-02T21:48:08Z

Luckily I only have to deal with single threaded processing. I might want to put text processing on a dedicated thread in the near future.

Gillibald · 2020-04-03T10:28:18Z

The preview runs just fine. After this is merged I will work on the changes to SKObject.GetObject
Using cached delegates instead of reflection should improve things a bit.

Do we really need to keep track of all SKObject instances? Ideally, we could just turn off caching.

mattleibow · 2020-04-03T11:23:05Z

We have to track them because if I call SKSurface.Canvas twice, I need to find it again. I can't really cache it directly, because some operations might destroy the canvas and then the object is destroyed in native code, but we still got a valid object in managed world. Sometimes we don't even know when it will happen, as it may be a result of another operation. It is easy from a developer's perspective because certain operations may do things, but to get it right it is going to be more difficult.

I know @kekekeks was asking for a pointer-only API: #1205

But, this is not to say we can't actually have a look at certain APIs and see if we can just store things in a field. For example, a surface will always have the same canvas, so no need to even go through the interop. But this is not true for SKBitmap.PeekPixels because a copy is made of the pixmap, and you can always change the underlying pixel pointer at time - and this will not reflect in the copied pixmap.

Overall, the slowest point of this library is the lookup. And this is made harder because some objects are reference counted, others are not. And then, some are reference counted, but not used like one. The API is not quite perfect.

mattleibow · 2020-04-03T11:26:09Z

Thanks for having a look. Merging this now so I can get a preview out.

mattleibow added 2 commits March 30, 2020 00:46

Rework the assembly fixtures

739c18b

Fix concurrency issue with handle registration

77778ec

mattleibow added the type/bug label Mar 29, 2020

Save several ms and useless allocations

23deee9

mattleibow commented Mar 29, 2020

View reviewed changes

mattleibow mentioned this pull request Mar 29, 2020

[BUG] macOS native crashes in gr_backendrendertarget_get_gl_framebufferinfo #1188

Closed

ziriax reviewed Mar 29, 2020

View reviewed changes

Lock the construction of the lazy dictionaries

e123a9a

- Lazy<T> still allocates, so not useful - Technically not needed due to objects not being thread-safe

This was referenced Mar 30, 2020

Remove reflection usage for most SKObject #1193

Closed

Crashes - and crash reports / stack traces (Forms) Mapsui/Mapsui#865

Closed

mattleibow added 16 commits March 30, 2020 17:15

A few tweaks

14a4215

Let's see what is happening on CI

a61f0b2

Add some more details

598adca

Merge branch 'master' into dev/fix-1188

ae2bb04

Tests should no longer need to be retried

c8ac92e

Not reliable

2e0358a

Order is variable

850d205

Make sure to correctly own objects

37ea41a

Go back and see if everything is suddenly working?

e59e5b7

Try this alone

260028e

Going crazy

65bdd97

Just check if something is being collected?

b01ff5e

Keep the bitmap alive when just using the pixmap

c2f6614

Start going back

053f3dc

Do it right

7e05532

mattleibow commented Apr 1, 2020

View reviewed changes

Cache the "static" font styles

a564f54

Merge branch 'master' into dev/fix-1188

c0df9bb

Use a field just in case the pixmap is reused

5e56e76

mattleibow closed this Apr 2, 2020

mattleibow reopened this Apr 2, 2020

mattleibow merged commit 14813f8 into master Apr 3, 2020

mattleibow deleted the dev/fix-1188 branch April 3, 2020 11:25

mattleibow added this to the v1.68.2 milestone Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve some concurrency issues #1200

Resolve some concurrency issues #1200

mattleibow commented Mar 29, 2020 •

edited

Loading

mattleibow Mar 29, 2020

mattleibow Mar 29, 2020 •

edited

Loading

mattleibow Mar 29, 2020

mattleibow Mar 29, 2020

mattleibow Mar 29, 2020

ziriax left a comment

mattleibow commented Mar 29, 2020

mattleibow Apr 1, 2020

mattleibow Apr 1, 2020

mattleibow Apr 1, 2020

mattleibow commented Apr 1, 2020

mattleibow commented Apr 2, 2020

Gillibald commented Apr 2, 2020

mattleibow commented Apr 2, 2020

Gillibald commented Apr 2, 2020

mattleibow commented Apr 2, 2020

Gillibald commented Apr 2, 2020

Gillibald commented Apr 3, 2020

mattleibow commented Apr 3, 2020 •

edited

Loading

mattleibow commented Apr 3, 2020

		internal ConcurrentDictionary<IntPtr, SKObject> OwnedObjects =>
		ownedObjects ??= new ConcurrentDictionary<IntPtr, SKObject> ();

Resolve some concurrency issues #1200

Resolve some concurrency issues #1200

Conversation

mattleibow commented Mar 29, 2020 • edited Loading

mattleibow Mar 29, 2020

Choose a reason for hiding this comment

mattleibow Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

mattleibow Mar 29, 2020

Choose a reason for hiding this comment

mattleibow Mar 29, 2020

Choose a reason for hiding this comment

mattleibow Mar 29, 2020

Choose a reason for hiding this comment

ziriax left a comment

Choose a reason for hiding this comment

mattleibow commented Mar 29, 2020

mattleibow Apr 1, 2020

Choose a reason for hiding this comment

mattleibow Apr 1, 2020

Choose a reason for hiding this comment

mattleibow Apr 1, 2020

Choose a reason for hiding this comment

mattleibow commented Apr 1, 2020

mattleibow commented Apr 2, 2020

Gillibald commented Apr 2, 2020

mattleibow commented Apr 2, 2020

Gillibald commented Apr 2, 2020

mattleibow commented Apr 2, 2020

Gillibald commented Apr 2, 2020

Gillibald commented Apr 3, 2020

mattleibow commented Apr 3, 2020 • edited Loading

mattleibow commented Apr 3, 2020

mattleibow commented Mar 29, 2020 •

edited

Loading

mattleibow Mar 29, 2020 •

edited

Loading

mattleibow commented Apr 3, 2020 •

edited

Loading