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

compile a static crengine library #1676

Closed

Conversation

benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Oct 24, 2023

There's only one user: libkoreader-cre.so, compiling a static library helps reduce code size and improve performance (I have not benchmarked it again, but FWIR, it was about 10% faster). Additionally, change the default symbol visibility to inlinehidden for further code reduction.

Size reduction (as reported by size -t libs/*cre* using the debug build, no LTO):

Size (bss+data+code)
Before 3.8 MB
Static lib 3.6 MB
Static lib + inlinehidden visibility 3.3 MB

So a 463.1 kB reduction.


This change is Reviewable

@benoit-pierre benoit-pierre force-pushed the pr/static_crengine_lib branch from 3c62d2c to 470683e Compare October 24, 2023 23:48
@benoit-pierre
Copy link
Contributor Author

WTF: /usr/bin/ccache g++ -D-DALLOW_KERNING=1 -D-DCR3_PATCH=1 -D-DMATHML_SUPPORT=1 -D-DUSE_FONTCONFIG=0 -D-DUSE_FREETYPE=1 -D-DUSE_FRIBIDI=1 -D-DUSE_HARFBUZZ=1 -D-DUSE_LIBUNIBREAK=1 -D-DUSE_LIBWEBP=1 -D-DUSE_LUNASVG=1 -D-DUSE_NANOSVG=0 -D-DUSE_SRELL_REGEX=1 -D-DUSE_UTF8PROC=1 -D-DUSE_ZSTD=1 -DCHM_SUPPORT_ENABLED=1 -DCR3_ANTIWORD_PATCH=1 -DCR_EMULATE_GETTEXT -DDEBUG=1

@benoit-pierre benoit-pierre force-pushed the pr/static_crengine_lib branch from 470683e to 11382fc Compare October 25, 2023 00:17
There's only one user `libkoreader-cre.so`.
Set it to `inlineshidden`, since we compile a static library and don't
need to export symbols (all accesses go through `libkoreader-cre.so`).
@benoit-pierre benoit-pierre force-pushed the pr/static_crengine_lib branch from 11382fc to be9d658 Compare October 25, 2023 00:49
@benoit-pierre
Copy link
Contributor Author

Does not work for some reason (double free pointer crash in the emulator). Some difference in the flags used to compile cre.cpp and the crengine library? Normally I would compile the release build with LTO, as it's a good way to detect those issue (the linker will warn if some types have different definitions), but I can't compile the release build. Not because of the usual luajit issue (-ffast-math = borked minilua), but some other issue in the bowels of MuPDF… It looks the MuPDF build generates a debug lib while a release one is expected? Anyway, I give up.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 25, 2023

I don't usually have troubles with LTO builds, FWIW, but it's been a while since I last tried ;).

I'll fire one up to see how it fares...

@benoit-pierre
Copy link
Contributor Author

benoit-pierre commented Oct 25, 2023

LTO is not the issue, a standard release build fails.

@benoit-pierre
Copy link
Contributor Author

The release build issue is because of using if(DEFINED ENV{KODEBUG}) instead of if($ENV{KODEBUG}) in several thirdparty CMakeFileList.txt.

The release build with a static library does work for some reason.

Moving more compilation flags (pretty much all of them) out of thirdparty/kpvcrlib/CMakeLists.txt to Makefile.defs fix the problem with the debug build.

Will clean up and revisit later.

@benoit-pierre
Copy link
Contributor Author

Take 2: includes #1677, #1678, and use a custom crsetup.h header to avoid having to move the flags around.

@benoit-pierre
Copy link
Contributor Author

Ah, can't re-open after force pushing…

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

Successfully merging this pull request may close these issues.

2 participants