Improve memory management, embrace modern .NET

[ericwj]

PR Details

Migrate away from IL weaving and take the first steps to improve memory management.

The mode of operation has generally been not to rewrite to safe code immediately.

Description

• Fixed a buffer overrun in ParameterCollection.CopyTo<T>.
• Identified a bug affecting the copy operation in ParameterCollection.UpdateLayout caused by parameters changing type and size after being initialized. Modified the copy operation to right-size them for every element in the collection, pending fixing of the bug causing the types to differ.
• Replaces uses of methods that are IL weaved with their C# equivalents. Rewrites Utilities marking most methods in it as obsolete, remove Interop.
  • In particular using Unsafe.CopyBlockUnaligned instead of Utilities.CopyMemory.
• Reviewed most uses of fixed
  • To verify that code around it throws instead of dereferencing null if the array being fixed is null or empty.
  • To elide checks on array.Length if the null result of fixed is intended when the array is null or empty.
• Add Debug.Assert in some places to verify correctness of CopyBlock and similar operations.
• Review all implementations of System.IO.Stream:
  • Add synchronous overloads for Read and Write that take Span<byte> as argument.
  • Fix the exception behavior to match the documentation of System.IO.Stream; no longer throw InvalidOperationException or EndOfStreamException, but use NotSupportedException and IOException.
  • Obsolete and remove NativeStream and NativeMemoryStream from the inheritance graph of Strides Stream types.
    • Obsolete related types such as NativeStreamWrapper.
    • SerializationStream.NativeStream is of type Stream, no longer NativeStream and is obsoleted in favor of the new property UnderlyingStream.
    • Use UnmanagedMemoryStream to replace all usage of NativeMemoryStream in Stride.
    • BlobStream now inherits from UnmanagedMemoryStream instead of from NativeMemoryStream.
  • Polyfill the virtual methods in NativeStream with extension methods. These are obsolete themselves since they are inefficient.
• Obsolete BinarySerialization on grounds that it is inefficent and a very minor benefit to Stride users.
• Added handle types to the P/Invoke declarations of Navigation.
• Many methods dealing with primitives and e.g. image data have had their type constraints changed from struct to unmanaged.
• Minor improvements in performance such as vector-enabled SequenceEqual over CompareMemory, correctness, code style and use of new language features in particular nint and nuint over IntPtr.

Related Issue

#1461

Motivation and Context

See issue for the motivation for starting this PR. Issues with buffer overruns which have surfaced are hard to fix without tracking of owned memory throughout the code. Hence the scope of this PR is broadened to include basic memory management.

Types of changes

• [x] ~Docs change~ / refactoring / ~dependency upgrade~
• [x] Bug fix (non-breaking change which fixes an issue) Access violations have been reported and there are hidden buffer overruns which may or may not be separate of those.
• [x] New feature (non-breaking change which adds functionality) The changes in types and inheritance hierarchy are binary breaking changs, but will be source-compatible.
• [x] Breaking change (fix or feature that would cause existing functionality to change)
  • [x] Public API affected by the described changes have been marked obsolete and will now generate warnings.

Did not make it into this PR

• [x] Many formal declarations using nint, IntPtr, void* or other unmanaged pointers will be obsoleted in favor or overloads that consistently carry length information, such as Memory<T> and Span<T>.
• [x] Use of arrays in signatures, fields and implementations may be (further) discouraged and obsoleted.

Checklist

• [x] This work in progress has completed.
• [x] My change requires a change to the documentation.
  • [x] Documentation comments have been modified and added.
• [x] I have added tests to cover my changes.
• [x] All new and existing tests passed. All tests succeed, except the following. 3 test projects fail to run due to test infrastructure failures. 1 test project fails all tests due to expecting packages for samples in $env:USERPROFILE\.nuget\packages which don't exist.

[xen2]

Great change! (that I meant to do for a long time to remove yet another AssemblyProcessor dependency but never got around to do)

One important point though: I was thinking, shouldn't we directly use the underlying Unsafe methods rather than writing yet another wrapper on it?

It would make the code more direct, standard & easy to understand, esp. for people familiar with recent high-perf C# (no need to check how our low-level Utilities/CoreUtilities map to yet another low-level Unsafe call).

That's what I initially had in mind when I saw all the new recently Unsafe equivalent to our Utilities class.

[ericwj]

I was thinking, shouldn't we directly use the underlying Unsafe methods rather than writing yet another wrapper on it?

Ideally yes but there are AV's, this is perhaps a halfway measure to have some checks (right now also but sometimes). Since Stride usually works, can also leave it for now and come back to the AV's later getting rid of AllocHGlobal and using a better, verifyable allocation strategy.

[ericwj]

I had Span<T>.CopyTo in a fair few cases. But testing it the first time it was clear it wasn't going to work.

[ericwj]

Checked it twice, don't see anything wrong - most changes are mundane. Yet I can't start GameStudio anymore - when it sais Loading scene opening Starbreach there is an AV.

[ericwj]

I have updated the title and the description to reflect the current plan as discussed with @xen2.

@manio143 would you review please as I go?

[manio143]

Please note that Vulkan builds are failing - VS by default only builds for D3D11 and you may need to modify sources\targets\Stride.props and set <StrideDefaultGraphicsApi>Vulkan</StrideDefaultGraphicsApi> on line 21 to have VS build Vulkan config for you.

[ericwj]

"Works on my machine."

[ericwj]

I changed this:

  <!-- Default GraphicsApi -->
  <PropertyGroup>
    <StrideGraphicsApis Condition="('$(StrideGraphicsApis)' == '' Or '$(StrideGraphicsApiDependentBuildAll)' == 'true') And ('$(TargetFramework)' == '$(StrideFramework)' Or '$(TargetFramework)' == '$(StrideFrameworkWindows)')">Direct3D11;Direct3D12;OpenGL;OpenGLES;Vulkan</StrideGraphicsApis>

    <StrideDefaultGraphicsApi Condition="'$(StrideGraphicsApis)' != ''">$(StrideGraphicsApis.Split(';', StringSplitOptions.RemoveEmptyEntries)[0])</StrideDefaultGraphicsApi>
    <StrideDefaultGraphicsApi Condition="'$(TargetFramework)' == '$(StrideFrameworkUWP)'">Direct3D11</StrideDefaultGraphicsApi>
    <StrideDefaultGraphicsApi Condition="'$(TargetFramework)' == '$(StrideFrameworkAndroid)'">OpenGLES</StrideDefaultGraphicsApi>
    <StrideDefaultGraphicsApi Condition="'$(TargetFramework)' == '$(StrideFrameworkiOS)'">OpenGLES</StrideDefaultGraphicsApi>
    <StrideDefaultGraphicsApi>Vulkan</StrideDefaultGraphicsApi>
  </PropertyGroup>

Stride builds but Starbreach gives me this:

Rebuild started...
Failed to restore C:\Operations\LargeFolders\Starbreach\Starbreach\Starbreach.csproj (in 605 ms).
Failed to restore C:\Operations\LargeFolders\Starbreach\VFXPackage\VFXPackage.csproj (in 605 ms).
Failed to restore C:\Operations\LargeFolders\Starbreach\Starbreach.Windows\Starbreach.Windows.csproj (in 1.04 sec).
1>------ Rebuild All started: Project: VFXPackage, Configuration: Debug Any CPU ------
NuGet package restore failed. Please see Error List window for detailed warnings and errors.
1>C:\Operations\LargeFolders\Starbreach\VFXPackage\VFXPackage.csproj : error NU1202: Package Stride.VirtualReality 4.1.0.1 is not compatible with net6.0 (.NETCoreApp,Version=v6.0). Package Stride.VirtualReality 4.1.0.1 does not support any target frameworks.
1>C:\Operations\LargeFolders\Starbreach\VFXPackage\VFXPackage.csproj : error NU1202: Package Stride.Games 4.1.0.1 is not compatible with net6.0 (.NETCoreApp,Version=v6.0). Package Stride.Games 4.1.0.1 does not support any target frameworks.
1>C:\Operations\LargeFolders\Starbreach\VFXPackage\VFXPackage.csproj : error NU1202: Package Stride.Input 4.1.0.1 is not compatible with net6.0 (.NETCoreApp,Version=v6.0). Package Stride.Input 4.1.0.1 does not support any target frameworks.
1>Done building project "VFXPackage.csproj" -- FAILED.
2>------ Rebuild All started: Project: Starbreach, Configuration: Debug Any CPU ------
2>C:\Operations\LargeFolders\Starbreach\Starbreach\Starbreach.csproj : error NU1202: Package Stride.Video 4.1.0.1 is not compatible with net6.0 (.NETCoreApp,Version=v6.0). Package Stride.Video 4.1.0.1 does not support any target frameworks.
2>C:\Operations\LargeFolders\Starbreach\Starbreach\Starbreach.csproj : error NU1202: Package Stride.VirtualReality 4.1.0.1 is not compatible with net6.0 (.NETCoreApp,Version=v6.0). Package Stride.VirtualReality 4.1.0.1 does not support any target frameworks.
2>C:\Operations\LargeFolders\Starbreach\Starbreach\Starbreach.csproj : error NU1202: Package Stride.Games 4.1.0.1 is not compatible with net6.0 (.NETCoreApp,Version=v6.0). Package Stride.Games 4.1.0.1 does not support any target frameworks.
2>C:\Operations\LargeFolders\Starbreach\Starbreach\Starbreach.csproj : error NU1202: Package Stride.Input 4.1.0.1 is not compatible with net6.0 (.NETCoreApp,Version=v6.0). Package Stride.Input 4.1.0.1 does not support any target frameworks.
2>Done building project "Starbreach.csproj" -- FAILED.
3>------ Rebuild All started: Project: Starbreach.Windows, Configuration: Debug Any CPU ------
3>C:\Operations\LargeFolders\Starbreach\Starbreach.Windows\Starbreach.Windows.csproj : error NU1202: Package Stride.Video 4.1.0.1 is not compatible with net6.0-windows7.0 (.NETCoreApp,Version=v6.0). Package Stride.Video 4.1.0.1 does not support any target frameworks.
3>C:\Operations\LargeFolders\Starbreach\Starbreach.Windows\Starbreach.Windows.csproj : error NU1202: Package Stride.VirtualReality 4.1.0.1 is not compatible with net6.0-windows7.0 (.NETCoreApp,Version=v6.0). Package Stride.VirtualReality 4.1.0.1 does not support any target frameworks.
3>Done building project "Starbreach.Windows.csproj" -- FAILED.
========== Rebuild All: 0 succeeded, 3 failed, 0 skipped ==========

[xen2]

I think you just want to add <StrideGraphicsApiDependentBuildAll>true</StrideGraphicsApiDependentBuildAll> to build/Stride.Build.props. If you specify them manually, you want to use StrideGraphicsApis (with a s) and have both Direct3D11 (mandatory) and the one you want, separated by semi-colons. Everything should happen in build/Stride.Build.props (ideally we should have a .user file also on .gitignore so that it's more clear it should be the one to edit).

[ericwj]

Yes now I get proper errors. Obsiously I have used Find All References and not regex :wink:

Would be great to use multi-targeting for that so that stuff compiles regardless of host machine.

[ericwj]

Grr

[ericwj]

Could just as well bring up now that the .NET SDK is broken also wrt the code analysis. Besides importantly the default defines not working.

It started just working when I was fiddling with the build system and skipping swaths of it. But the number of warnings is absolutely massive, making it almost useless for that reason. Still perhaps enabling some of these rules to catch some pervasive patterns I encounter would be a much better solution than using #warning and [Obsolete(DiagnosticID = ...)].

[ericwj]

Documented the changes in this PR in the description, since the intent is to merge and continue in a future PR.

[xen2]

I didn't have the time to go through every single change yet, but from what I saw so far it's a great PR! It simplifies a lot of code and put it much more in line with recent C#! (esp. unsafe and lot of custom workaround we had)