Fix generic arrays and dictionaries in .NET not calling set_typed

[juanjp600]

See #97948 (review), the changes made here are based on this feedback.

I wasn't able to find a way to completely avoid using reflection, but since ScriptManagerBridge does rely on it for a few things, I figured that I could generalize what it does use and get something that's still going to work with trimming.

Here are some highlights in this PR:

• CSharpScript::get_instance_base_type now returns a cached StringName that is passed in by Godot.Bridge.ScriptManagerBridge.UpdateScriptClassInfo. This reduces interop overhead and the use of reflection, which is important to speed up the godotsharp_*_set_typed functions.
• GodotObject.InternalGetClassNativeBase is no longer recursive. Also, it now performs a direct comparison with a reference to the GodotSharp assembly, and only performs a name comparison in the editor because getting an assembly's name has some overhead.
• Now there's a GodotObject.InternalGetClassNativeBaseName method. This method caches the StringNames it returns, to avoid always getting the static fields' values through reflection.

I benchmarked this PR using the following project: TypedCollectionBenchmarkDotnet.zip. I've made sure that it works on NativeAOT.

Here are my results:

JIT

BenchmarkDotNet v0.14.0, Pop!_OS 22.04 LTS
AMD Ryzen 5 3600, 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.110
  [Host] : .NET 8.0.10 (8.0.1024.46610), X64 RyuJIT AVX2

Method	Mean	Error	StdDev	Median
UntypedDictionary	859.7 ns	17.19 ns	43.75 ns	854.3 ns
TypedDictionaryWithNativeElements	1,938.9 ns	23.91 ns	26.57 ns	1,930.0 ns
TypedDictionaryWithScriptElements	2,177.0 ns	43.56 ns	102.67 ns	2,135.7 ns
UntypedArray	854.8 ns	25.56 ns	75.37 ns	829.7 ns
TypedArrayWithNativeElements	1,574.6 ns	31.54 ns	72.47 ns	1,555.6 ns
TypedArrayWithScriptElements	1,734.3 ns	34.38 ns	75.47 ns	1,707.4 ns

NativeAOT

BenchmarkDotNet v0.14.0, Pop!_OS 22.04 LTS
AMD Ryzen 5 3600, 1 CPU, 12 logical and 6 physical cores
  [Host] : .NET 8.0.10, X64 NativeAOT AVX2

Method	Mean	Error	StdDev
UntypedDictionary	1,053.0 ns	15.93 ns	14.12 ns
TypedDictionaryWithNativeElements	3,200.2 ns	63.78 ns	156.46 ns
TypedDictionaryWithScriptElements	3,419.6 ns	68.35 ns	191.66 ns
UntypedArray	725.2 ns	13.28 ns	11.77 ns
TypedArrayWithNativeElements	1,928.4 ns	37.92 ns	56.76 ns
TypedArrayWithScriptElements	2,102.2 ns	30.91 ns	28.92 ns

[a-johnston]

Overall I think these are good changes but it's a bummer the calls are so much slower. I'm assuming that untyped vs typed performance difference is similar when using gdscript? If _p->typed.validate is that expensive, it might be worth (in a different PR) adding internal unchecked_set methods and using those when calling from a fully typed gdscript or generic c# context.

[a-johnston]

now that there is more common logic, should these array and dictionary constructors generally use : this() { .. } or is it still worth checking/throwing prior?

[a-johnston]

it's existing code but imo it's a little weird that these end up with mutable externally provided underlying arrays, and as a result if array is not typed and not empty, we can't set typing on the native array. It's probably not worth shallow copying the input array to get around this, but it's a bummer that it won't be able to fix cases where we have a existing saved untyped array as in #98309. At least in that case the data just has to be regenerated. Maybe it would be worth changing set_typed to allow checking of the existing elements, although in this case it may also run against the refcount <= 1 constraint.

[juanjp600]

I personally think that the constructor should shallow copy, because right now you can have the odd situation where you have two collections that are not the same according to object.ReferenceEquals, but nonetheless refer to the same engine-side array and as such will observe changes made by each other.

However, changing that would be a breaking change with user-observable effects (IIRC this is relevant when there are multiple instances of the same scene) and at the very least I think it's not within scope for this PR.

Also worth noting is that were this constructor changed to shallow copy, the implementation of the Duplicate method should also change to remain as efficient as possible.

[a-johnston]

Yeah I agree it should be the behavior but given the current use it would be both a breaking change and make existing code paths of native -> c# array slower; maybe godot 5 lol? I think the current constructors with that behavior could be made internal and explicitly for cases like CreateTakingOwnershipOfDisposableValue, and then the public constructor making a shallow copy still benefits over the c# collection constructors since it knows it can make a single api call to duplicate rather than 2N calls to read/insert.

[raulsntos]

Thank you so much for this amazing work, and sorry it took me so long to review.

I appreciate how much work you put into this, trying to reduce the overhead as much as possible, benchmarking the changes, and ensuring NativeAOT works.

My only concern is with setting the type for the underlying array/dictionary in the constructors that "borrow" a reference to an untyped array/dictionary. I share the opinion that these constructors should shallow copy, but unfortunately that'd be a breaking change.

[raulsntos]

Do we need to copy the StringName?

[juanjp600]

I'm fairly certain that this is necessary. At the very least, it's consistent with how ScriptManagerBridge.GetScriptNativeName handles passing a managed StringName to unmanaged code.

If I'm understanding this correctly, it's effectively the same as an implicit call to StringName's copy constructor within C++. This is important to maintain a correct refcount; I tested what happens when a copy isn't made, and I get the following errors when I close the editor:

ERROR: BUG: Unreferenced static string to 0: EditorExportPlugin
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: HBoxContainer
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: EditorPlugin
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: MarginContainer
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: EditorInspectorPlugin
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: RefCounted
   at: unref (core/string/string_name.cpp:144)
ERROR: BUG: Unreferenced static string to 0: Node2D
   at: unref (core/string/string_name.cpp:144)

I suppose given the static lifetime of the StringNames returned by GodotObject.InternalGetClassNativeBaseName, it might be safe to pass a pointer to the underlying native form of that instead of constructing a new one, but I am not 100% sure about that so I went with this since I'm more confident that it's safe.

[raulsntos]

Yes, I think you are 100% correct. I was also thinking about passing a pointer to the underlying native form, but I agree it's safer to do the copy.

[raulsntos]

I'm guessing we don't expect this to ever create a script here, since if T is not a native class it must be a C# script so a script reference should already exist.

[juanjp600]

It is possible for the construction of a collection to trigger ScriptManagerBridge.CreateScriptBridgeForType. Here's an example project illustrating this: RecursiveReferenceThruArrays.zip

The stacktrace for such a call looks like this:

   at System.Environment.get_StackTrace()
   at Godot.Bridge.ScriptManagerBridge.CreateScriptBridgeForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 565
   at Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 469
   at Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForPath(godot_string* scriptPath, godot_string* c_backtrace, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 452
   at Godot.Bridge.ScriptManagerBridge.GetOrLoadOrCreateScriptForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 540
   at Godot.NativeInterop.Marshaling.GetTypedCollectionParameterInfo[T](Type& variantType, godot_string_name& className, godot_ref& script) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/Marshaling.cs:line 222
   at Godot.Collections.Array`1.SetTypedForUnderlyingArray() in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs:line 1065
   at Godot.Collections.Array`1..ctor() in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs:line 1102
   at ScriptNodeA.GetGodotPropertyDefaultValues() in /home/juan/GodotMrps/RecursiveReferenceThruArrays/Godot.SourceGenerators/Godot.SourceGenerators.ScriptPropertyDefValGenerator/ScriptNodeA_ScriptPropertyDefVal.generated.cs:line 15
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
   at Godot.Bridge.ScriptManagerBridge.GetPropertyDefaultValuesForType(Type type, IntPtr scriptPtr,  addDefValFunc) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 1125
   at Godot.Bridge.ScriptManagerBridge.GetPropertyDefaultValues(IntPtr scriptPtr,  addDefValFunc) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 1100
   at Godot.Bridge.ScriptManagerBridge.CreateScriptBridgeForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 572
   at Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 469
   at Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForPath(godot_string* scriptPath, godot_string* c_backtrace, godot_ref* outScript) in /***/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 452

I'm not aware of any issues that might arise in this scenario.

[raulsntos]

I see, thank you. I don't think there should be any issues with this.

[raulsntos]

Looks good to me, thanks!

[Repiteo]

Thanks!