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

Add Span equality (== and !=) operators. #104280

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Mar 17, 2025

This happens to allow us to deduplicate a bunch of operator== from String and Vector.
It might be possible to remove explicit overloads in those functions, instead relying on implicit conversions to Span, but this is a bit risky compilation wise and thus out of scope for this PR. Will revisit this in a future PR.

Span<char32_t> comparison to Span<char> is somewhat questionable from a String perspective, because it implicitly interprets the Span<char> as latin1, although it's usually ascii (still correct) or utf-8 (incorrect).
Still, from a Span perspective this is entirely reasonable, because individual elements report equality. It is up to callers to differentiate encodings if they matter.

Benchmark

This PR happens to optimize the equality checks of Packed*Array (fundamentally typed Vector). The reason is that the old implementation lacked a memcmp optimization. I naively benchmarked a ~4x improvement on my computer. The improvement is likely to be better on modern x86_64 machines, due to wider SIMD support (up to 16x).

I benchmarked this code:

extends Node2D

func _ready() -> void:
	var a := PackedInt32Array()
	a.resize(500000000)
	var b := PackedInt32Array()
	b.resize(500000000)

	for i in 20:
		a == b

This resulted in:

 ❯ hyperfine -m5 -iw1 "bin/godot.macos.editor.arm64.master --path /Users/lukas/new-game-project --quit" "bin/godot.macos.editor.arm64 --path /Users/lukas/new-game-project --quit"
Benchmark 1: bin/godot.macos.editor.arm64.master --path /Users/lukas/new-game-project --quit
  Time (mean ± σ):     11.970 s ±  0.092 s    [User: 10.632 s, System: 0.543 s]
  Range (min … max):   11.856 s … 12.081 s    5 runs

Benchmark 2: bin/godot.macos.editor.arm64 --path /Users/lukas/new-game-project --quit
  Time (mean ± σ):      3.168 s ±  0.035 s    [User: 1.863 s, System: 0.512 s]
  Range (min … max):    3.129 s …  3.221 s    5 runs

Summary
  bin/godot.macos.editor.arm64 --path /Users/lukas/new-game-project --quit ran
    3.78 ± 0.05 times faster than bin/godot.macos.editor.arm64.master --path /Users/lukas/new-game-project --quit

@Ivorforce Ivorforce requested review from a team as code owners March 17, 2025 17:13
@AThousandShips AThousandShips added this to the 4.x milestone Mar 17, 2025
@Ivorforce Ivorforce force-pushed the span-equality branch 2 times, most recently from 453d348 to 67696c2 Compare March 17, 2025 17:44
Comment on lines +128 to +139
// Special case: Comparing a signed and an unsigned type.
// This is undefined behavior, so we need to cast to a common type before comparison.
using CommonType = std::common_type_t<LHS, RHS>;

// Normal case: Need to iterate the array manually.
for (size_t j = 0; j < p_lhs.size(); j++) {
if (static_cast<CommonType>(p_lhs.ptr()[j]) != static_cast<CommonType>(p_rhs.ptr()[j])) {
return false;
}
}

return true;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is arguably a bad idea, because I'm actively circumventing our sanity check of signed comparisons.
The alternative would be to have callers handle this case. This would work for String comparisons (char could be cast to uint8_t). However, it doesn't work for non bitcastable types, e.g. Span<int> and Span<uint8_t> could not be compared with this implementation.

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

Successfully merging this pull request may close these issues.

2 participants