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.

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

Expose a zero allocation API to Godot C# bindings #7842

Open
reduz opened this issue Sep 24, 2023 · 92 comments
Open

Expose a zero allocation API to Godot C# bindings #7842

reduz opened this issue Sep 24, 2023 · 92 comments

Comments

@reduz
Copy link
Member

reduz commented Sep 24, 2023

Describe the project you are working on

Godot C# bindings

Describe the problem or limitation you are having in your project

For the past weeks, I've been discussing with several Unity users intending to move to Godot C# regarding dealing with the C# garbage collector.

The most common complaint I hear from users is that, in Unity, allocations can trigger unexpected GC spikes into the game.

In Godot, we target to make all of the high performance APIs (those that intended to be called every frame) not allocate any memory, so theoretically the GC should not be a problem. Additionally, Godot starting from 4.0, uses the Microsoft CoreCLR version of .net, which also supposedly has a better garbage collector than Unity.

But in all, after several discussions with Unity users, neither is enough reassurance for them, and they would really feel safer if Godot exposed a zero allocation API.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The idea of this proposal is that Godot exposes zero allocation versions of many functions in the C# API, that users can use if they desire.

Technically, this could be done from the binding generator itself, without breaking compatibility, and without doing any modification to Godot itself.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

WARNING I am not familiar with C#, so take this as pseudocode.

Imagine you have two functions exposed as to C#:

void MyClass.SetArray( Vector2[] array);
Vector2[] MyClass.GetArray();

This works and is pretty and intuitive. However, it has two problems:

  • GC is allocated on return
  • Memory is copied to Godot native formats every time there is a call.

The idea is to add NoAlloc versions, which can be generated directly by the binder automatically when required:

void MyClass.SetArrayNoAlloc( Godot.Collections.PackedVector2Array array);
void MyClass.GetArrayNoAlloc(ref Godot.Collections.PackedVector2Array ret_vec2_array);

This way, we solve two problems:

  • Using the Godot native types, no conversion happens between C# and C++, the Godot array can be used directly from C#, and even reused to pass multiple times as argument.
  • The return version takes an existing Godot array and fills it.

As a result, the Godot API can be used entirely from C# in a way so, if this area is performance critical somehow, we ensure no memory copies, safe native access to Godot internal data and ability to control what goes to the GC.

The idea is to provide these specialized call syntaxes for functions using packed arrays, arrays and dictionaries.

If this enhancement will not be used often, can it be worked around with a few lines of script?

N/A

Is there a reason why this should be core and not an add-on in the asset library?

N/A

@xzbobzx
Copy link

xzbobzx commented Sep 24, 2023

I like this part:

"Technically, this could be done from the binding generator itself, without breaking compatibility, and without doing any modification to Godot itself."

This proposal would give users more control over their game's performance while the tradeoff on Godot's part is essentially zero. (Sure, some dev time to get it set up.)

It sounds like a lot of pros for hardly any cons. Am I reading it correctly like that?

@reduz
Copy link
Member Author

reduz commented Sep 24, 2023

@xzbobzx Yeah I think so.

@wareya
Copy link

wareya commented Sep 24, 2023

Would this automatic re-binding work for APIs that have variant Arrays in them, like ArrayMesh's add_surface_from_arrays? Or would add_surface_from_arrays continue to take a Godot Array with C# arrays inside it with no alternative? (To note, some of the entries in the Array passed to add_surface_from_arrays are of determined-at-runtime types - the custom-format arrays like ARRAY_CUSTOM0 etc).

@reduz
Copy link
Member Author

reduz commented Sep 24, 2023

@wareya I think the main problem here is more along the lines of the raycast function rather than a GC one. If you want to build and update geometry every frame, Godot has API in C++ for this (as example the one used by softbodies), but its not exposed to the binder due to limitations of it. Solving that issue should solve this one too.

@Nomad1
Copy link

Nomad1 commented Sep 24, 2023

Are there any actual confirmed cases of GC stalls caused by this? Don’t get me wrong, but getting a huge array of vectors from the engine is not a common task, it goes to Gen1 type GC queue and shouldn’t affect performance anyway. Submitting arrays to the engine is an another story but still can be improved easily with pointers - pass a pointer to the array instead of doing stackalloc and copying the contents in manage score but copy it in CPP part instead.
My concerns is that Godot.Collections are not so common and flexible as built in CLR types and with it we are moving farther from vanilla .Net to a custom language dialect.
P.S. Surely, if you can implement a NoAlloc version in a few clicks, we can give it a try and I might be wrong and it will be even handlier.

@wareya
Copy link

wareya commented Sep 24, 2023

@reduz I don't rebuild geometry every frame all the time when standing still with the world fully loaded, but I do while the world is loading and also when the player travels into new chunks. I do it on a separate thread, streaming everything in as fast as possible, so while it doesn't cause a lower framerate or any stuttering, a performance improvement in removing the extra copy would be noticeable to players as being able to turn up the render distance higher and distant lands becoming visible more quickly. Despite being an unusual edge case, it makes the extra copy when converting the C# arrays into Godot packed arrays worth worrying about to me for a reason other than framerate or loading time impact.

@reduz
Copy link
Member Author

reduz commented Sep 24, 2023

@Nomad1 Godot internally does not use C# arrays, so you have to pick. You use the pretty dialect and stand copies and GC issues, or you use the Godot dialect and ensure maximum performance.

I am pretty sure that not all your code will need to use the Godot dialect, so it should be reserved for cases where you absolutely want the performance.

@gknedo
Copy link

gknedo commented Sep 24, 2023

Nice! I'm following this discussion and this proposal has the potential to touch some hearts.

I think it's out of the scope of this discussion, but would be nice to see the boundaries of this zero allocation API. Having a list of the mapped functions. So the work to be done would be clear.

And I think it's not the moment to think about the details of the implementation, but what do you think of using the same structure of the current API with a different namespace? Something like

void NoAlloc.MyClass.SetArray( Godot.Collections.PackedVector2Array array);
void NoAlloc.MyClass.GetArray(ref Godot.Collections.PackedVector2Array ret_vec2_array);

@tkgalk
Copy link

tkgalk commented Sep 24, 2023

Great stuff, very much in favour, and I really appreciate that you heard the feedback and opened the proposal. Proposal coming from Juan will have a different reach than one made by a random "Unity-refugee".

Obvious next step would be a C# profiler in Editor, but that's a separate thing and I'm sure will be tackled in the future (for now the JetBrains one has to be enough).

@Nomad1
Copy link

Nomad1 commented Sep 24, 2023

@Nomad1 Godot internally does not use C# arrays, so you have to pick. You use the pretty dialect and stand copies and GC issues, or you use the Godot dialect and ensure maximum performance.

C# Vector2[] in memory is stored exactly the same as C++ Vector2*. We can pass it to the native code for free, with zero copying or using Godot.Collections.Array. There won't be any issues unless you take the pointer and hold it longer than needed. And even if you intend to make a hard copy of that data in C++ code, it will be out of GC scope.
So I'd say it's generally better than using your own collection types and losing things like Span<>, BlockCopy, natively optimized iteration, etc.

@reduz
Copy link
Member Author

reduz commented Sep 24, 2023

@Nomad1

C# Vector2[] in memory is stored exactly the same as C++ Vector2*. We can pass it to the native code for free

Except for some specific cases where Godot does take pointers (in very high performance APIs such as RenderingDevice::buffer_update as example), in far most cases it uses reference counted arrays because its far easier and safer to pass around information this way, so even if Unity can pass a raw pointer, this is not useful to Godot.

Hence, the way to do these things efficiently in C# in Godot is to use the Godot collections.

@fodinabor
Copy link

Is the array always big enough or do you need to provide a hook in the api to communicate its too small, i.e.

bool MyClass.GetArrayNoAlloc(ref Godot.Collections.PackedVector2Array ret_vec2_array, ref int size);
// or if PackedVector2Array knows its size:
int MyClass.GetArrayNoAlloc(ref Godot.Collections.PackedVector2Array ret_vec2_array);

@SamPruden
Copy link

This is very cool! I'm glad to see this being embraced!

As the person who started all of this mess for you (sorry) I'd be happy to work on the design and maybe implementation side of this if it would help. I've got quite a few ideas about how to do this style of API nicely.

The basic idea here is very similar to what I had imagined, which is lovely - particularly where it's handled automatically in the binding.

I'll put some more detailed proposals down sometime soon if they're welcome.

@Nomad1
Copy link

Nomad1 commented Sep 24, 2023

@reduz

C# Vector2[] in memory is stored exactly the same as C++ Vector2*. We can pass it to the native code for free

Except for some specific cases where Godot does take pointers (in very high performance APIs such as RenderingDevice::buffer_update as example), in far most cases it uses reference counted arrays because its far easier and safer to pass around information this way, so even if Unity can pass a raw pointer, this is not useful to Godot.

Hence, the way to do these things efficiently in C# in Godot is to use the Godot collections.

Well, my approach was never about adopting the raw pointers internally, but about adding C++ overloads that can make a Godot.Collections.Array from data pointer instead of numerous calls to monsters like godotsharp_packed_vector2_array_new_mem_copy. That's how we can achieve performant interop without extra pressure on GC or filling the code with collections created in C++ and passed back and forth between unmanaged and managed parts. However, I understand that it will be nowhere near your solution by the amount of changes required.

@SamPruden
Copy link

I agree with @Nomad1 that this is a good opportunity to get some additional performance wins (in both C# and GDE) by allowing some internal allocations to be optionally skipped in favour of external preallocated buffers.

For these things, it actually should be possible to use the C# collection types.

@wareya
Copy link

wareya commented Sep 24, 2023

Well, my approach was never about adopting the raw pointers internally, but about adding C++ overloads that can make a Godot.Collections.Array from data pointer instead of [...]

Godot's array types are dynamic arrays, not static arrays. They need to be able to resize themselves, which means that the pointers inside them have to come from the same allocator that they themselves use. This is a UB minefield and probably wouldn't even be safe for C# code.

(They would need to come from the same allocator even if they were static arrays, since they have to be able to destruct themselves when their last reference expires, but the possible workarounds for that are less dangerous than with full reallocation.)

@Nomad1
Copy link

Nomad1 commented Sep 24, 2023

Well, my approach was never about adopting the raw pointers internally, but about adding C++ overloads that can make a Godot.Collections.Array from data pointer instead of [...]

Godot's array types are dynamic arrays, not static arrays. They need to be able to resize themselves, which means that the pointers inside them have to come from the same allocator that they themselves use. This is a UB minefield and probably wouldn't even be safe for C# code.

Those are created and resized in C++ code with a consistent allocator: https://github.com/godotengine/godot/blob/master/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs
So it's safe and nice, but not super-fast since every Add or Count call is an Interop. I'd love to avoid those classes in ultra performance-critical parts, i.e. when you are animating the water on the CPU and sending a bunch of vertices every frame. However as I said before, I don't think that it would make a big difference anyway. Will need to do some profiling first.

@wareya
Copy link

wareya commented Sep 24, 2023

So it's safe and nice, but not super-fast since every Add or Count call is an Interop. I'd love to avoid those classes in ultra performance-critical parts, i.e. when you are animating the water on the CPU and sending a bunch of vertices every frame. However as I said before, I don't think that it would make a big difference anyway. Will need to do some profiling first.

I feel like something similar to this could be achieved by reading the raw pointer out of the array and making sure not to destroy or modify it array until you're done using the raw pointer, rather than by trying to construct a godot array out of a raw pointer.

@Nomad1
Copy link

Nomad1 commented Sep 24, 2023

So it's safe and nice, but not super-fast since every Add or Count call is an Interop. I'd love to avoid those classes in ultra performance-critical parts, i.e. when you are animating the water on the CPU and sending a bunch of vertices every frame. However as I said before, I don't think that it would make a big difference anyway. Will need to do some profiling first.

I feel like something similar to this could be achieved by reading the raw pointer out of the array and making sure not to destroy or modify it array until you're done using the raw pointer, rather than by trying to construct a godot array out of a raw pointer.

I doubt it's possible. We can only guarantee that the raw pointer is immutable during the single call and within fixed(Vector2 * ptr = &array) { ... }. But that data should be copied to some other collection anyway. Right now it's done in the managed code in the GodotSharp assembly, I recommend doing it in the unmanaged code, Juan proposes to use unmanaged collections from the start. All three approaches have flaws but Juan's one is easily achievable.

@reduz
Copy link
Member Author

reduz commented Sep 24, 2023

@wareya @Nomad1 Consider that what I am describing in this proposal is not possible yet because the Godot collection types are not properly exposed in C#. They need to be rewritten to work as I describe using the new extension API in Godot.

The is because C# support in Godot is quite old (from the Godot 3 days) while Godot 4 uses a new extension API that is far more efficient and supersedes all the glue code that Ignacio had to write from scratch for mono.

Ideally, in the coming months (after getting current C# to work on mobile), C# support in Godot will most likely be rewritten to use the new Godot 4 extension API (this will make performance 100% optimal, allow rewriting any part of Godot in C#, even renderers, add classes as native classes, unify the .net editor with regular editor, etc) and this will be implemented there.

@lupin-de-mid
Copy link

Consider use Span as return type. That could be nice wrapper around native arrays

@Repiteo
Copy link

Repiteo commented Sep 24, 2023

I don't know if NoAlloc needs to have any special designation in the name; would a native collection as an argument be enough to make that functionality implicit?

@wareya
Copy link

wareya commented Sep 24, 2023

I don't know if NoAlloc needs to have any special designation in the name; would a native collection as an argument be enough to make that functionality implicit?

Godot's API gets exposed to languages that don't have argument-list-based overloads, and you might want the API to be consistent between them and languages that do. I'm not sure if NoAlloc is the best way to do it, but it makes at least a little sense.

@Repiteo
Copy link

Repiteo commented Sep 24, 2023

Godot's API gets exposed to languages that don't have argument-list-based overloads, and you might want the API to be consistent between them and languages that do. I'm not sure if NoAlloc is the best way to do it, but it makes at least a little sense.

Oh yeah, in the native binding format it makes sense to have a designation. I was thinking more in the context of C# specifically, and if it could suffice with a uniform name on the surface

@AwayB
Copy link

AwayB commented Sep 24, 2023

Are there any actual confirmed cases of GC stalls caused by this? Don’t get me wrong, but getting a huge array of vectors from the engine is not a common task, it goes to Gen1 type GC queue and shouldn’t affect performance anyway. Submitting arrays to the engine is an another story but still can be improved easily with pointers - pass a pointer to the array instead of doing stackalloc and copying the contents in manage score but copy it in CPP part instead. My concerns is that Godot.Collections are not so common and flexible as built in CLR types and with it we are moving farther from vanilla .Net to a custom language dialect.

I do not believe that these GC stalls happen, or are a problem at all.

I believe that this proposal stems from the recent social media pressure. Since 2 weeks ago, there's been a very judgmental tone online from a few of the recent "Unity refugees", about Godot's supposed "poor performance".
The demands they've been giving can roughly be summarised as:

  • Godot should have the best performance possible, at any cost, even that of good design
  • Godot must have the best performance in C#, no care for C++ at all

While this mentality would make sense in Unity, where the codebase was handled by Unity and was never the community's responsibility, and it only supported C# for coding, it doesn't at all in Godot, where the community has to maintain a large and complex codebase on a limited budget, and where the usability/performance ladder goes:

  • GDscript, VM, instant iteration, quick to write and read, highly integrated, mild performance
  • C#, compiled, slower iteration, access to a massive ecosystem of libs, great performance
  • C++, compiled, slowest iteration, best performance

In the past days, attempts to explain these design aspects of Godot to these demanding Unity users has been unfruitful. The main point of failure in these attempts to explain is sadly that, well, they don't care. While some Unity former users have made amazing work with Godot's features, and one absolute monster has transferred the entirety of his game's logic in C# from Unity to Godot in 14 hours, for some Unity devs, it's been a full public judgement of Godot's codebase before talking to anyone from Godot, or even asking why are things made the way they are.

Articles have been written and have taken an extremely small part of the engine and made extensive judgements with hyperbolic words such as "terrified, limited potential, dramatically and systematically worse performance". These articles have the same validity as me going to Unreal and picking the worst, most unoptimised part of their stack and claiming that its "incredibly unoptimised"...yet a very large number of people have taken these articles as a legitimate study of Godot's performance in depth and ran with it.

Besides the quick, and mostly false, judgements, the bigger problem is that the claimed pain point of these people, performance, is, at the risk of triggering them, of secondary importance for most of gamedev, and only one of the main goals of Godot's design.
As I said above, there are 3 languages that Godot lets you develop with, and each responds to a different kind of need in gamedev:

  • Highly tweakable components (player characters, gameplay feel, special effects, enemies, AI, immediate environment...)
  • Regular, strong performance components (systems, game background logic...)
  • Mandatory, best performance components (render/physics, very large amount of entities, high perf systems...)

This division of labor is because gamedev isn't about slogging through code optimisation like you're making YAPSQL but about a slew of different trades working together (artists, animators, game designers, programmers, render/physics specialists, tool engineers, etc).
GDscript is meant to help artists, animators, designers, iterate as fast as possible, with a simpler, readable syntax, running on a VM, with tight integration, hot reloading and extremely practical iteration.
C# is to open up generic purpose, high performance code to a very large ecosystem of libraries and tools, for programmers to add any logic that their game could need.
C++ is to get the best performance possible, in the most performance critical areas of the engine, for render, physics, or top performance code.
And GDExtension, even without modifying the C++ source code, allows to extend into almost any language, so you can even get your own flavor of code.

The very design of the engine is centered, all the way down to its multiple languages, to cater to all these needs as best as possible. Each layer fit for purpose, with performance and usability being yin and yang. This in my opinion, is Godot's greatest strength. It's what sets it apart from any other engine. Godot intelligently balances out performance, usability, productivity, amount of targets, and manages to remain greatly sustainable and multifunction.

However, and without mentioning any one individual in particular, this "star feature" of Godot was completely ignored in favor of a highly ideological, inapplicable vision of programming and...well, in favor of Unity.
The given justification of "performance", which is waved around like a flag, falls apart quickly when their complaints are about non-performance critical parts of Godot, or when you ask them to directly use the engine's C++. The former is rejected on ideological grounds: "everything should be high performance". Even if it needlessly complexifies and damages the engine's code clarity. The latter on convenience grounds: "I don't know C++". Asking for the greatest C# performance and NoGC policies when Godot's C++ and GDExtension are right in their noses is, to say the least, out of touch.

In fact, they are so out of touch that we're now to the point that rather than them dealing with C++'s manual memory management, we're now being pressured into considering a proposal where we have to build a whole new copy of the C# bindings to walk around C#'s GC, instead of telling them to use the base engine language that already has none. Because C# was their main horse in Unity, and so they demand that it remains it now, and Godot is supposed to reimplement a custom version of something that already exists.

Whether you look at this situation from a usability, productivity, code clarity, or performance standpoint, I do not believe that this proposal responds to any needs of Godot, but merely satisfies the demands from a group of people who intend for Godot to patchwork itself into replacing Unity for them.

I do not believe that this extra layer of complexity to C# is in the interest of Godot, or fitting to Godot's brilliant design. If truly, a C# layer with strong performance isn't good enough, then C++ is right open to a user. Or GDExtension. Instead of flatly stating why Godot is what it is, we're conceding to social media pressure from a vocal minority, and considering denting Godot's excellent design, for people who did not and do not care to understand Godot, did not care to try it for the vast majority of them, and who want:

  • Unity like constructs
  • Unity like design
  • Unity like habits
  • Unity like language

Even if it brings:

  • needless complexity in C#
  • needless performance optimisations that C++ already offers
  • unwanted divergence from Godot's clever division of labor
  • a precedent that C# should become the dominant language of Godot

This isn't tribalism, nor is it personal. It is a raison d'être. Godot is designed in the best way that I can think of. Unity was not. This proposal is encouraging that we start abandoning Godot's greatest strength to appeal to Unity users who did not even care enough to try Godot, let alone adapt to it, and instead, come with quick judgements and a demanding stance. The numerous other users that have engaged positively haven't been close to this demanding, and instead lay praise on Godot regularly, whether for its usability or its performance or design.

Godot has always been open to bettering itself. I don't believe that this is about bettering it. This is departing from its clean design to satisfy people who do not even try to understand Godot and judge it "wrong" for not being Unity.

I believe that it is a very bad idea to support this. What this will effectively do is set a precedent where, instead of imposing Godot's design, we accept to harm the design in the name of appealing to Unity users who will certainly keep pushing further for Godot to become even more like Unity. Complaint after complaint, we will have to argue Godot's design against Unity's design, which will only fracture the community.

While there is certainly some good to take from Unity, dismissing the core design of Godot is not something we should encourage or accept. It'll lead to the original Godot users pushing for Godot, while the new ones will push for Unity.
When the Unity users who started out by writing judgmental articles without knowing Godot well are encouraged with "Unitifying" it, we're marching against Godot's long term interest.

I believe that we should accept that in the implosion of Unity, we have gathered a massive amount of new users, support, and love, and that some people will not even care to look at Godot for what it is, and should be politely told that it's probably not the engine for them. And if they feel like this is a justification to throw shade on it and make judgmental claims without having even tried it, well, they were doing that from the start. Caving in is not going to stop that attitude. If anything, after this request, there will be another, and another, and more and more, and that any rejection will be another round of public complaints and judgements, until we cede to them again.

Is this proposal made in the interest of Godot?
Is it following the proper division of labor and design principles of Godot?
I think not.

Is it meant to concede to this vocal minority of Unity devs who have not engaged with Godot, but merely demanded that it change to suit them better?
Will this minority be encouraged to demand even more and keep reshaping Godot to fit the preferences they have for Unity?
I think so.

Please consider this along with the technical aspects of this proposal.

@GabrielRavier
Copy link

GabrielRavier commented Sep 24, 2023

@AwayB Ok, so let us assume for a minute that the presuppositions on the technical merits of the proposal that you are making here (that those merits are near to non-existent) are true (not that I agree with these presuppositions, but it should make things simpler).

...So what if this proposal is "meant to concede to this vocal minority of Unity devs who have not engaged with Godot". Why is it supposed to matter whether there is much a particularly large technical merit to the proposal ? From what I've read in this thread, the technical cost of the proposal is near-nil. Is that such a big cost to take in exchange for getting a massive amount of Unity devs over to Godot ? In other words, it seems to me like even the smallest technical merits for the proposal seem to me like they would be enough to accept it, given how little it costs to accept compared to how much good it can make for the Godot project, even if only to see if it is worthwhile in practice. Is the mere existence of a zero allocation C# API next to the other bindings going to make the rest of them worse ?

The only arguments I can see in your post that could counter this would be that either this will magically send Godot down some hyper slippery slope that will instantly destroy the engine, or that the engine being popular is actually a bad thing. Neither seem plausible to me. In particular, could you detail how this is supposed to be "dismissing the core design of Godot" ?

Also, one last thing. I'd like to point out one quote from what you've said:

I do not believe that this proposal responds to any needs of Godot, but merely satisfies the demands from a group of people who intend for Godot to patchwork itself into replacing Unity for them.

What does the "needs of Godot" refer to here ? Is Godot some kind of abstract entity completely separate from what its users want ? Because that sentence just sounds like "I don't think Godot should answer to the needs of its users" to me.

@Visne
Copy link

Visne commented Sep 24, 2023

The only real counter-argument I could find was this:

we're now being pressured into considering a proposal where we have to build a whole new copy of the C# bindings to walk around C#'s GC, instead of telling them to use the base engine language that already has none

But they themselves admitted that C# allows for faster iteration than C++, and I think we can all agree that it is easier to use as well. Obviously developers using Godot should be allowed to pick C# over the other two languages, and whether they come from Unity or elsewhere is irrelevant to the amount of support they would get from the engine.

And while there is undeniably an influx of Unity developers, the alleged "social media pressure" from these developers is far outweighed by the doubling of monthly Godot funding that came as a result of all of this. I assume that is more than enough to develop and maintain the relatively small part of the codebase that would be required to implement this proposal.

@Repiteo
Copy link

Repiteo commented Sep 24, 2023

Technically, this could be done from the binding generator itself, without breaking compatibility, and without doing any modification to Godot itself

Ideally, in the coming months (after getting current C# to work on mobile), C# support in Godot will most likely be rewritten to use the new Godot 4 extension API (this will make performance 100% optimal, allow rewriting any part of Godot in C#, even renderers, add classes as native classes, unify the .net editor with regular editor, etc) and this will be implemented there

Both quotes from reduz, the first being from the OP. This isn't a matter of overhauling a working system to appease a vocal minority, it's recognizing something that can be implemented in a rewrite that's going to happen anyway. The C# system as it currently exists is inherently legacy, there's nothing antagonistic in saying that

@xzbobzx
Copy link

xzbobzx commented Sep 25, 2023

Let's say this is implemented, what part of the "bloat" would actually affect any Godot user?

Would the engine become slower? Would it become harder to build? Harder to maintain?

I can't imagine any of this being the case, most regular users wouldn't even come in contact with it and on the build side the whole thing appears to be automatic.

So beyond a slippery slope argument the only other argument against its favor would be "Maybe it doesn't help as much as people think it will."

Which, okay fair, but beyond dealing in hypotheticals the only way of actually knowing if that's the case is testing the code and playing around and benchmarking it.

@GabrielRavier
Copy link

GabrielRavier commented Sep 25, 2023

@AwayB So I hope I haven't misunderstood anything that you've been saying here, but so it seems to me like what you're saying boils down to the following: C# must, within the context of Godot, always remain as a language that is the middle ground between GDScript and C++ - slower iteration than GDScript, but faster than C++, and better performance than GDScript, but slower than C++.

As such, I would like to ask you one thing: Why is that fundamental to Godot's design ? It seems to me like you've simply been repeating that same thing over and over in different manners each time, that C# must always be the middle ground and not try to expand anywhere beyond that, but nowhere have I seen an explanation as to why it not trying is so important for Godot.

To me at least, it seems obvious that even if you have such views, you should still be able to support this proposal - if it would be so detrimental to Godot's fundamental design principles, then trying to put the proposal into place would make that obvious, and then it could be reverted. From what I can see, this seems like the obvious way to go, especially given the quite small technical cost of the proposal, which, from what I have seen, you have not disputed.

@tkgalk
Copy link

tkgalk commented Sep 25, 2023

Last post from me that is off topic.

Is Godot made by its community or not? If a large part of that community, even if new, wants something done should their needs/ideas be gate-kept just based on the previous engine they used?

Food for thought.

@wareya
Copy link

wareya commented Sep 25, 2023

Getting built-in C# profiling is a long-standing feature request. It is not primarily coming from recent new users.

@reduz
Copy link
Member Author

reduz commented Sep 25, 2023

@tannergooding

Here, you're covering that it should be possible for Godot.Collections.Array to itself expose a way to get a Span directly to this underlying buffer on the C# side.

Yes, to be more specific, Godot internally uses thread safe copy on write reference counted arrays to move buffers of data around. This works exceedingly well for the engine design because it lets you keep them, modify them, etc. without risk of breaking anything else and without doing extra copies.

This way even cool things like loading a mesh buffer from disk, and sending it to the render thread via command queue for registering there can be done without additional code, or during multi thread scene processing you can defer calls between any function in the engine using those arrays.

Godot uses this a lot and its one of the core parts of the architecture that lets it be very efficient and keep it simple.

Technically these arrays should be exposed as Godot collections, but currently the C# API does not give you very good access to them.

The ideal implementation is that they are better exposed to C#, so you can access their internals via Span. These buffers are safe, if you try to write to them they will do copy on write (if used somewhere else), otherwise if you want to just read, it does not do any copy (so you need to use a const version of the Span).

Exposing the actual API calls with those arrays, and exposing their internals via Span should solve the performance issues mentioned above.

Because of this, exposing a C# binding for such an API that looks like void Method(System.ReadOnlySpan input); is not possible because the API hasn't been designed to work with memory like this.

Yes exposing methods like this is not possible, you have to use the existing Godot collection objects in order to access their memory directly. You have to create a buffer, resize it, then access it via Span and write it and finally send it as argument. You can reuse that buffer as much as you want if you don´t want to trigger the GC.

@tannergooding
Copy link

Is Godot made by its community or not? If a large part of that community, even if new, wants something done should their needs/ideas be gate-kept just based on the previous engine they used?

Notably, community made doesn't mean doing anything that any member of the community wants, nor does it even necessarily catering to large portions of the community.

It is the responsibility of the maintainers, of any project (OSS or not), to take a comprehensive look at what's being asked and determine whether or not its a good fit. In an ideal world, they will get the resulting decision right most if not all of the time.

But, we aren't in an ideal world and maintainers will make mistakes. At the same time, so will the community. The reality is that a majority having a shared idea doesn't necessarily mean that idea is good or bad, it can be either or even both. No one has all the context and no one has future sight.

The Godot maintainers obviously have some idea around what they're doing, as they've successfully maintained and grown the repo for nearly 10+ years now after all.


It's very clear that a large portion of the .NET community cares deeply about perf. It's also clear that Godot does as well, but that there are some considerations that come from the Godot side around various factors that do impact the overall maintainability and usability of the codebase.

It is not as simple as just exposing new C# APIs in the ideal shape, it is not as simple as just adding APIs in the ways that people may expect. There is a lot of complexity and consideration to exposing new APIs, to supporting new patterns, and in general to extending that to all the places that may need, want, or benefit from similar functionality.

So, ideally, we in the .NET community work with the Godot maintainers to better understand what limits they have in place and why. We consider what our core needs are and how those could be achieved given any limitations or constraints the maintainers set out. We then work together to see if a reasonable compromise can be found.

That's why I initially gave some context as to how this is handled in modern .NET. I then asked for more clarity around what reduz said in response, and offered an alternative that would still allow .NET to benefit as well as other languages, in a way that (at least to my current understanding) might mesh well with the existing codebase.

It could be that I'm still missing key context, but the important bit is to have a two way discussion and really attempting to understand these core factors and limitations. With enough discussion and interest, we can surely find some common ground that works for everyone and doesn't needlessly force churn where it isn't actually required. -- If a pattern can be found and agreed upon, then it can be lit up as needed, much as we did in the BCL when adding Span<T>. We didn't get everything in .NET Core 2.1 and we still don't have "everything" in .NET 8. We've incrementally added the support to core APIs and otherwise based on user request, need, feedback, and perf data showing its required.

We expect the same communication and open discussion for API suggestions and contributions on dotnet/runtime, and so its more than reasonable to provide the same courtesy here to Godot ❤️. -- We also, have "angry" users plenty because we have to tell them "no". They don't care that we have more context and insight into how to maintain the runtime and refuse to see it anything but their way, and that's just generally not beneficial to anyone.

@tannergooding
Copy link

Thanks for confirming @reduz!

It sounds, then, like there are some good options possible for places that are actually identified to need it.

Much like on the other thread I'm on around improving the numerics types (godot/issues/69490), I'm more than happy to continue discussions in this area and to provide any insight into common pitfalls or other considerations for .NET

If there's anything I can do to help out in this area or to help prototype, let me know and I'd be more than happy to (I'm also on the Discord now, so am there as tannergooding as well)!

@Repiteo
Copy link

Repiteo commented Sep 25, 2023

@AwayB

Please explain to me how an engine whose core language has manual memory management, and a higher level language incorporated, can rewrite the same exact same memory management pattern in the higher level language without it being repetition, and thus bloat

Actually, I can touch on this! One of my previous C# PRs had the intent of improving the binding generation in several respects. One such improvement was making the generated documentation significantly smaller by removing redundancies via <inheritdoc/>. However, this was ultimately decided against; because, while it made the generated code smaller, it served to make the actual binding code itself more convoluted—which is the area where contributors will actually be working. As raulsantos put it:

In general, we should prioritize cleaner code in the non-generated code since it's the code that we have to write and maintain ourselves, the generated code can be as long and repetitive as we want since it's machine-generated.

This is to say that, despite surface-level redundancies, it's not necessarily to the benefit of the codebase to do absolutely everything to remove said redundancies. In other words, repetition is not inherently bloat. Repetition can have the benefit of making the elements of the codebase that are actually worked with much leaner and readable, without compromising functionality. Repetition can have the benefit of only showing itself in generated sections of code, which is exactly what this binding scenario would represent. Repetition stands to have nothing to lose in a scenario where, even in the hypothetical situation where it produces no real-world benefit, introducing it would have less-than-negligible performance and maintenance costs.

@EgorBo
Copy link

EgorBo commented Sep 25, 2023

  • GDscript, VM, instant iteration, quick to write and read, highly integrated, mild performance
  • C#, compiled, slower iteration, access to a massive ecosystem of libs, great performance

My 2 cents on this - why is it an issue for C# to be slower for the inner developer-loop. C# has an interpreter and hot-reload abilities. Roslyn is also extremely fast with the compiler server.

@tkgalk
Copy link

tkgalk commented Sep 25, 2023

I'm also not sure why, but it does feel like C# "has to be relegated to second-class citizen or people get angry/overly protective".

Even when I said that C# profiler would be a great feature that was somehow taken as "inflammatory".

Making one thing better doesn't mean making your chosen language/engine worse. It's not a zero sum game.

@rodrigofbm
Copy link

Getting built-in C# profiling is a long-standing feature request. It is not primarily coming from recent new users.

I'm using C# since it's production ready in 3.x. I think this guy has nothing batter to do other than shouting things from his imagination.

@reduz
Copy link
Member Author

reduz commented Sep 25, 2023

Folks please chill. Godot is driven by community demand. If significant community wants to implement C# and they want to maintain it, then nobody is to oppose this.

If significant amount of the community wants to implement JavaScript and they want to maintain it its the same, nobody will oppose (not the case though 😅 )

There are no resources wasted since those who want to work on something will do, and those who don't will not. This is the beauty of FOSS.

@wareya
Copy link

wareya commented Sep 25, 2023

Hey, Javascript is actually a really good idea, since it's the only way to get JIT on iOS!

@Visne
Copy link

Visne commented Sep 25, 2023

They said they were done with this issue, let's stop pinging them and get back on topic.

@nockawa
Copy link

nockawa commented Sep 25, 2023

This thread has a lot of off-topics and discussion, so it's hard to follow things and not posting doing the same, but I'll try to contribute anyway, hoping I won't waste your time and/or add noise.

Wrapping a native memory buffer (allocated by something else than .net) in a very efficient way in .net is fairly easy.
You can create a Span<T> instance only with an address and a length. You just have to ensure this memory block has a lifetime as big as the Span you create. If someone a reallocation occurred, then .net is like C++, it will crash unexpectedly.

So if you have C++ exposed API to:

  • bool BeginArrayRO(void*& addr, int& size). For .net to retrieve the address and length, and ensuring it will stay valid until you call the following one.
  • void EndArrayRO(void* arrayAddr). To release the usage.
  • Their counterpart in read-write, maybe with more data exposed like the array header that contains the length and the count of items currently present, to allow the .net side to do an Add without interop.
  • void ResizeArray(void*& arrayAddr, int newSize) to resize when needed.

Then it's fairly easy to expose the array in .net, expose (ReadOnly)Span<T> access, implement Enumerator (using ref struct to avoid GC alloc of the enumerator) to allow foreach and even IEnumerable<T> to allow LINQ.
Performances will be nearly identical with C++ and your Array will be compatible with many .net APIs working on data manipulation.

Overall, you must avoid:

  • Copying data from native to managed. This, to me, is a no brainer and really faisable.
  • Reduce the interop call as much as possible. In the Array.cs @reduz gave the link, there is an interop call for each Add operation. There is a clear performance difference from using the native API for sure.

thread safe copy on write reference counted arrays to move buffers of data around.
Could you elaborate please?

It means data is shared as long as it's accessed in read-only, but if you want to modify the content then it creates your own copy? Is there online documentation about this I could read? Edit: found it.

@r-bertolini
Copy link

@Visne fair enough, I've deleted my previous post.

@minim271
Copy link

Really glad to see this proposal, maybe i can close #2757 ?

@ewrogers
Copy link

ewrogers commented Sep 26, 2023

void NoAlloc.MyClass.SetArray( Godot.Collections.PackedVector2Array array);
void NoAlloc.MyClass.GetArray(ref Godot.Collections.PackedVector2Array ret_vec2_array);

Personally I don't find this namespace prefixing intuitive for several reasons:

  1. I need to be aware of the namespace
  2. It won't show in auto-complete side-by-side in IDEs (SetArray() + SetArrayNonAlloc())
  3. Refactoring becomes a lot harder to find/replace (s/SetArray/SetArrayNonAlloc/g)
  4. Going against convention, which was sparked by Unity developers in the first place

The signatures for *NonAlloc() variants will usually be different anyhow, so I don't see how the namespace part really adds any value compared to a different method name.

The main benefit of the non-alloc versions is the ability to re-use buffers so their signature is typically "same but you pass in the array/list storage" instead. Typically in raycasts.

@makemefeelgr8
Copy link

makemefeelgr8 commented Sep 26, 2023

Godot exposes zero allocation versions of many functions in the C# API, that users can use if they desire.
I am not familiar with C#

But you're trying to optimize GC calls?! Oh man... Well, I see what happened here. Let me help you.
There's the thing called "P/Invoke". I'm begging you: just read about it. Please. Here, I'll even leave the link:
https://learn.microsoft.com/en-us/dotnet/standard/native-interop/pinvoke
This is industry standard. This is how you make C# work with native code. This is the way.

When you're done reading, take a look at this one too, as it will allow to compile C# runtime into a tiny binary (and facilitate porting to mobile devices, once dotnet8 is out):
https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot

As for the GC- don't worry about it. C# integration has bigger issues now. WAY bigger. Experienced C# programmers can easily deal with memory allocations and write performant code, GC or no GC.


Why are you booing me? I'm right!

Seriously, though, I'm back with some numbes, and I've even got some code to prove my point. This is how one can force the GC to perform the absolute worst nightmare ultimate type of garbage collection, and completely halt the application until it completes! This is the worst scenario that never happens in real life. But it makes for a nice test. It doesn't get worse than this.

GC.Collect(2, GCCollectionMode.Forced);
GC.WaitForPendingFinalizers();

I challenge you: put this into a _Process function, of your project, and test how much performance you loose!
It was about 15% for me.

...and now let's take a look at the infamous article about the overhead of calling raycasting APIs and compare the numbers. This is where you loose not 15%, but 1500%.

So, if I were to compose the list of proirities of what should be done to make C# integration work better, I'd start with addressing the elephant in the room, and leave the GC alone.

@SamPruden
Copy link

SamPruden commented Sep 26, 2023

I'd like to clear up some confusion that's happened (and that I might be a little responsible for!) over the argument between passing in destination buffers and returning Godot arrays. These are both things which we would want to do under different circumstances! There's no either-or or contention between the patterns.

Returning arrays from core

As @reduz has pointed out, Godot's packed arrays can be very efficient in certain cases because they're implemented with CoW, which allows them to be passed from the core into user code with no copying or allocations (except the wrapper, in C#'s case). These are great in circumstances where the core is already holding such an array preallocated internally. Semi-random examples of this include MeshDataTool::get_vertex_weights, HeightMapShape3D::get_map_data, Curve2D::get_baked_points, and CPUParticles2D::get_emission_points. These are the perfect tool for the job in this situation.

However there's a second category of method which in my analysis so far is actually more common, and is certainly very common. These are methods which allocate and write into a brand new array for the purpose of returning it. In a few cases this is directly copying data from other places which could possibly be promoted to use CoW, but in many cases this is done because the core simply does not and cannot cache that information, typically because it depends on parameters. Examples of this include AStar2D::get_point_path, Area2D::get_overlapping_bodies, Curve3D::tessellate, Geometry2D::intersect_polygons, and PhysicsDirectSpaceState2D::intersect_point.

It's this latter category which would benefit from the user being able to supply a target buffer. This allows the user to skip heap allocations by writing into pre-allocated buffers, to take advantage of a bump allocator (Rust would probably love this! And C# too.), and to put data into collection types which are more ergonomic to use in that language, not to mention sometimes faster because they never have to call back into core for any operations. [Edit: Calling back into the core may be avoidable in more cases anyway.]

Passing arrays into core

When passing data from script into core APIs, it's advantageous to be as flexible as possible (up to a point) about what format that data comes in in. This helps the various language bindings with their various collection types and patterns to efficiently pass data into the core without having to do copies in script in order to get it into a useful format, or to write less ergonomic code using Godot collections instead of the native ones.

Sometimes we want this input type to be a CoW Godot array because Godot can hold onto the data without copying, which is perfect. However, sometimes that's not practical for the user, and sometimes Godot doesn't even want the array to leave the callstack so there's no advantage to it being CoW. Ideally, API functions should be able to handle both of these cases depending on what the user gives them.

Postamble

None of this is C# specific. I may have gotten myself a little bit of a (exaggerated) reputation as a C# fanatic, but I've been thinking about this from a GDE first perspective. These ideas are as language agnostic as it's possible to be - they define patterns for basic communication protocols between the core and the scripting language, and the scripting language can interface with those protocols in whichever way works best for that language.

@danbolt
Copy link

danbolt commented Sep 26, 2023

It's this latter category which would benefit from the user being able to supply a target buffer. This allows the user to skip heap allocations by writing into pre-allocated buffers... and to put data into collection types which are more ergonomic to use in that language, not to mention sometimes faster because they never have to call back into core for any operations.

I think @SamPruden's suggestion here would be a really lovely addition for teams shipping a GDScript project that has certain Nodes rewritten in C++. The programmer that's doing the optimizing can take advantage of how the memory's being managed for their specific use case in their specific hierarchy.

@lewiji
Copy link

lewiji commented Oct 8, 2023

@makemefeelgr8

But you're trying to optimize GC calls?! Oh man... Well, I see what happened here. Let me help you.
There's the thing called "P/Invoke". I'm begging you: just read about it. Please. Here, I'll even leave the link:
https://learn.microsoft.com/en-us/dotnet/standard/native-interop/pinvoke
This is industry standard. This is how you make C# work with native code. This is the way.

Juan didn't write the interop code, nor will he be writing this API if it's implemented. There are C# experts who do understand this stuff doing the work.

godotengine/godot@2c180f6

3.x used P/Invoke for its interop, the CoreCLR implementation moved to pointer invocation to avoid pinning. Whether that was a good idea or not, I don't know. But the suggestion that the contributors to C# support don't know what they're doing is easily disproved in the commit history. There's a precedent of using P/Invoke, the current implementation does something different, if P/Invoke proves to be a better way of doing things, Godot can go back to that method.

When you're done reading, take a look at this one too, as it will allow to compile C# runtime into a tiny binary (and facilitate porting to mobile devices, once dotnet8 is out):
https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot

godotengine/godot@4b90d16

The contributors are already aware of this and have made an effort to make this possible from the very beginning of the CoreCLR integration.

So, if I were to compose the list of proirities of what should be done to make C# integration work better, I'd start with addressing the elephant in the room, and leave the GC alone.

I actually overall agree with your point, GC pressure has never been a huge issue for me (using a C# ECS library with godot, and having used C# with godot for several years now) and most areas where it could become a problem are easily identified in a profiling session and worked around. There are devs who are very sensitive to garbage generation, and maybe in certain projects it could become an issue, but in general, if people are that sensitive to GC they can use another language without GC. There are other proposals and ongoing PRs that are intended to address the problems in the internals around allowing for structs and structuring the return values of API methods. A proposal here isn't a statement of priority; it's used to gauge community interest, gather opinions from contributors about their concerns or technical ideas, and figure out priority from there. I think this proposal is an interesting idea, but I don't think it's a huge priority right now.

@makemefeelgr8
Copy link

@lewiji
Thanks for the info! I thought the guy who is not familiar with C# is going to implement performance critical piece of code, and it sounded kinda questionable at best.

3.x used P/Invoke for its interop, the CoreCLR implementation moved to pointer invocation to avoid pinning. Whether that was a good idea or not, I don't know.

And this made C# code in Godot use a lot of unsafe stuff by default. Not the best idea, if you ask me, but well... They were using some similar trickery back when SharpDX was alive, so I guess the benefits are worth it after all.

About the AOT feature. I'm talking AOT + direct P/Invoke. The thing's fast. You can even do some static linking. Totally worth checking out, especially with dotnet 8 rc.

I think this proposal is an interesting idea, but I don't think it's a huge priority right now.

Now these are great news. Instead of making tight coupling to C# and exposing some weird internal methods with no GC, I'd rather propose they provide a dll file, kinda like WinAPI does. One can generate bindings for any language (nodejs, etc) in no time, and make a nice integration. Add a nuget/npm package with some types & bindings, and C#/nodejs integration's done.
Well, that depends on a general roadmap.
And the built-in C# IDE they're talking so much about and making an accent on... It's kind of not a real thing. It's about as good as a notepad, without code highlighting, without autocomplete, intellisense, resharper, copilot, custom plugins. I don't see a C# programmer using it. Like, there's absolutely no way.

@uzkbwza
Copy link

uzkbwza commented Sep 6, 2024

I have experienced the limitations of this first-hand, while working on an asset that heavily queried the physics and rendering servers (>1000 times per frame). the c# equivalent code was far, far slower than the Gdscript code I wrote first, and there was nothing I could do to improve the performance. it is not just the garbage collector that is a problem, allocating so much memory every frame is an enormous hit to performance. this proposal is nearly a year old, yet it appears almost nothing has been done about this ubiquitous and significant problem which completely alienates any serious dev who wishes to make a game with many elements in c#. please tell me I am wrong and I am missing some kind of progress on improving this situation.

It seems c# is only more performant than gdscript when your game logic does not interact with the API.

@Braveo
Copy link

Braveo commented Sep 6, 2024

I have experienced the limitations of this first-hand, while working on an asset that heavily queried the physics and rendering servers (>1000 times per frame). the c# equivalent code was far, far slower than the Gdscript code I wrote first, and there was nothing I could do to improve the performance. it is not just the garbage collector that is a problem, allocating so much memory every frame is an enormous hit to performance. this proposal is nearly a year old, yet it appears almost nothing has been done about this ubiquitous and significant problem which completely alienates any serious dev who wishes to make a game with many elements in c#. please tell me I am wrong and I am missing some kind of progress on improving this situation.

It seems c# is only more performant than gdscript when your game logic does not interact with the API.

I feel like the plan to move C# to GDExtension that I've been seeing around is what's making this process take longer, as far as I know. There's also the struct type that's in the proposal stage for gdscript to manage data differently in a way that would make C# support better, though I definitely do wish there was more being discussed about this.

@C-Higgins
Copy link

C-Higgins commented Sep 20, 2024

@lewiji
if people are that sensitive to GC they can use another language without GC.

C# is perfectly capable of writing the code without GC. It's purely the language bindings that are forcing GC to occur. If that weren't the case, this thread's proposal would be impossible.
The current implementation on Godot's end is gimping C#. This should be fixed. C# is the secondary officially supported language, and gimping it unnecessarily with relatively little effort to not do that doesn't make sense. There have been more developer-hours spent arguing in this issue than it would take to just add the noalloc proposal. Whether a nebulous future improvement is planned or not, this is low hanging fruit that should not take years to figure out.

@lewiji
Copy link

lewiji commented Sep 23, 2024

@lewiji
if people are that sensitive to GC they can use another language without GC.

C# is perfectly capable of writing the code without GC. It's purely the language bindings that are forcing GC to occur. If that weren't the case, this thread's proposal would be impossible.
The current implementation on Godot's end is gimping C#. This should be fixed. C# is the secondary officially supported language, and gimping it unnecessarily with relatively little effort to not do that doesn't make sense. There have been more developer-hours spent arguing in this issue than it would take to just add the noalloc proposal. Whether a nebulous future improvement is planned or not, this is low hanging fruit that should not take years to figure out.

Yeah, in the year that's it's been since I wrote that, I've changed my stance and agree this is more of a priority now. At the time, it was early days for Godot 4 and there was a lot more to take care of. Now it has matured, I think a zero alloc api is much needed. However, I'm not sure who on the contributor team has the skills or bandwidth to implement it right now. If you have any ideas on how to start doing so, I would support that and would be willing to put work in, but it's not something I'm capable of contributing on my own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests