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

StbTrueTypeFont: Use fabsf instead of fabs #73

Merged
merged 1 commit into from Nov 12, 2019

Conversation

Squareys
Copy link
Contributor

Hey @mosra !

As discussed via gitter, the performance of StbTrueTypeFont on MSVC Debug builds is crazy bad. Through profiling I figured out that all of the time is spent in fabs() during rasterization of the Glyphs in fillGlyphCache.
In embedded stbfreetype in ImGui this is not an issue for some reason, and I found that they override STBTT_fabs(x) with fabsf(x), the float override of fabs.
This didn't entirely fix the issue (imgui is still over 10x faster at loading a font), but at least 30% speedup.

I tried to figure out if there are different compile flags applied during compilation of imgui, but that doesn't seem to be the case.

Best,
Jonathan.

Signed-off-by: Squareys <squareys@googlemail.com>
@codecov-io
Copy link

codecov-io commented Nov 11, 2019

Codecov Report

Merging #73 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage      91%   90.96%   -0.04%     
==========================================
  Files          53       40      -13     
  Lines        4480     3410    -1070     
==========================================
- Hits         4077     3102     -975     
+ Misses        403      308      -95
Impacted Files Coverage Δ
.../MagnumPlugins/StbTrueTypeFont/StbTrueTypeFont.cpp 96.29% <ø> (+1%) ⬆️
...agnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp 92.36% <0%> (-0.55%) ⬇️
src/Magnum/OpenDdl/Document.h 88.57% <0%> (-0.32%) ⬇️
...agnumPlugins/StanfordImporter/StanfordImporter.cpp 93.59% <0%> (-0.16%) ⬇️
...mPlugins/JpegImageConverter/JpegImageConverter.cpp 96.1% <0%> (-0.15%) ⬇️
src/Magnum/OpenDdl/OpenDdl.cpp 90.64% <0%> (-0.12%) ⬇️
src/MagnumPlugins/OpenGexImporter/openGexSpec.hpp 100% <0%> (ø) ⬆️
src/Magnum/OpenDdl/Validation.h 100% <0%> (ø) ⬆️
src/MagnumPlugins/FreeTypeFont/FreeTypeFont.cpp
...mPlugins/BasisImageConverter/BasisImageConverter.h
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c036eed...9f613eb. Read the comment docs.

@mosra mosra added this to the 2019.1c milestone Nov 11, 2019
#include <math.h>
/* Use fabsf instead of fabs (double version) for 30% performance
improvement on MSVC Debug builds */
#define STBTT_fabs(x) fabsf(x)
Copy link
Owner

Choose a reason for hiding this comment

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

👍 for the comment explaining why

@mosra mosra added this to TODO in Asset management via automation Nov 11, 2019
@mosra mosra merged commit 9f613eb into mosra:master Nov 12, 2019
Asset management automation moved this from TODO to Done Nov 12, 2019
@mosra
Copy link
Owner

mosra commented Nov 12, 2019

Thank you!

@mosra
Copy link
Owner

mosra commented Nov 15, 2019

Hmm. This causes GCC to emit a warning:

../src/external/stb/stb_truetype.h: In function ‘void stbtt__rasterize_sorted_edges(stbtt__bitmap*, stbtt__edge*, int, int, int, int, void*)’:
../src/MagnumPlugins/StbTrueTypeFont/StbTrueTypeFont.cpp:41:30: warning: use of old-style cast to ‘float’ [-Wold-style-cast]
   41 | #define STBTT_fabs(x) fabsf(x)
      |                              ^

I'll need to investigate why it's using doubles at all. Maybe that's responsible for the remaining perf drop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants