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

Optimize Font calculations by avoiding unnecessary copy-on-write. #102132

Merged
merged 1 commit into from
Feb 7, 2025
+19 −18
Merged

Optimize Font calculations by avoiding unnecessary copy-on-write. #102132

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
37 changes: 19 additions & 18 deletions scene/resources/font.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,10 @@ real_t Font::get_height(int p_font_size) const {
if (dirty_rids) {
_update_rids();
}

real_t ret = 0.f;
for (int i = 0; i < rids.size(); i++) {
ret = MAX(ret, TS->font_get_ascent(rids[i], p_font_size) + TS->font_get_descent(rids[i], p_font_size));
ret = MAX(ret, TS->font_get_ascent(rids.get(i), p_font_size) + TS->font_get_descent(rids.get(i), p_font_size));
}
return ret + get_spacing(TextServer::SPACING_BOTTOM) + get_spacing(TextServer::SPACING_TOP);
}
Expand All @@ -223,7 +224,7 @@ real_t Font::get_ascent(int p_font_size) const {
}
real_t ret = 0.f;
for (int i = 0; i < rids.size(); i++) {
ret = MAX(ret, TS->font_get_ascent(rids[i], p_font_size));
ret = MAX(ret, TS->font_get_ascent(rids.get(i), p_font_size));
}
return ret + get_spacing(TextServer::SPACING_TOP);
}
Expand All @@ -234,7 +235,7 @@ real_t Font::get_descent(int p_font_size) const {
}
real_t ret = 0.f;
for (int i = 0; i < rids.size(); i++) {
ret = MAX(ret, TS->font_get_descent(rids[i], p_font_size));
ret = MAX(ret, TS->font_get_descent(rids.get(i), p_font_size));
}
return ret + get_spacing(TextServer::SPACING_BOTTOM);
}
Expand All @@ -245,7 +246,7 @@ real_t Font::get_underline_position(int p_font_size) const {
}
real_t ret = 0.f;
for (int i = 0; i < rids.size(); i++) {
ret = MAX(ret, TS->font_get_underline_position(rids[i], p_font_size));
ret = MAX(ret, TS->font_get_underline_position(rids.get(i), p_font_size));
}
return ret + get_spacing(TextServer::SPACING_TOP);
}
Expand All @@ -256,7 +257,7 @@ real_t Font::get_underline_thickness(int p_font_size) const {
}
real_t ret = 0.f;
for (int i = 0; i < rids.size(); i++) {
ret = MAX(ret, TS->font_get_underline_thickness(rids[i], p_font_size));
ret = MAX(ret, TS->font_get_underline_thickness(rids.get(i), p_font_size));
}
return ret;
}
Expand Down Expand Up @@ -476,9 +477,9 @@ Size2 Font::get_char_size(char32_t p_char, int p_font_size) const {
_update_rids();
}
for (int i = 0; i < rids.size(); i++) {
if (TS->font_has_char(rids[i], p_char)) {
int32_t glyph = TS->font_get_glyph_index(rids[i], p_font_size, p_char, 0);
return Size2(TS->font_get_glyph_advance(rids[i], p_font_size, glyph).x, get_height(p_font_size));
if (TS->font_has_char(rids.get(i), p_char)) {
int32_t glyph = TS->font_get_glyph_index(rids.get(i), p_font_size, p_char, 0);
return Size2(TS->font_get_glyph_advance(rids.get(i), p_font_size, glyph).x, get_height(p_font_size));
}
}
return Size2();
Expand All @@ -489,10 +490,10 @@ real_t Font::draw_char(RID p_canvas_item, const Point2 &p_pos, char32_t p_char,
_update_rids();
}
for (int i = 0; i < rids.size(); i++) {
if (TS->font_has_char(rids[i], p_char)) {
int32_t glyph = TS->font_get_glyph_index(rids[i], p_font_size, p_char, 0);
TS->font_draw_glyph(rids[i], p_canvas_item, p_font_size, p_pos, glyph, p_modulate);
return TS->font_get_glyph_advance(rids[i], p_font_size, glyph).x;
if (TS->font_has_char(rids.get(i), p_char)) {
int32_t glyph = TS->font_get_glyph_index(rids.get(i), p_font_size, p_char, 0);
TS->font_draw_glyph(rids.get(i), p_canvas_item, p_font_size, p_pos, glyph, p_modulate);
return TS->font_get_glyph_advance(rids.get(i), p_font_size, glyph).x;
Comment on lines +493 to +496
Copy link
Member

Choose a reason for hiding this comment

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

Would we gain anything from extracting rids.get(i) to a const variable, or will the compiler optimize it all the same?

Copy link
Member Author

@Ivorforce Ivorforce Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good question!
Array.get has the overhead of a function call, an if statement that is likely to be predicted correctly (read_only), and another if statement that is likely to be predicted correctly (bad index). If the array is large, there will also be an unnecessary cache contention (through repeated access of size() and dereferencing ArrayPrivate).
All in all, it could be optimized, but it's unlikely to make an observable difference, since all these factors should add up to only ~20 wasted clock cycles1 for each get call in most cases - and draw_char isn't particularly problematic in the profiler right now anyway.

If I were to make another rebase, I'd probably cache the reference to be safe, but through this estimation I don't think it's particularly important.

Footnotes

  1. Napkin math via Agner Fog's heuristics. ↩

Copy link
Contributor

Choose a reason for hiding this comment

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

60 cycles gained for adding a single line seems like a good trade off to me :)

Copy link
Member Author

@Ivorforce Ivorforce Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I actually implemented this and was close to pushing, but then found this bug when checking related code to make sure it's safe.
It doesn't appear that Font.rids is read-only, but I didn't want to risk ruining a simple PR by introducing a potential regression, so I would like to keep it as-is for now. I plan to make a PR switching Font.rids to Vector for 4.5 anyway, and I'll just add the cache there I think.

}
}
return 0.f;
Expand All @@ -503,10 +504,10 @@ real_t Font::draw_char_outline(RID p_canvas_item, const Point2 &p_pos, char32_t
_update_rids();
}
for (int i = 0; i < rids.size(); i++) {
if (TS->font_has_char(rids[i], p_char)) {
int32_t glyph = TS->font_get_glyph_index(rids[i], p_font_size, p_char, 0);
TS->font_draw_glyph_outline(rids[i], p_canvas_item, p_font_size, p_size, p_pos, glyph, p_modulate);
return TS->font_get_glyph_advance(rids[i], p_font_size, glyph).x;
if (TS->font_has_char(rids.get(i), p_char)) {
int32_t glyph = TS->font_get_glyph_index(rids.get(i), p_font_size, p_char, 0);
TS->font_draw_glyph_outline(rids.get(i), p_canvas_item, p_font_size, p_size, p_pos, glyph, p_modulate);
return TS->font_get_glyph_advance(rids.get(i), p_font_size, glyph).x;
}
}
return 0.f;
Expand All @@ -518,7 +519,7 @@ bool Font::has_char(char32_t p_char) const {
_update_rids();
}
for (int i = 0; i < rids.size(); i++) {
if (TS->font_has_char(rids[i], p_char)) {
if (TS->font_has_char(rids.get(i), p_char)) {
return true;
}
}
Expand All @@ -531,7 +532,7 @@ String Font::get_supported_chars() const {
}
String chars;
for (int i = 0; i < rids.size(); i++) {
String data_chars = TS->font_get_supported_chars(rids[i]);
String data_chars = TS->font_get_supported_chars(rids.get(i));
for (int j = 0; j < data_chars.length(); j++) {
if (chars.find_char(data_chars[j]) == -1) {
chars += data_chars[j];
Expand Down