X Tutup
Skip to content

Simplify varray#108118

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
YYF233333:varray_and_vformat
Sep 18, 2025
Merged

Simplify varray#108118
Repiteo merged 1 commit intogodotengine:masterfrom
YYF233333:varray_and_vformat

Conversation

@YYF233333
Copy link
Contributor

@YYF233333 YYF233333 commented Jun 29, 2025

Reduce complexity of these two functions with initializer list constructor.

varray should be ok because it is used together with sarray, so not performace critical.

For vformat I run both microbenchmark and a test project:

code 
#define ANKERL_NANOBENCH_IMPLEMENT
#include "nanobench.h"
#include "core/templates/list.h"
#include "core/templates/local_vector.h"

class Test
{
public:
    static void bench()
    {
        String fmt = "Test vformat %s %04d-%02d-%02d %02d:%02d:%02d";
        auto bench = ankerl::nanobench::Bench()
            .minEpochIterations(1000);
        bench.run("vformat", [&]{
            for (int i = 0; i < 100; i++) {
                auto r = vformat(fmt, "test", 2023, 10, 5, 12, 30, 45);
            }
        });
    }
};
result

master:

ns/op op/s err% total benchmark
223,433.91 4,475.60 0.4% 2.67 vformat

This PR:

ns/op op/s err% total benchmark
220,315.97 4,538.94 0.4% 2.63 vformat
code 
extends Node2D

func _ready() -> void:
	var times = []
	for i in range(10):
		var start = Time.get_ticks_usec()
		for j in range(100000):
			Time.get_datetime_string_from_unix_time(1751215307)
		var end = Time.get_ticks_usec()
		print((end - start) / 1000.0, " ms")
		times.append((end - start) / 1000.0)
	var avg = 0.0
	for t in times:
		avg += t
	avg /= times.size()
	print("average: ", avg, " ms")
result

master:
175.029 ms
172.557 ms
170.201 ms
172.307 ms
172.757 ms
172.753 ms
172.192 ms
171.617 ms
171.543 ms
173.941 ms
average: 172.4897 ms

This PR:
171.95 ms
170.445 ms
170.952 ms
168.081 ms
169.044 ms
170.402 ms
170.3 ms
171.469 ms
171.817 ms
169.767 ms
average: 170.4227 ms

both tests show no regressions.

@YYF233333 YYF233333 requested a review from a team as a code owner June 29, 2025 17:36
@Chaosus Chaosus added this to the 4.x milestone Jun 29, 2025
Ivorforce
Ivorforce requested changes Jul 2, 2025
View reviewed changes
Copy link
Member

@Ivorforce Ivorforce left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me to simplify these functions; they're definitely more complex than they need to be.
However, initializer_list has a hidden cost for non-trivial types. You can read about this problem here: godotengine/godot-proposals#12247

We've chosen to ignore this problem in most cases, since it's unlikely to lead to problems for most allocations.
However, as commonly called functions, varray and vformat deserve some extra care.

In practice, instead of using initializer_list, I'd probably create the vector, reserve on it (#105928 is scheduled to be merged early in 4.6), and then push_back items from p_args sequentially. This should both simplify and optimize the code. (or, alternatively, resize and loop through the args, assigning to ptrw(). This should be even faster but might be more annoying to implement).

@YYF233333 YYF233333 force-pushed the varray_and_vformat branch from fd38b09 to aece13e Compare July 2, 2025 19:56
@YYF233333
Copy link
Contributor Author

YYF233333 commented Jul 2, 2025

I changed vformat. For varray it is both not performace critical and also I think it is more suitable as a macro in its current state (same for sarray), as long as you don't mind we have a bit more macros.

@YYF233333
Copy link
Contributor Author

YYF233333 commented Jul 2, 2025 •
edited
Loading

I forget there is already #103917 😅. That should supersede vformat part of this PR. I'll revert them.

@YYF233333 YYF233333 force-pushed the varray_and_vformat branch from aece13e to 6dd71ce Compare July 2, 2025 20:38
@YYF233333 YYF233333 changed the title Simplify varray and vformat Simplify varray Jul 2, 2025
Ivorforce
Ivorforce reviewed Jul 3, 2025
View reviewed changes
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized args necessitates a copy anyway, in the same vein that initializer_list does. Therefore, this shouldn't be a performance regression.

The whole initializer_list thing is still relevant, but since it has no satisfying solution yet, I'm OK just simplifying the code for now.

@Ivorforce Ivorforce modified the milestones: 4.x, 4.6 Jul 3, 2025
@Repiteo Repiteo merged commit d90d8af into godotengine:master Sep 18, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Sep 18, 2025

Thanks!

@YYF233333 YYF233333 deleted the varray_and_vformat branch September 19, 2025 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ivorforce Ivorforce Ivorforce approved these changes

Assignees

No one assigned

Labels

enhancement performance topic:core

Projects

None yet

Milestone

4.6

Development

Successfully merging this pull request may close these issues.

4 participants

@YYF233333 @Repiteo @Ivorforce @Chaosus
X Tutup