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

Changes flif and libflif to link from object files #436

Merged
merged 1 commit into from
Jun 7, 2017

Conversation

Traneptora
Copy link
Contributor

@Traneptora Traneptora commented Jun 7, 2017

The flif binary should not linked against libflif.so because this does not work on some systems
Compiling cpp files to objects avoids compiling them twice

This essentially attempts to redo the functionality of #416 properly. The goal of that PR was to not compile the cpp files twice, once for flif and once for libflif, but by linking flif to libflif, we're taking advantage of gcc's symbol exporting in a way that we really shouldn't. This could break on many systems.

This should redo the functionality, in that we only compile the cpp code to machine code once with gcc, and then we link the object files to libflif and also flif. This also renders #434 unnecessary should it be merged, because flif is linked statically and doesn't depend on libflif.

This also has the added benefit that it is easier to debug flif because it doesn't also rely on the library.

This doesn't change the build code for dflif or libflif_dec.so because those need to be compiled with different CXXFLAGS. This is best fixed by having a proper configure script rather than a separate make target.

The flif binary should not linked against libflif.so because this does not work on some systems
Compiling cpp files to objects avoids compiling them twice
@hrj
Copy link
Member

hrj commented Jun 7, 2017

Have you done any performance and binary size benchmarking with this new method, comparing it to the original (prior to #416) and the latest master (after #416)?

@Traneptora
Copy link
Contributor Author

I have not, but it should be fairly comparable, and if anything it should be ever-so-slightly faster than just before this patch, because it's not a frontend to the library. Putting several source files together into one humongous source file before sending it to the compiler is only really advantageous if you're turning on -finline-functions, which we are not (because it's provided by -O3, not -O2. See the GCC docs.)

Keep in mind that we use -flto so it'll be reoptimized on linking.

However, any marginal speed differences compared to before this patch should not matter because the build is currently broken on some systems, like Windows. And even still, trying to compile the entire binary file directly from a mass of C++ sources files is bad practice and should be avoided.

Basically, if you are worried about speed, this is not how you fix it.

@hrj
Copy link
Member

hrj commented Jun 7, 2017

I don't know whether the speed differences are marginal. I hope someone does keep a tab on that.

@Traneptora
Copy link
Contributor Author

Traneptora commented Jun 7, 2017

If you really want some numbers, I did just do a quick-n-dirty heuristic test. I have two very large flif images (both 5184x3456) named gecko1.flif and gecko2.flif. I ran a quick test where I decoded gecko1.flif followed by gecko2.flif and then repeated 6 times, for a total of 12 decodes. Git master clocked 82.417 seconds, and with the new patch it clocked at 81.310 seconds, which is a 1.35 percent difference, easily within the reasonable margin of error of a sample size as small as this one. Test methodology was basically:

time for i in {1..6}; do flif -d gecko1.flif - >/dev/null; flif -d gecko2.flif - >/dev/null; done

@jonsneyers jonsneyers merged commit 8470149 into FLIF-hub:master Jun 7, 2017
@jonsneyers
Copy link
Member

Thanks! This is indeed a better way to build it, especially to reduce compile times when coding.

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.

3 participants