Skip to content
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.

Sign up for GitHub

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

Ujjwalchadha/fix value type caching 2 #980

Merged
merged 5 commits into from
Aug 31, 2021
Merged

Ujjwalchadha/fix value type caching 2 #980

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
81f1a30
revert incorrect Fix for value type caching (#932)
ujjwalchadha Aug 27, 2021
eb466fe
Added conditionalweaktable reference to native value type
ujjwalchadha Aug 28, 2021
21539f0
Refactor
ujjwalchadha Aug 31, 2021
b482ddf
Moved cache to ComWrappersSupport
ujjwalchadha Aug 31, 2021
7bd5513
Added cache factory for String and type
ujjwalchadha Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 64 additions & 56 deletions src/Tests/UnitTest/TestComponentCSharp_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
using Windows.Security.Cryptography.Core;
using System.Reflection;
using Windows.Devices.Enumeration.Pnp;

#if NET
using WeakRefNS = System;
#else
Expand All @@ -50,11 +50,11 @@ public class TestCSharp
public TestCSharp()
{
TestObject = new Class();
}

}

// Test a fix for a bug in Mono.Cecil that was affecting the IIDOptimizer when it encountered long class names
[Fact]
[Fact]
public void TestLongClassNameEventSource()
{
bool flag = false;
Expand Down Expand Up @@ -744,10 +744,10 @@ public void TestCustomProjections()
Assert.Equal("name", propertyName);

bool eventCalled = false;
TestObject.CanExecuteChanged += (object sender, EventArgs e) =>
{
eventCalled = true;
};
TestObject.CanExecuteChanged += (object sender, EventArgs e) =>
{
eventCalled = true;
};

TestObject.RaiseCanExecuteChanged();
Assert.True(eventCalled);
Expand All @@ -761,8 +761,8 @@ public void TestCustomProjections()
// Ensure robustness with bad runtime class names (parsing errors, type not found, etc)
var badRuntimeClassName = Class.BadRuntimeClassName;
Assert.NotNull(badRuntimeClassName);
}
}

[Fact]
public void TestKeyValuePair()
{
Expand Down Expand Up @@ -2566,8 +2566,8 @@ public void TestEventSourceCaching()
GC.WaitForPendingFinalizers();
Assert.False(weakClassInstance.TryGetTarget(out _));
Assert.False(weakEventHandlerClass.TryGetTarget(out _));
}

}
class EventHandlerClass
{
private readonly Action eventCalled;
Expand All @@ -2580,6 +2580,58 @@ public EventHandlerClass(Action eventCalled)
public void IntPropertyChanged(object sender, int e) => eventCalled();
}

[Fact]
private async Task TestPnpPropertiesInLoop()
{
for (int i = 0; i < 10; i++)
{
await TestPnpPropertiesAsync();
}
}

private async Task TestPnpPropertiesAsync()
{
var requestedDeviceProperties = new List<string>()
{
"System.Devices.ClassGuid",
"System.Devices.ContainerId",
"System.Devices.DeviceHasProblem",
"System.Devices.DeviceInstanceId",
"System.Devices.Parent",
"System.Devices.Present",
"System.ItemNameDisplay",
"System.Devices.Children",
};
var devicefilter = "System.Devices.Present:System.StructuredQueryType.Boolean#True";
var presentDevices = (await PnpObject.FindAllAsync(PnpObjectType.Device, requestedDeviceProperties, devicefilter).AsTask().ConfigureAwait(false)).Select(pnpObject => {
var prop = pnpObject.Properties;
// Iterating through each key is necessary for this test even though we do not use each key directly
// This makes it more probable for a native pointer to get repeated and a value type to be cached and seen again.
foreach (var key in prop.Keys)
{
var val = prop[key];
if (key == "System.Devices.ContainerId" && val != null)
{
var val4 = pnpObject.Properties[key];
if (val is not Guid || val4 is not Guid)
{
throw new Exception("Incorrect value type Guid. Actual type: " + val.GetType() + " " + val4.GetType());
}
}
if (key == "System.Devices.Parent" && val != null)
{
var val4 = pnpObject.Properties[key];
if (val is not string || val4 is not string)
{
throw new Exception("Incorrect value type string Actual type: " + val.GetType() + " " + val4.GetType());
}
}

}
return pnpObject;
}).ToList();
}

#if NET
[TestComponentCSharp.Warning] // NO warning CA1416
class WarningManaged { };
Expand Down Expand Up @@ -2628,49 +2680,5 @@ private void TestSupportedOSPlatformWarnings()
WarningStatic.WarningEvent += (object s, Int32 v) => { }; // warning CA1416
}
#endif

[Fact]
private async Task TestPnpPropertiesAsync()
{
var requestedDeviceProperties = new List<string>()
{
"System.Devices.ClassGuid",
"System.Devices.ContainerId",
"System.Devices.DeviceHasProblem",
"System.Devices.DeviceInstanceId",
"System.Devices.Parent",
"System.Devices.Present",
"System.ItemNameDisplay",
"System.Devices.Children",
};
var devicefilter = "System.Devices.Present:System.StructuredQueryType.Boolean#True";
var presentDevices = (await PnpObject.FindAllAsync(PnpObjectType.Device, requestedDeviceProperties, devicefilter).AsTask().ConfigureAwait(false)).Select(pnpObject => {
var prop = pnpObject.Properties;
// Iterating through each key is necessary for this test even though we do not use each key directly
// This makes it more probable for a native pointer to get repeated and a value type to be cached and seen again.
foreach (var key in prop.Keys)
{
var val = prop[key];
if (key == "System.Devices.ContainerId" && val != null)
{
var val4 = pnpObject.Properties[key];
if (val is not Guid || val4 is not Guid)
{
throw new Exception("Incorrect value type Guid");
}
}
if (key == "System.Devices.Parent" && val != null)
{
var val4 = pnpObject.Properties[key];
if (val is not string || val4 is not string)
{
throw new Exception("Incorrect value type string");
}
}

}
return pnpObject;
}).ToList();
}
}
}
29 changes: 6 additions & 23 deletions src/WinRT.Runtime/ComWrappersSupport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,11 +280,10 @@ private static Func<IInspectable, object> CreateNullableTFactory(Type implementa
ParameterExpression[] parms = new[] { Expression.Parameter(typeof(IInspectable), "inspectable") };
var createInterfaceInstanceExpression = Expression.New(helperType.GetConstructor(new[] { typeof(ObjectReference<>).MakeGenericType(vftblType) }),
Expression.Call(parms[0],
typeof(IInspectable).GetMethod(nameof(IInspectable.As)).MakeGenericMethod(vftblType)));

var getValueExpression = Expression.Convert(Expression.Property(createInterfaceInstanceExpression, "Value"), typeof(object));
var setValueInWrapperexpression = Expression.New(typeof(ValueTypeWrapper).GetConstructor(new[] { typeof(object) }), getValueExpression);
return Expression.Lambda<Func<IInspectable, object>>(setValueInWrapperexpression, parms).Compile();
typeof(IInspectable).GetMethod(nameof(IInspectable.As)).MakeGenericMethod(vftblType)));

return Expression.Lambda<Func<IInspectable, object>>(
Expression.Convert(Expression.Property(createInterfaceInstanceExpression, "Value"), typeof(object)), parms).Compile();
}

private static Func<IInspectable, object> CreateArrayFactory(Type implementationType)
Expand All @@ -297,9 +296,8 @@ private static Func<IInspectable, object> CreateArrayFactory(Type implementation
Expression.Call(parms[0],
typeof(IInspectable).GetMethod(nameof(IInspectable.As)).MakeGenericMethod(vftblType)));

var getValueExpression = Expression.Property(createInterfaceInstanceExpression, "Value");
var setValueInWrapperexpression = Expression.New(typeof(ValueTypeWrapper).GetConstructor(new[] { typeof(object) }), getValueExpression);
return Expression.Lambda<Func<IInspectable, object>>(setValueInWrapperexpression, parms).Compile();
return Expression.Lambda<Func<IInspectable, object>>(
Expression.Property(createInterfaceInstanceExpression, "Value"), parms).Compile();
}

internal static Func<IInspectable, object> CreateTypedRcwFactory(string runtimeClassName)
Expand Down Expand Up @@ -728,20 +726,5 @@ internal InspectableInfo(Type type, Guid[] iids)
}

}

internal class ValueTypeWrapper
{
private readonly object value;

public ValueTypeWrapper(object value)
{
this.value = value;
}

public object Value
{
get => this.value;
}
}
}
}
31 changes: 21 additions & 10 deletions src/WinRT.Runtime/ComWrappersSupport.net5.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,25 +92,24 @@ private static T CreateRcwForComObject<T>(IntPtr ptr, bool tryUseCache)
var flags = tryUseCache ? CreateObjectFlags.TrackerObject : CreateObjectFlags.TrackerObject | CreateObjectFlags.UniqueInstance;
var rcw = ComWrappers.GetOrCreateObjectForComInstance(ptr, flags);
CreateRCWType.Value = null;

// Resurrect IWinRTObject's disposed IObjectReferences, if necessary
if (rcw is IWinRTObject winrtObj)
{
winrtObj.Resurrect();
}

// Because .NET will de-duplicate strings and WinRT doesn't,
// our RCW factory returns a wrapper of our string instance.
// This ensures that ComWrappers never sees the same managed object for two different
// native pointers. We unwrap here to ensure that the user-experience is expected
// and consumers get a string object for a Windows.Foundation.IReference<String>.
// We need to do the same thing for System.Type because there can be multiple WUX.Interop.TypeName's
// for a single System.Type.

// Resurrect IWinRTObject's disposed IObjectReferences, if necessary
if (rcw is IWinRTObject winrtObj)
{
winrtObj.Resurrect();
}

return rcw switch
{
ABI.System.Nullable<string> ns => (T)(object)ns.Value,
ABI.System.Nullable<string> ns => (T)(object)ns.Value,
ABI.System.Nullable<Type> nt => (T)(object)nt.Value,
ValueTypeWrapper vt => (T) vt.Value,
T castRcw => castRcw,
_ when tryUseCache => CreateRcwForComObject<T>(ptr, false),
_ => throw new ArgumentException(string.Format("Unable to create a wrapper object. The WinRT object {0} has type {1} which cannot be assigned to type {2}", ptr, rcw.GetType(), typeof(T)))
Expand Down Expand Up @@ -433,8 +432,13 @@ private static unsafe bool IsRuntimeImplementedRCW(Type objType)
return isRcw;
}

// This is used to hold the reference to the native value type object (IReference) until the actual value in it (boxed as an object) gets cleaned up by GC
// This is done to avoid pointer reuse until GC cleans up the boxed object
private static ConditionalWeakTable<object, IObjectReference> valueTypeRefHolder = new ConditionalWeakTable<object, IObjectReference>();
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved

private static object CreateObject(IObjectReference objRef)
{
Console.WriteLine("Creating object");
ujjwalchadha marked this conversation as resolved.
Show resolved Hide resolved
if (objRef.TryAs<IInspectable.Vftbl>(out var inspectableRef) == 0)
{
IInspectable inspectable = new IInspectable(inspectableRef);
Expand All @@ -446,7 +450,14 @@ private static object CreateObject(IObjectReference objRef)
// we use the Inspectable wrapper directly.
return inspectable;
}
return ComWrappersSupport.GetTypedRcwFactory(runtimeClassName)(inspectable);

var obj = ComWrappersSupport.GetTypedRcwFactory(runtimeClassName)(inspectable);
var type = TypeNameSupport.FindTypeByNameCached(runtimeClassName);
if (type != null && (type.IsValueType || runtimeClassName.StartsWith("Windows.Foundation.IReferenceArray`1")))
ujjwalchadha marked this conversation as resolved.
Show resolved Hide resolved
ujjwalchadha marked this conversation as resolved.
Show resolved Hide resolved
{
valueTypeRefHolder.Add(obj, objRef);
}
return obj;
}
else if (objRef.TryAs<ABI.WinRT.Interop.IWeakReference.Vftbl>(out var weakRef) == 0)
{
Expand Down
12 changes: 10 additions & 2 deletions src/WinRT.Runtime/ComWrappersSupport.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ public static partial class ComWrappersSupport

internal static InspectableInfo GetInspectableInfo(IntPtr pThis) => UnmanagedObject.FindObject<ComCallableWrapper>(pThis).InspectableInfo;

// This is used to hold the reference to the native value type object (IReference) until the actual value in it (boxed as an object) gets cleaned up by GC
// This is done to avoid pointer reuse until GC cleans up the boxed object
private static ConditionalWeakTable<object, IObjectReference> valueTypeRefHolder = new ConditionalWeakTable<object, IObjectReference>();

public static T CreateRcwForComObject<T>(IntPtr ptr)
{
if (ptr == IntPtr.Zero)
Expand All @@ -38,6 +42,11 @@ public static T CreateRcwForComObject<T>(IntPtr ptr)
var inspectable = new IInspectable(identity);
string runtimeClassName = GetRuntimeClassForTypeCreation(inspectable, typeof(T));
runtimeWrapper = string.IsNullOrEmpty(runtimeClassName) ? inspectable : TypedObjectFactoryCache.GetOrAdd(runtimeClassName, className => CreateTypedRcwFactory(className))(inspectable);
var type = TypeNameSupport.FindTypeByNameCached(runtimeClassName);
ujjwalchadha marked this conversation as resolved.
Show resolved Hide resolved
if (type != null && (type.IsValueType || runtimeClassName.StartsWith("Windows.Foundation.IReferenceArray`1")))
{
valueTypeRefHolder.Add(runtimeWrapper, identity);
}
}
else if (identity.TryAs<ABI.WinRT.Interop.IWeakReference.Vftbl>(out var weakRef) == 0)
{
Expand Down Expand Up @@ -74,8 +83,7 @@ public static T CreateRcwForComObject<T>(IntPtr ptr)
{
ABI.System.Nullable<string> ns => (T)(object)ns.Value,
ABI.System.Nullable<Type> nt => (T)(object)nt.Value,
ValueTypeWrapper vt => (T)vt.Value,
_ => (T)rcw
_ => (T)rcw
};
}

Expand Down