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.

Sign up for GitHub

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

Jump to bottom

Optimize Node::add_child validation #75760

Merged
merged 1 commit into from
Apr 7, 2023
Merged

Optimize Node::add_child validation #75760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 73 additions & 6 deletions core/string/ustring.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2593,6 +2593,23 @@ double String::to_float(const wchar_t *p_str, const wchar_t **r_end) {
return built_in_strtod<wchar_t>(p_str, (wchar_t **)r_end);
}

uint32_t String::num_characters(int64_t p_int) {
int r = 1;
if (p_int < 0) {
r += 1;
if (p_int == INT64_MIN) {
p_int = INT64_MAX;
} else {
p_int = -p_int;
}
}
while (p_int >= 10) {
Copy link
Member

Choose a reason for hiding this comment

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

if (p_int < 10) return r+0;
if (p_int < 100) return r+1;
// ...

may be faster because it doesn't do divisions, but I didn't test.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is probably faster, but the code is a mess, specially because this is int64 :P Still, in this context the code should be ok as-is, you won't get a really meaningful performance gain.

p_int /= 10;
r++;
}
return r;
}

int64_t String::to_int(const char32_t *p_str, int p_len, bool p_clamp) {
if (p_len == 0 || !p_str[0]) {
return 0;
Expand Down Expand Up @@ -4561,15 +4578,65 @@ String String::property_name_encode() const {
}

// Changes made to the set of invalid characters must also be reflected in the String documentation.
const String String::invalid_node_name_characters = ". : @ / \" " UNIQUE_NODE_PREFIX;

static const char32_t invalid_node_name_characters[] = { '.', ':', '@', '/', '\"', UNIQUE_NODE_PREFIX[0], 0 };

String String::get_invalid_node_name_characters() {
// Do not use this function for critical validation.
String r;
const char32_t *c = invalid_node_name_characters;
while (*c) {
if (c != invalid_node_name_characters) {
r += " ";
}
r += String::chr(*c);
c++;
}
return r;
}

String String::validate_node_name() const {
Vector<String> chars = String::invalid_node_name_characters.split(" ");
String name = this->replace(chars[0], "");
for (int i = 1; i < chars.size(); i++) {
name = name.replace(chars[i], "");
// This is a critical validation in node addition, so it must be optimized.
const char32_t *cn = ptr();
if (cn == nullptr) {
return String();
}
return name;
bool valid = true;
uint32_t idx = 0;
while (cn[idx]) {
const char32_t *c = invalid_node_name_characters;
while (*c) {
if (cn[idx] == *c) {
valid = false;
break;
}
c++;
}
if (!valid) {
break;
}
idx++;
}

if (valid) {
return *this;
}

String validated = *this;
char32_t *nn = validated.ptrw();
while (nn[idx]) {
const char32_t *c = invalid_node_name_characters;
while (*c) {
if (nn[idx] == *c) {
nn[idx] = '_';
break;
}
c++;
}
idx++;
}

return validated;
}

String String::get_basename() const {
Expand Down
3 changes: 2 additions & 1 deletion core/string/ustring.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ class String {
static double to_float(const char *p_str);
static double to_float(const wchar_t *p_str, const wchar_t **r_end = nullptr);
static double to_float(const char32_t *p_str, const char32_t **r_end = nullptr);
static uint32_t num_characters(int64_t p_int);

String capitalize() const;
String to_camel_case() const;
Expand Down Expand Up @@ -432,7 +433,7 @@ class String {
String property_name_encode() const;

// node functions
static const String invalid_node_name_characters;
static String get_invalid_node_name_characters();
String validate_node_name() const;
String validate_identifier() const;
String validate_filename() const;
Expand Down
2 changes: 1 addition & 1 deletion editor/scene_tree_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ void SceneTreeEditor::_renamed() {
String new_name = raw_new_name.validate_node_name();

if (new_name != raw_new_name) {
error->set_text(TTR("Invalid node name, the following characters are not allowed:") + "\n" + String::invalid_node_name_characters);
error->set_text(TTR("Invalid node name, the following characters are not allowed:") + "\n" + String::get_invalid_node_name_characters());
error->popup_centered();

if (new_name.is_empty()) {
Expand Down
25 changes: 23 additions & 2 deletions scene/main/node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,8 +994,29 @@ void Node::_validate_child_name(Node *p_child, bool p_force_human_readable) {

if (!unique) {
ERR_FAIL_COND(!node_hrcr_count.ref());
String name = "@" + String(p_child->get_name()) + "@" + itos(node_hrcr_count.get());
reduz marked this conversation as resolved.
Show resolved Hide resolved
p_child->data.name = name;
// Optimized version of the code below:
// String name = "@" + String(p_child->get_name()) + "@" + itos(node_hrcr_count.get());
uint32_t c = node_hrcr_count.get();
String cn = p_child->get_class_name().operator String();
const char32_t *cn_ptr = cn.ptr();
uint32_t cn_length = cn.length();
uint32_t c_chars = String::num_characters(c);
uint32_t len = 2 + cn_length + c_chars;
char32_t *str = (char32_t *)alloca(sizeof(char32_t) * (len + 1));
Copy link
Member

Choose a reason for hiding this comment

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

This buffer isn't deleted (the string constructor doesn't take ownership of it)

Also would it make sense to use a thread static shared buffer of like size 64 or so and only allocate this temporary buffer if that is too small?

Alternatively make this buffer a CowData<char32_t> and add a String constructor that takes ownership of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

alloca() allocates on the stack. It's the most efficient way of allocation for this kind of usage and it does not need to be freed.

Copy link
Member

Choose a reason for hiding this comment

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

Or you can just simplest solution that is already implemented; nvm then

uint32_t idx = 0;
str[idx++] = '@';
for (uint32_t i = 0; i < cn_length; i++) {
str[idx++] = cn_ptr[i];
}
str[idx++] = '@';
idx += c_chars;
ERR_FAIL_COND(idx != len);
str[idx] = 0;
while (c) {
str[--idx] = '0' + (c % 10);
c /= 10;
}
p_child->data.name = String(str);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions tests/core/string/test_string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1678,8 +1678,8 @@ TEST_CASE("[String] validate_node_name") {
String name_with_kana = U"Name with kana ゴドツ";
CHECK(name_with_kana.validate_node_name() == U"Name with kana ゴドツ");

String name_with_invalid_chars = "Name with invalid characters :.@removed!";
CHECK(name_with_invalid_chars.validate_node_name() == "Name with invalid characters removed!");
String name_with_invalid_chars = "Name with invalid characters :.@%removed!";
CHECK(name_with_invalid_chars.validate_node_name() == "Name with invalid characters ____removed!");
}

TEST_CASE("[String] validate_identifier") {
Expand Down