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

TextServerAdvanced: 2x performance improvement by removing redundant lookups #92575

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

rune-scape
Copy link
Contributor

@rune-scape rune-scape commented May 31, 2024

  • removes redundant font for size cache lookups
  • also caches editor setting
  • usies a faster hash method on the FontForSizeAdvanced cache (its the same function its just not converting the key to a variant before hashing)
  • ShapedTextDataAdvanced::valid changed to an SafeFlag to avoid locking a mutex, as its read Very often

in total from my benchmark it takes ~39% of the time to render the same text in a Label (~45% in CodeEdit, the main script editor in godot)
all of these optimizations came from profiling a build using target=editor optimize=speed_trace they all gave at least a 10% improvement to speed at the time of profiling
now when profiled, the actual hotpath goes to freetype rendering and shaping and such

benchmark: TextServerTest.zip
benchmark compile flags: scons target=template_release production=yes optimize=speed lto=full tests=yes deprecated=no scu_build=yes module_text_server_adv_enabled=yes module_text_server_fb_enabled=no
benchmark run command: ./godot/bin/godot.linuxbsd.template_release.x86_64 --disable-vsync ./TextServerTest/project.godot

results before:

Godot Engine v4.3.beta.custom_build.6b38be699 (2024-05-30 20:38:39 UTC) - https://godotengine.org
Vulkan 1.3.277 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce GTX 1080 Ti

18.1370287230521ms avg draw time (99 samples)
18.1469531974407ms avg draw time (198 samples)
18.1223431018868ms avg draw time (297 samples)
18.159361198695ms avg draw time (396 samples)
18.1944639995845ms avg draw time (495 samples)
18.1336226286712ms avg draw time (594 samples)
18.14051692792ms avg draw time (693 samples)
18.1234557219226ms avg draw time (792 samples)
18.0797271856957ms avg draw time (891 samples)
18.0556610377148ms avg draw time (990 samples)
18.0405357542993ms avg draw time (1089 samples)

after:

Godot Engine v4.3.beta.custom_build.02f8b6443 (2024-05-30 22:54:24 UTC) - https://godotengine.org
Vulkan 1.3.277 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce GTX 1080 Ti

7.03977334378946ms avg draw time (99 samples)
7.00848030321526ms avg draw time (198 samples)
7.01666921878905ms avg draw time (297 samples)
7.02174504597982ms avg draw time (396 samples)
7.03076449307528ms avg draw time (495 samples)
7.036976942711ms avg draw time (594 samples)
7.03303741686272ms avg draw time (693 samples)
7.02608956231011ms avg draw time (792 samples)
7.01870688150494ms avg draw time (891 samples)
7.01467798213766ms avg draw time (990 samples)
7.0109612576123ms avg draw time (1089 samples)

the last line of each is the most important, its the average of all frame times (minus some longer startup frames)

the same improvements can be made to TextServerFallback

@rune-scape rune-scape requested a review from a team as a code owner May 31, 2024 04:09
@bruvzg bruvzg self-requested a review May 31, 2024 04:50
@rune-scape rune-scape force-pushed the rune-text-srvr-cacher branch 4 times, most recently from 7d3036b to e7b3339 Compare May 31, 2024 06:39
@Calinou Calinou added this to the 4.x milestone May 31, 2024
@rune-scape rune-scape force-pushed the rune-text-srvr-cacher branch 2 times, most recently from bf9054c to ffa6459 Compare May 31, 2024 08:43
@rune-scape rune-scape force-pushed the rune-text-srvr-cacher branch from ffa6459 to 186c993 Compare May 31, 2024 11:00
@rune-scape rune-scape force-pushed the rune-text-srvr-cacher branch 2 times, most recently from efd0372 to c60a94a Compare May 31, 2024 11:19
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

The only thing I would change is returning value from the _ensure_cache_for_size as pointer instead of reference, to avoid unnecessary local variable which are unused in most cases.

-	FontForSizeAdvanced *ffsd = nullptr;
-	ERR_FAIL_COND_V(!_ensure_cache_for_size(fd, size, ffsd), 0);
+	ERR_FAIL_COND_V(!_ensure_cache_for_size(fd, size, nullptr), 0);

@rune-scape rune-scape force-pushed the rune-text-srvr-cacher branch from c60a94a to e0ce638 Compare July 4, 2024 02:03
@clayjohn clayjohn modified the milestones: 4.x, 4.4 Jul 19, 2024
@akien-mga
Copy link
Member

Needs a rebase, which should show a compilation error (tested locally):

In file included from ./core/string/char_utils.h:34,
                 from ./core/string/ustring.h:36,
                 from modules/text_server_adv/script_iterator.h:46,
                 from modules/text_server_adv/text_server_adv.h:39,
                 from modules/text_server_adv/text_server_adv.cpp:31:
modules/text_server_adv/text_server_adv.cpp: In member function 'virtual PackedInt32Array TextServerAdvanced::_font_get_supported_glyphs(const RID&) const':
modules/text_server_adv/text_server_adv.cpp:3603: error: no matching function for call to 'TextServerAdvanced::_ensure_cache_for_size(TextServerAdvanced::FontAdvanced*&, Vector2i) const'
 3603 |                 ERR_FAIL_COND_V(!_ensure_cache_for_size(fd, fd->msdf ? Vector2i(fd->msdf_source_size, 0) : Vector2i(16, 0)), PackedInt32Array());
./core/typedefs.h:272:41: note: in definition of macro 'unlikely'
  272 | #define unlikely(x) __builtin_expect(!!(x), 0)
      |                                         ^
modules/text_server_adv/text_server_adv.cpp:3603: note: in expansion of macro 'ERR_FAIL_COND_V'
 3603 |                 ERR_FAIL_COND_V(!_ensure_cache_for_size(fd, fd->msdf ? Vector2i(fd->msdf_source_size, 0) : Vector2i(16, 0)), PackedInt32Array());
modules/text_server_adv/text_server_adv.cpp:1360: note: candidate: 'bool TextServerAdvanced::_ensure_cache_for_size(FontAdvanced*, const Vector2i&, FontForSizeAdvanced*&) const'
 1360 | _FORCE_INLINE_ bool TextServerAdvanced::_ensure_cache_for_size(FontAdvanced *p_font_data, const Vector2i &p_size, FontForSizeAdvanced *&r_cache_for_size) const {
modules/text_server_adv/text_server_adv.cpp:1360: note:   candidate expects 3 arguments, 2 provided

@rune-scape rune-scape force-pushed the rune-text-srvr-cacher branch from e0ce638 to 37d0d62 Compare August 28, 2024 20:00
+ caching editor setting
+ using a faster hash method on the FontForSizeAdvanced cache
+ SafeFlag for ShapedTextDataAdvanced::valid
@rune-scape rune-scape force-pushed the rune-text-srvr-cacher branch from 37d0d62 to 4ba7738 Compare August 28, 2024 20:01
@akien-mga akien-mga merged commit 03d1f43 into godotengine:master Aug 29, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@rune-scape rune-scape deleted the rune-text-srvr-cacher branch August 30, 2024 02:25
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.

6 participants