FLIF-hub / FLIF

Free Lossless Image Format
Other
3.72k stars 229 forks source link

Changes flif and libflif to link from object files #436

Closed Traneptora closed 7 years ago

Traneptora commented 7 years ago

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.

hrj commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

Traneptora commented 7 years ago

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 commented 7 years ago

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