Skip to content

Commit ee7b6bb

Browse files
authored
[Java.Interop] Use lock+Dictionary, not ConcurrentDictionary (#791)
Context: dotnet/android#5455 Context: #791 (comment) Context: https://devblogs.microsoft.com/oldnewthing/20201112-00/?p=104444 Context: https://www.drdobbs.com/cpp/avoid-calling-unknown-code-while-inside/202802983 We would like to further reduce the `.apk` size of Xamarin.Android's `samples/HelloWorld` sample, and an *unnecessary contribution* to `.apk` size is the use of [`ConcurrentDictionary<TKey, TValue>`][0] within `Java.Interop.JniRuntime`. Replacing `ConcurrentDictionary<TKey, TValue>` with a `lock` + `Dictionary<TKey, TValue>` saves ~9.5KB in IL, by dropping the `System.Collections.Concurrent.dll` reference: Size difference in bytes ([*1] apk1 only, [*2] apk2 only): + 22 assemblies/Java.Interop.dll - 316 assemblies/System.Private.CoreLib.dll - 702 assemblies/System.Linq.dll - 8,471 assemblies/System.Collections.Concurrent.dll *1 Summary: - 9,467 Assemblies -1.32% (of 719,277) - 15,872 Uncompressed assemblies -1.06% (of 1,491,456) Additionally, `ConcurrentDictionary<TKey, TValue>` is *slower* than `lock` + `Dictionary` for infrequently contested locks (8cdb16d). Finally, fix a potential deadlock: it is *inadvisable* to invoke "unknown code" from within a lock, as you don't know what that code will do -- it could *call back into* the method holding the lock! Address this theoretical deadlock by updating `JniRuntime.ClearTrackedReferences()` to "capture" all of the `IDisposable` instances that need to be disposed within the lock, but waiting until we're *outside* the lock to execute them. This avoids possible deadlock. [0]: https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2?redirectedfrom=MSDN&view=net-5.0
1 parent 52082e2 commit ee7b6bb

File tree

1 file changed

+16
-8
lines changed

1 file changed

+16
-8
lines changed

src/Java.Interop/Java.Interop/JniRuntime.cs

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
#nullable enable
22

33
using System;
4-
using System.Collections.Concurrent;
54
using System.Collections.Generic;
65
using System.Diagnostics;
76
using System.Linq;
@@ -168,7 +167,7 @@ public static void SetCurrent (JniRuntime newCurrent)
168167
current = newCurrent;
169168
}
170169

171-
ConcurrentDictionary<IntPtr, IDisposable> TrackedInstances = new ConcurrentDictionary<IntPtr, IDisposable> ();
170+
Dictionary<IntPtr, IDisposable> TrackedInstances = new Dictionary<IntPtr, IDisposable> ();
172171

173172
JavaVMInterface Invoker;
174173
bool DestroyRuntimeOnDispose;
@@ -403,21 +402,30 @@ public int WeakGlobalReferenceCount {
403402

404403
internal void Track (JniType value)
405404
{
406-
TrackedInstances.TryAdd (value.PeerReference.Handle, value);
405+
lock (TrackedInstances) {
406+
if (!TrackedInstances.ContainsKey (value.PeerReference.Handle))
407+
TrackedInstances [value.PeerReference.Handle] = value;
408+
}
407409
}
408410

409411
internal void UnTrack (IntPtr key)
410412
{
411-
TrackedInstances.TryRemove (key, out var _);
413+
lock (TrackedInstances) {
414+
if (TrackedInstances.ContainsKey (key))
415+
TrackedInstances.Remove (key);
416+
}
412417
}
413418

414419
void ClearTrackedReferences ()
415420
{
416-
foreach (var k in TrackedInstances.Keys.ToList ()) {
417-
if (TrackedInstances.TryRemove (k, out var d))
418-
d.Dispose ();
421+
List<IDisposable> values;
422+
lock (TrackedInstances) {
423+
values = new List<IDisposable> (TrackedInstances.Values);
424+
TrackedInstances.Clear ();
419425
}
420-
TrackedInstances.Clear ();
426+
427+
foreach (var d in values)
428+
d.Dispose ();
421429
}
422430

423431
public virtual bool ExceptionShouldTransitionToJni (Exception e)

0 commit comments

Comments
 (0)