vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
454 stars 72 forks source link

Reflection Problems for Trimming #209

Closed ds5678 closed 5 months ago

ds5678 commented 6 months ago

When publishing an application with trimming enabled, this library executes differently. I suspect the issue is caused by its use of reflection in a few key places, like VertexUtils.

Static Abstract properties in interfaces is likely the most elegant solution to the problem, in addition to providing a performance boost. However, it is only available on .NET 6 and above. Is dropping .NET Standard a palatable option, or do I need to find another solution when making my pull request?

vpenades commented 6 months ago

For now I still need to keep supporting down to NetStandard2.0 because some users are using this library in plugins for platforms that target net4.7.1 , so changing the interfaces is not an option.

recently, I did some changes to the vertexutils with attribute decoration that are supposed to fix the issue; could you try cloning the repo and using the latest source code and see if the issue remains?

If the issue persist, I can look further into the issue

ds5678 commented 6 months ago

I did notice your improvements, especially relating to _JsonArray.

There remains other issues to resolve:

ILC : Trim analysis warning IL2075: SharpGLTF.Geometry.VertexTypes.VertexUtils.GetVertexAttributes(IVertexBuilder,Int32,PackedEncoding): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicFields' in call to 'System.Type.GetFields()'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

ILC : Trim analysis warning IL2077: SharpGLTF.Schema2.ExtensionsFactory.Create(JsonSerializable,String): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicConstructors' in call to 'System.Activator.CreateInstance(Type,BindingFlags,Binder,Object[],CultureInfo)'. The field 'System.ValueTuple`3<String,Type,Type>.Item3' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

There is a third warning, but I don't think it should have any significant impact.

ILC : Trim analysis warning IL2075: SharpGLTF.Validation.ModelException._CreateBaseMessage(JsonSerializable,String): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperty(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

vpenades commented 6 months ago

I'll look into them

vpenades commented 6 months ago

I've pushed some changes that might solve the remaining trimming issues.

For some odd reason I don't understand, the analyzers don't expose the issues you mentioned, so I checked them blindly.

So, could you check the latest changes?

ds5678 commented 6 months ago

So, could you check the latest changes?

I looked them over, and they appear fine. However, due to my build process, it's difficult for me to test without you posting a NuGet release.

the analyzers don't expose the issues you mentioned

On .NET 6, you can enable the analyzers with <IsTrimmable>true</IsTrimmable>.

On .NET 8, additional analyzers can be enabled with <IsAotCompatible>true</IsAotCompatible>.

https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming

https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/?tabs=net8plus

ds5678 commented 6 months ago

could you try cloning the repo and using the latest source code and see if the issue remains?

I did, and unfortunately I still get exceptions at runtime:

System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at Internal.Runtime.CompilerHelpers.ThrowHelpers.ThrowIndexOutOfRangeException() + 0x24
   at SharpGLTF.Geometry.VertexTypes.VertexUtils.CreateVertexMemoryAccessors[TVertex](IReadOnlyList`1, PackedEncoding) + 0x165
   at SharpGLTF.Geometry.PackedPrimitiveBuilder`1.SetStridedVertices(IPrimitiveReader`1, PackedEncoding) + 0x3e
   at SharpGLTF.Geometry.PackedMeshBuilder`1.Create(IMeshBuilder`1, PackedEncoding, EncodingType, SceneBuilderSchema2Settings) + 0x332
   at SharpGLTF.Geometry.PackedMeshBuilder`1.<CreatePackedMeshes>d__0.MoveNext() + 0x1a8
   at System.Collections.Generic.List`1..ctor(IEnumerable`1) + 0xed
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1) + 0x3d
   at SharpGLTF.Schema2.Toolkit.CreateMeshes[TMaterial](ModelRoot, Converter`2, SceneBuilderSchema2Settings, IMeshBuilder`1[]) + 0x274
   at SharpGLTF.Scenes.Schema2SceneBuilder.AddGeometryResources(ModelRoot, IEnumerable`1, SceneBuilderSchema2Settings) + 0x368
   at SharpGLTF.Scenes.SceneBuilder.ToGltf2(IEnumerable`1, SceneBuilderSchema2Settings) + 0x64
   at SharpGLTF.Scenes.SceneBuilder.ToGltf2() + 0x40

and warnings at compile time:

ILC : Trim analysis warning IL2077: SharpGLTF.Schema2.ExtensionsFactory.Create(JsonSerializable,String): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicConstructors' in call to 'System.Activator.CreateInstance(Type,BindingFlags,Binder,Object[],CultureInfo)'. The field 'System.ValueTuple`3<String,Type,Type>.Item3' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

ILC : Trim analysis warning IL2075: SharpGLTF.Geometry.VertexTypes.VertexUtils.GetVertexAttributes(IVertexBuilder,Int32,PackedEncoding): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicFields' in call to 'System.Type.GetFields()'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

ILC : Trim analysis warning IL2075: SharpGLTF.Validation.ModelException._CreateBaseMessage(JsonSerializable,String): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperty(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

I have a potential solution:

interface IVertexGeometry
{
   FieldInfo[] GetFields();
}
interface IVertexMaterial
{
   FieldInfo[] GetFields();
}
interface IVertexSkinning
{
   FieldInfo[] GetFields();
}

struct VertexPosition : IVertexGeometry
{
   readonly FieldInfo[] IVertexGeometry.GetFields() => typeof(VertexPosition).GetFields();
}

This should avoid the trimming issues and allow you to remove the DynamicallyAccessedMembers guards you put in.

If ReadOnlySpan<T> is permissible, it can be optimized with static fields:

interface IVertexGeometry
{
   ReadOnlySpan<FieldInfo> GetFields();
}
struct VertexPosition : IVertexGeometry
{
   private static readonly FieldInfo[] _fields = typeof(VertexPosition).GetFields();
   readonly ReadOnlySpan<FieldInfo> IVertexGeometry.GetFields() => _fields;
}

If that's too verbose, the fields can be stored in a generic static class:

internal static class FieldCache<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)] T>
{
   private static readonly FieldInfo[] _fields = typeof(T).GetFields();
   public static ReadOnlySpan<FieldInfo> GetFields() => _fields;
}
struct VertexPosition : IVertexGeometry
{
   readonly ReadOnlySpan<FieldInfo> IVertexGeometry.GetFields() => FieldCache<VertexPosition>.GetFields();
}

What would you prefer?

vpenades commented 6 months ago

What keeps ticking me is that I am not seeing these warnings you mention, which is preventing me finding these problems early, and I have to fix them blindly.

I am still looking for alternatives to this problem, but ultimately if I don't find a solution, I'll have to bite the bullet and fix this in line with what you're proposing.

I also think part of the problem is that it is not possible to run the unit tests in AOT mode to expose these issues, and the only way to surface them is with proper AOT demo applications.

do you have a shareable proof of concept application for me to test?

ds5678 commented 6 months ago

What keeps ticking me is that I am not seeing these warnings you mention, which is preventing me finding these problems early, and I have to fix them blindly.

Something is strange, for lack of a better word, about your project. Whenever I build your project, I don't see these warnings, which is in line with your experience. I only see them when I publish my project.

However, for other projects, I see the warnings in the IDE. I think something in the SharpGLTF repository is suppressing these trim warnings.

I noticed something significant while investigating your code. I can get Aot warnings in the IDE with your project, which are new with .NET 8. Perhaps, the trim warnings were just disabled a long time ago by accident.

do you have a shareable proof of concept application for me to test?

You could add <PublishAot>true</PublishAot> to your example project. That would cover a lot of the code.

https://github.com/vpenades/SharpGLTF/blob/779ca4aca857ead48f5831a2a5a6de2918ba573f/examples/Example1/Example1.csproj

vpenades commented 6 months ago

Something is strange, for lack of a better word, about your project. Whenever I build your project, I don't see these warnings, which is in line with your experience. I only see them when I publish my project.

I am aware of that, normal builds don't show up these warnings, but rebuilds do, which is what I usually do to surface as many warnings as possible. Maybe another one is building in debug/release.

vpenades commented 5 months ago

I've figured out that a plain GetType call does not work even if you decorate a method/class with DynamicallyAccessedMembers

what it does work is to introduce this method into the struct:

#if NET6_0_OR_GREATER
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicFields)]
#endif
public Type GetVertexType() { return typeof(VertexPositionNormal); }   

So instead of calling GetType().GetFields() which does not work, now you would call GetVertexType().GetFields() which does work.

But then, the whole point of using attributes was to make it easy to define custom vertex formats. If now an additional weird method is required, it defeats the purporse of the attribute. And relying on these DynamicallyAccessedMembers feels weak to say the least. So I'll do some more research.

ds5678 commented 5 months ago

The bloat from #if NET6_0_OR_GREATER and #endif can be removed if you add a (nontransitive) reference to PolySharp and include this property:

<PolySharpIncludeRuntimeSupportedAttributes>true</PolySharpIncludeRuntimeSupportedAttributes>
vpenades commented 5 months ago

I've refactored things so now attribute information is retrieved using an interface. please, try it now and let me know if it works

ds5678 commented 5 months ago

I greatly appreciate this and will check as soon as possible.

ds5678 commented 5 months ago

Everything appears to be working on my end. I still get two warnings in my build log, but they don't seem to have any impact on my project.

I'm not using any extensions:

ILC : Trim analysis warning IL2077: SharpGLTF.Schema2.ExtensionsFactory.Create(JsonSerializable,String): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicConstructors' in call to 'System.Activator.CreateInstance(Type,BindingFlags,Binder,Object[],CultureInfo)'. The field 'System.ValueTuple`3<String,Type,Type>.Item3' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

A trimming issue at this location, at worst, could cause a different exception to be thrown:

ILC : Trim analysis warning IL2075: SharpGLTF.Validation.ModelException._CreateBaseMessage(JsonSerializable,String): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperty(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
ds5678 commented 5 months ago

I built with your recent commit for the extensions factory, and everything seems good. Thank you so much!