It seems upstream made this slower (probably #100944).

However, changing the members to a TightLocalVector instead made the performance improvement come back. GDScriptInstance::members is resized only once when an instance is created, so maybe nothing is lost by doing this. Force-pushed.

CC @Ivorforce @Nazarwadim

Some regressions are to be expected from resize changes by pure chance. If this is significantly impacted by it it definitely needs a better solution in general.

Not sure how TighLocalVector could even help unless it's being resized every single frame. It has no effect if there's no resize. Do you have a repro for us to toy with?

Do you have a repro for us to toy with?

I use this stress test for every PR that affects GDScript performance: https://github.com/GaijinEntertainment/godot-das/tree/master/examples/bunnymark

After further testing, I noticed there is a performance improvement only when there are too much instances:

6000 bunnies:

20000 bunnies:

Didn't bother measuring with regular LocalVector as all results were lower performance.

It seems a bit situational. I'll report after testing on production builds which matter the most.

I'll report after testing on production builds which matter the most.

The proportions are the same, with or without production=yes

I admit that back then I was pairing the changes of this PR with #90082, which already caused a massive boost to GDScript.

I decided to test everything again with both changes from #90082 and this PR over the current master.

At 5000 bunnies:

At 10000 bunnies:

At 20000 bunnies:

At 50000 bunnies:

The results are too unstable, but I think maybe we should wait for #90082. A regular LocalVector seems more stable than a Tight one.

While experimenting how fast it would be with a std::vector, I realized GDScriptInstance::reload_members() would overwrite GDScriptInstance::members by giving it a regular Vector, and casting that to LocalVector has a bit of overhead.

I have modified so it makes and assigns a LocalVector instead.

It seems whatever made the FPS drop is now gone (not sure if it was this missing change as it was fine with TightLocalVector, probably because both it and CowData are tight, or probably because of #104985 being merged), but the FPS gain is minimal (around 1.5%).

I'm getting mixed performance results though.

That's why I say we should wait for #90082 or something similar.

Every time some PR touches GDScript or LocalVector it changes the outcome of this one. But in all cases combined with #90082 the performance was better.

Out of curiosity I overlapped this PR over some changes of #90082 in a production build (6000 bunnies as that was the one affected negatively):

The same improvement ratio from above can be seen with 10000 or 50000 bunnies

Rebased so I could check the benefits from #106020

While the profiler shows that the overhead of a LocalVector is significantly lower than that of CowData, there is a Variant::operator== that skyrockets. This also happens in the screenshots at the top of this PR page.

Before:

After:

At 10000 bunnies, I can measure a 9% performance improvement with a production template build, but at 1000 bunnies it varies between a 0.3% improvement and a 2.3% decrease. Performance decrease disaappears after 2000 bunnies.

I hope the issue with Variant isn't some LocalVector to Vector conversion.

EDIT: turns out most of CowData::get() overhead is simply being inherited by Variant::operator[].

I have tried to keep a regular Vector, but make GDScriptInstance store members's ptrw() solely on instantiation (GDScriptFunction::call() calls ptrw() on every iteration), but iterating over it revealed where the overhead came from. And was still slower.

Using LocalVector still results in both vector get and Variant::operator== being in their least overhead forms.

At 1000 bunnies, master vs PR

Looks like the problem was never inside Variant::get_named or Variant::set_named, which are actually benefited by this PR. VariantSetGet_Vector2_x/y::validated_get seems to get a massive increase in cycles.

Does anyone know a possible solution for this? Or is it too reliant on CoW?

It seems like whatever overhead in GDScriptInstance::set/get() created by this change can be mitigated by using std::move() when setting/getting the variant.

I don't know if doing this would create unwanted behavior or possibly cause crashes, but so far it recovers some of the performance loss (the increase in Variant::operator= mentioned here goes away and the FPS with 1000 bunnies is higher), tests pass and TPS Demo runs just fine.

I think the lack of CoW is causing Variant constructors to be called in the VM, resulting in overhead. It was fixed on set/get's side by using std::move(), and I don't know how to do it on the VM's side, or if this is even possible or viable at this point.

Rebased after a very long time.

By the time I opened this PR I thought I knew how to read perf outputs, but #112129 taught me a lesson (didn't know callback cycles around the change increasing could potentially indicate a perfomance gain)

Here are a few changes I did:

• Keeping it as Tight, as the vector never gets resized except when the GDScript instance is made or when reloading the script.
• All std::move() workarounds removed (except the script reload one) as they turned out to be doing more harm than good.

On very recent tests, when it comes to Bunnymark, there should be no performance difference at 500 bunnies, maybe a 0.06% gain (from many runs with the least amount programs running on background + Feral GameMode, as GDScript is extremely sensible to fluctuations when not performing too heavy tasks).

Lastly, when running before and after at the same time, it's possible to see sometimes that memory usage isn't stable. Sometimes before consumes many more MBs, sometimes after does. But when they match, after is always one MB lower than before. Also the build is 4KiB smaller.

From a recent discussion in the Contributors Chat, the conclusion is that members here shouldn't need CoW at all (members uses a CoW vector since before the engine was open-sourced). Maybe #107002 could be dismissed.

Will update the PR description later.

PR description updated with newer benchmarks and more considerations that are not the wrong profiler reading.

Switched back to regular LocalVector. While the performance change is around the same and the vector is never resized, members being a LocalVector decreased build sizes by a total of 8KiB.