search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.71k forks source link

System.Reflection.Metadata: Avoid allocations in custom attribute parser #16724

Open MichalStrehovsky opened 8 years ago

MichalStrehovsky commented 8 years ago

The custom attribute parser allocates an ImmutableArray for FixedArguments and NamedArguments.

We should explore options to parse these lazily and avoid the array allocation.

joshfree commented 8 years ago

@tmat

karelz commented 7 years ago

Next step: We need real scenario demonstrating the perf issue, so that we can measure the improvement and if it is worth the effort. Steps afterword: Design API.

Note: Roslyn scenario probably doesn't care says @tmat.

MichalStrehovsky commented 7 years ago

We need real scenario demonstrating the perf issue

This is about API consistency. The rest of the APIs in S.R.Metadata tries hard to avoid allocations (which is why we chose to use it in the CoreRT compiler) and this one doesn't even try.

If it's not fixed, we might end up just not using S.R.Metadata's parser in CoreRT and roll our own. It's not rocket science and other people can do it too. It's a question of whether S.R.Metadata wants to be a one stop solution for people who want to read metadata without allocations or not.

joelverhagen commented 3 years ago

Next step: We need real scenario demonstrating the perf issue, so that we can measure the improvement and if it is worth the effort. Steps afterword: Design API.

Regarding this comment, I think an additional factor is not just performance but also edge case handling. There exists badly formatting DLLs (I have found several on NuGet.org) that have corrupted array length attributes causing one of these allocations to exceed many GBs of memory, per https://github.com/dotnet/runtime/issues/57531.

I have been using System.Reflection.Metadata as a high performance DLL analysis tool and in nearly all places it processes DLLs with very low memory consumption. This problem appears to be an outlier from an API consistency standpoint.

steveharter commented 1 year ago

There exists badly formatting DLLs (I have found several on NuGet.org) that have corrupted array length attributes causing one of these allocations to exceed many GBs of memory, per https://github.com/dotnet/runtime/issues/57531.

In https://github.com/dotnet/runtime/issues/57531 I don't believe that the length was corrupted in the various assemblies, but that the code parsing the assemblies using S.R.Metadata did not properly look up the underlying Enum types (int, long, etc). Enum references are not expressed in metadata in a nice way where you can easily get the length -- you have to load the referenced type and inspect it.

steveharter commented 1 year ago

If it's not fixed, we might end up just not using S.R.Metadata's parser in CoreRT and roll our own

@MichalStrehovsky I realize this is a super old issue, but do have any updates on its usage (NativeAOT)?

MichalStrehovsky commented 1 year ago

@MichalStrehovsky I realize this is a super old issue, but do have any updates on its usage (NativeAOT)?

We use the existing APIs that allocate ImmutableArrays