Skip to content

Remove unnecessary StringName idx cache in _Data to reduce its size. #105760

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

Merged
merged 1 commit into from
Apr 28, 2025
Merged

Remove unnecessary StringName idx cache in _Data to reduce its size. #105760

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
113 changes: 51 additions & 62 deletions core/string/string_name.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,38 +33,31 @@
#include "core/os/os.h"
#include "core/string/print_string.h"

bool StringName::_Data::operator==(const String &p_name) const {
return name == p_name;
}

bool StringName::_Data::operator!=(const String &p_name) const {
return !operator==(p_name);
}
struct StringName::Table {
constexpr static uint32_t TABLE_BITS = 16;
constexpr static uint32_t TABLE_LEN = 1 << TABLE_BITS;
constexpr static uint32_t TABLE_MASK = TABLE_LEN - 1;

bool StringName::_Data::operator==(const char *p_name) const {
return name == p_name;
}

bool StringName::_Data::operator!=(const char *p_name) const {
return !operator==(p_name);
}
static inline _Data *table[TABLE_LEN];
Copy link
Contributor

Choose a reason for hiding this comment

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

Does static inline _Data *table[TABLE_LEN] = {}; not 0 initialize it, so we could remove the setup call altogether ?

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 would. But it's out of scope. It probably makes sense to allocate at runtime anyway to avoid the 500kb bloating the binary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought standard forced static arrays to be zero initialized anyway, but I'm not sure with inline. And it would probably be put in the bss section, so it would not bloat anything (but making sure no compiler options is messing with that would be the no fun part)

Copy link
Member Author

@Ivorforce Ivorforce Apr 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

And it would probably be put in the bss section, so it would not bloat anything (but making sure no compiler options is messing with that would be the no fun part)

I'm pretty sure bloaty told me the table made up 500kb of the final binary. I was planning on testing this again (along with potential speed impact) when I prepare the according PR.

I thought standard forced static arrays to be zero initialized anyway, but I'm not sure with inline.

Makes sense to me! inline in this case just means it is defined where it's declared (and doesn't have to be put into the cpp), so that shouldn't have any effect.

static inline Mutex mutex;
};

void StringName::setup() {
ERR_FAIL_COND(configured);
for (int i = 0; i < STRING_TABLE_LEN; i++) {
_table[i] = nullptr;
for (uint32_t i = 0; i < Table::TABLE_LEN; i++) {
Table::table[i] = nullptr;
}
configured = true;
}

void StringName::cleanup() {
MutexLock lock(mutex);
MutexLock lock(Table::mutex);

#ifdef DEBUG_ENABLED
if (unlikely(debug_stringname)) {
Vector<_Data *> data;
for (int i = 0; i < STRING_TABLE_LEN; i++) {
_Data *d = _table[i];
for (uint32_t i = 0; i < Table::TABLE_LEN; i++) {
_Data *d = Table::table[i];
while (d) {
data.push_back(d);
d = d->next;
Expand All @@ -77,7 +70,7 @@ void StringName::cleanup() {
int unreferenced_stringnames = 0;
int rarely_referenced_stringnames = 0;
for (int i = 0; i < data.size(); i++) {
print_line(itos(i + 1) + ": " + data[i]->get_name() + " - " + itos(data[i]->debug_references));
print_line(itos(i + 1) + ": " + data[i]->name + " - " + itos(data[i]->debug_references));
if (data[i]->debug_references == 0) {
unreferenced_stringnames += 1;
} else if (data[i]->debug_references < 5) {
Expand All @@ -90,9 +83,9 @@ void StringName::cleanup() {
}
#endif
int lost_strings = 0;
for (int i = 0; i < STRING_TABLE_LEN; i++) {
while (_table[i]) {
_Data *d = _table[i];
for (uint32_t i = 0; i < Table::TABLE_LEN; i++) {
while (Table::table[i]) {
_Data *d = Table::table[i];
if (d->static_count.get() != d->refcount.get()) {
lost_strings++;

Expand All @@ -101,7 +94,7 @@ void StringName::cleanup() {
}
}

_table[i] = _table[i]->next;
Table::table[i] = Table::table[i]->next;
memdelete(d);
}
}
Expand All @@ -115,18 +108,16 @@ void StringName::unref() {
ERR_FAIL_COND(!configured);

if (_data && _data->refcount.unref()) {
MutexLock lock(mutex);
MutexLock lock(Table::mutex);

if (CoreGlobals::leak_reporting_enabled && _data->static_count.get() > 0) {
ERR_PRINT("BUG: Unreferenced static string to 0: " + _data->name);
}
if (_data->prev) {
_data->prev->next = _data->next;
} else {
if (_table[_data->idx] != _data) {
ERR_PRINT("BUG!");
}
_table[_data->idx] = _data->next;
const uint32_t idx = _data->hash & Table::TABLE_MASK;
Table::table[idx] = _data->next;
}

if (_data->next) {
Expand All @@ -145,15 +136,15 @@ uint32_t StringName::get_empty_hash() {

bool StringName::operator==(const String &p_name) const {
if (_data) {
return _data->operator==(p_name);
return _data->name == p_name;
}

return p_name.is_empty();
}

bool StringName::operator==(const char *p_name) const {
if (_data) {
return _data->operator==(p_name);
return _data->name == p_name;
}

return p_name[0] == 0;
Expand Down Expand Up @@ -217,7 +208,7 @@ StringName::StringName(const StringName &p_name) {
}

void StringName::assign_static_unique_class_name(StringName *ptr, const char *p_name) {
MutexLock lock(mutex);
MutexLock lock(Table::mutex);
if (*ptr == StringName()) {
*ptr = StringName(p_name, true);
}
Expand All @@ -233,14 +224,14 @@ StringName::StringName(const char *p_name, bool p_static) {
}

const uint32_t hash = String::hash(p_name);
const uint32_t idx = hash & STRING_TABLE_MASK;
const uint32_t idx = hash & Table::TABLE_MASK;

MutexLock lock(mutex);
_data = _table[idx];
MutexLock lock(Table::mutex);
_data = Table::table[idx];

while (_data) {
// compare hash first
if (_data->hash == hash && _data->operator==(p_name)) {
if (_data->hash == hash && _data->name == p_name) {
break;
}
_data = _data->next;
Expand All @@ -264,8 +255,7 @@ StringName::StringName(const char *p_name, bool p_static) {
_data->refcount.init();
_data->static_count.set(p_static ? 1 : 0);
_data->hash = hash;
_data->idx = idx;
_data->next = _table[idx];
_data->next = Table::table[idx];
_data->prev = nullptr;

#ifdef DEBUG_ENABLED
Expand All @@ -275,10 +265,10 @@ StringName::StringName(const char *p_name, bool p_static) {
_data->static_count.increment();
}
#endif
if (_table[idx]) {
_table[idx]->prev = _data;
if (Table::table[idx]) {
Table::table[idx]->prev = _data;
}
_table[idx] = _data;
Table::table[idx] = _data;
}

StringName::StringName(const String &p_name, bool p_static) {
Expand All @@ -291,13 +281,13 @@ StringName::StringName(const String &p_name, bool p_static) {
}

const uint32_t hash = p_name.hash();
const uint32_t idx = hash & STRING_TABLE_MASK;
const uint32_t idx = hash & Table::TABLE_MASK;

MutexLock lock(mutex);
_data = _table[idx];
MutexLock lock(Table::mutex);
_data = Table::table[idx];

while (_data) {
if (_data->hash == hash && _data->operator==(p_name)) {
if (_data->hash == hash && _data->name == p_name) {
break;
}
_data = _data->next;
Expand All @@ -321,8 +311,7 @@ StringName::StringName(const String &p_name, bool p_static) {
_data->refcount.init();
_data->static_count.set(p_static ? 1 : 0);
_data->hash = hash;
_data->idx = idx;
_data->next = _table[idx];
_data->next = Table::table[idx];
_data->prev = nullptr;
#ifdef DEBUG_ENABLED
if (unlikely(debug_stringname)) {
Expand All @@ -332,10 +321,10 @@ StringName::StringName(const String &p_name, bool p_static) {
}
#endif

if (_table[idx]) {
_table[idx]->prev = _data;
if (Table::table[idx]) {
Table::table[idx]->prev = _data;
}
_table[idx] = _data;
Table::table[idx] = _data;
}

StringName StringName::search(const char *p_name) {
Expand All @@ -347,14 +336,14 @@ StringName StringName::search(const char *p_name) {
}

const uint32_t hash = String::hash(p_name);
const uint32_t idx = hash & STRING_TABLE_MASK;
const uint32_t idx = hash & Table::TABLE_MASK;

MutexLock lock(mutex);
_Data *_data = _table[idx];
MutexLock lock(Table::mutex);
_Data *_data = Table::table[idx];

while (_data) {
// compare hash first
if (_data->hash == hash && _data->operator==(p_name)) {
if (_data->hash == hash && _data->name == p_name) {
break;
}
_data = _data->next;
Expand Down Expand Up @@ -382,14 +371,14 @@ StringName StringName::search(const char32_t *p_name) {
}

const uint32_t hash = String::hash(p_name);
const uint32_t idx = hash & STRING_TABLE_MASK;
const uint32_t idx = hash & Table::TABLE_MASK;

MutexLock lock(mutex);
_Data *_data = _table[idx];
MutexLock lock(Table::mutex);
_Data *_data = Table::table[idx];

while (_data) {
// compare hash first
if (_data->hash == hash && _data->operator==(p_name)) {
if (_data->hash == hash && _data->name == p_name) {
break;
}
_data = _data->next;
Expand All @@ -406,14 +395,14 @@ StringName StringName::search(const String &p_name) {
ERR_FAIL_COND_V(p_name.is_empty(), StringName());

const uint32_t hash = p_name.hash();
const uint32_t idx = hash & STRING_TABLE_MASK;
const uint32_t idx = hash & Table::TABLE_MASK;

MutexLock lock(mutex);
_Data *_data = _table[idx];
MutexLock lock(Table::mutex);
_Data *_data = Table::table[idx];

while (_data) {
// compare hash first
if (_data->hash == hash && _data->operator==(p_name)) {
if (_data->hash == hash && _data->name == p_name) {
break;
}
_data = _data->next;
Expand Down
15 changes: 1 addition & 14 deletions core/string/string_name.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,7 @@
class Main;

class StringName {
enum {
STRING_TABLE_BITS = 16,
STRING_TABLE_LEN = 1 << STRING_TABLE_BITS,
STRING_TABLE_MASK = STRING_TABLE_LEN - 1
};
struct Table;

struct _Data {
SafeRefCount refcount;
Expand All @@ -52,28 +48,19 @@ class StringName {
#ifdef DEBUG_ENABLED
uint32_t debug_references = 0;
#endif
const String &get_name() const { return name; }
bool operator==(const String &p_name) const;
bool operator!=(const String &p_name) const;
bool operator==(const char *p_name) const;
bool operator!=(const char *p_name) const;

int idx = 0;
uint32_t hash = 0;
_Data *prev = nullptr;
_Data *next = nullptr;
_Data() {}
};

static inline _Data *_table[STRING_TABLE_LEN];

_Data *_data = nullptr;

void unref();
friend void register_core_types();
friend void unregister_core_types();
friend class Main;
static inline Mutex mutex;
static void setup();
static void cleanup();
static uint32_t get_empty_hash();
Expand Down