StringName Dictionary keys

[rune-scape]

follow up for #68747
removes conversion from StringName to String in Dictionary

it seems like a simple change, but many places still compare a Variants type with STRING
simply removing the conversion gave me alot of errors, so i did the sane thing and looked at every instance of an equality comparison to Variant::STRING, and updated most of them with an is_string() method i added to variant:
-- if it accepted String and StringName, i updated it to use is_string() (might as well, it's much easier to read)
-- if it was a closed system, (as in no external input, the type is always known to be string), i let it be
-- if it accepted input, (especially from GDScript), i updated it
-- if i couldn't tell for sure if it was a closed system (like in editor_debugger_inspector.cpp):
---- if it gave an error for StringName input, i left it (could be caught later)
---- if it silently failed, i updated it

note: Variant does not convert StringName to Color or to NodePath, i'm not sure whether it makes sense to accept StringName, so i let it be for now

fixes #68834

here are some numbers i got from this example, with different length keys (on a production build):
TestDictIndexing.zip:

"%s":
  2.318s: String keys create
 81.679s: StringName keys create
  7.384s: String assign create
  1.973s: String dict String index read
  2.145s: String dict StringName index read
  7.278s: StringName assign create
  2.324s: StringName dict String index read
  1.788s: StringName dict StringName index read

"longer-test-key-%s":
  3.883s: String keys create
 83.304s: StringName keys create
  8.323s: String assign create
  2.406s: String dict String index read
  2.462s: String dict StringName index read
  7.543s: StringName assign create
  2.792s: StringName dict String index read
  1.732s: StringName dict StringName index read

with a long enough test key, StringName is faster even when finding a String key:

"how-long-could-it-take-to-hash-this-string?-key-%s":
  4.566s: String keys create
 82.376s: StringName keys create
 10.346s: String assign create
  2.949s: String dict String index read
  2.697s: String dict StringName index read
  8.529s: StringName assign create
  3.408s: StringName dict String index read
  1.727s: StringName dict StringName index read

[KoBeWi]

I tested this time and it fixes #68834 for real, without affecting usability.

[Repiteo]

Nice performance boost & helps iron out those last few string/stringname discrepancies, LGTM!

[Calinou]

Tested locally (rebased on top of master aaa4560), it works as expected.

However, I can't really discern a noticeable performance difference before and after this PR. It's slightly faster in some cases (StringName assign create) but is slower in others (StringName dict String index read).

Using a release x86_64 Linux export template binary (optimize=speed lto=full) for running the MRP.

Binary size with this PR is 4 KB smaller compared to master.

Before

  1.451s: String keys create
 33.451s: StringName keys create
  4.333s: String assign create
  0.482s: String dict String index read
  0.546s: String dict StringName index read
  4.511s: StringName assign create
  0.503s: StringName dict String index read
  0.561s: StringName dict StringName index read

After

  1.473s: String keys create
 33.515s: StringName keys create
  4.403s: String assign create
  0.483s: String dict String index read
  0.566s: String dict StringName index read
  4.322s: StringName assign create
  0.638s: StringName dict String index read
  0.461s: StringName dict StringName index read

PC specifications

• CPU: Intel Core i9-13900K
• GPU: NVIDIA GeForce RTX 4090
• RAM: 64 GB (2×32 GB DDR5-5800 C30)
• SSD: Solidigm P44 Pro 2 TB
• OS: Linux (Fedora 39)

[rune-scape]

it may be because StringName == String doesnt do an in-place comparison like it could, copying the internal String of the StringName (or allocating a new String if not from GDScript) before comparing it leading to an extra ref then deref, ive already made a fix including this change on my branch, ill find some time to test production builds with the settings u mentioned on all 3 variants [before PR, after PR, after PR with performance fix]

[KoBeWi]

The performance impact is what #68834 is about.

[dalexeev]

Makes sense to me, and is essential for supporting typed dictionaries. It has some quirks, but the tradeoff between performance and compatibility seems worth it. String and StringName are interchangeable in most cases, so this shouldn't cause major compatibility breaks. However, users who explicitly check the key type or rely on its string representation may have some issues.

[rune-scape]

rebased. and looked through comparisons to Variant::STRING again and changed a few more

also i recommend that #92570 gets merged too, so the performance gets better in all cases and not worse in some

[akien-mga]

var dict1 := { x = 1 }
var dict2 := { "x": 1 }
print(dict1) # { &"x": 1 }
print(dict2) # { "x": 1 }

I'm quite concerned about this, I can really see users running into unexpected bugs when dict1.x == dict2.x will be false, and they won't understand why.

I'm not sure it's worth it to introduce this kind of inconsistency in the API just based on how you declared a dictionary, for a relatively minor performance gain. The tradeoff seems bad a priori (didn't review the changes and discussion in depth).

[KoBeWi]

I can really see users running into unexpected bugs when dict1.x == dict2.x will be false, and they won't understand why.

From what I understand, this would return true actually. The idea is that Dictionaries behave the same as currently, but internally allow StringNames instead of always converting.

[rune-scape]

yes dictionaries should behave the same as before (because of StringLikeVariantComparator), just that retrieving a key might give you a string name when it gave a string before (meaning comparing the type of a key to TYPE_STRING might not work anymore, maybe there should be an is_string global gdscript method for convenience and to make updating code easier)

theres a later PR i have to add support for iterating stringname characters with a for loop (relies on #92570)
might be good to have, with another way of leaking more stringnames into code

theres also less than and greater than comparisons but to match that to string, the behavior of the stringname comparisons would need to change, so probly not

[dalexeev]

I'm quite concerned about this, I can really see users running into unexpected bugs when dict1.x == dict2.x will be false, and they won't understand why.

I'm still a bit worried about this kind of inconsistency too. However, in practice it shouldn't cause major issues, since String and StringName are interchangeable in most operations, including comparison and matching. The exceptions are type checking (is, is_instance_of(), typeof()) and strict comparison (is_same()).

Conceptually, slightly¹ different dictionary contents depending on the style sounds bad, but on the other hand it makes some sense. If you use Lua style, you probably have a fixed key set and use dictionary.property syntax, not dictionary["property"].

I think we could test this as early as possible and adjust the behavior later if users report issues. Or let's wait for @vnen's opinion on this.

Footnotes

1. As far as I remember, { &"x": 1 } == { "x": 1 }. Please correct me if I am wrong. ↩

[vnen]

1. The following behavior may be confusing:

@tool
extends EditorScript

func _run() -> void:
    var dict := {
        #"x": 1,
        #&"x": 2, # Error: Key "x" was already used in this dictionary (at line 6).
    }
    dict["x"] = 1
    dict[&"x"] = 2
    dict[&"y"] = 3
    dict["y"] = 4
    print(dict) # { "x": 2, &"y": 4 }

This is the same as the current behavior, the only difference is that currently all keys will be strings even if you us StringName. While in this case it might feel odd, I believe it is expected that using String to access a StringName key just replaces the value, not the key (and in practice does not even matter much, given how interchangeable they are).

2. There are some compatibility breakage and inconsistency between Lua- and Python-style dictionaries:

var dict1 := { x = 1 }
var dict2 := { "x": 1 }
print(dict1) # { &"x": 1 }
print(dict2) # { "x": 1 }

The only breakage is printing pretty much. I don't think we guarantee compatibility to that level, relying on printed serialization was always finicky.

1. As far as I remember, { &"x": 1 } == { "x": 1 }. Please correct me if I am wrong.

That is correct. Even with this PR this is still true.

---

IMO the trade-off is good. The Lua-style was always supposed to use StringNames and only changed because of the issues with the type difference. But those issues are pretty much solved now, so it makes sense to allow users have better performance when they need it.

[vnen]

Small nitpick, otherwise looks good to me.

[vnen]

LGTM

[akien-mga]

Thanks!

[Faless]

This is the same as the current behavior, the only difference is that currently all keys will be strings even if you us StringName.

As I discovered in #96915 , one side effect of this change is that the following code:

extends Node

func _ready() -> void:
	var dict := {
		&"B": "test",
		&"A": "test"
	}
	var keys := dict.keys()
	keys.sort() # Will sort strings alphabetically, but StringNames by pointer.
	print(keys)

Prints:

["A", "B"] # Until Godot 4.3
[&"B", &"A"] # Since Godot 4.4 (after this PR)

While this is a consequence of the StringName not being casted to String, the resulting behavior may not be so obvious.

Since I suspect this case might be quite common across people updating, I would suggest we mention it (prominently) in the release notes.