mapbox / node-cpp-skel

Skeleton for bindings to C++ libraries for Node.js using node-addon-api
Creative Commons Zero v1.0 Universal
72 stars 10 forks source link

Enable LTO #110

Closed springmeyer closed 6 years ago

springmeyer commented 6 years ago

For projects that use node-cpp-skel and end up having lots of .cpp files, LTO starts to become potentially important for best performance per https://github.com/mapbox/cpp/blob/master/glossary.md#link-time-optimization.

This ticket tracks getting an LTO example working in node-cpp-skel that other modules can follow. This is worthwhile but a little hairy since it requires:

Ideally as part of this effort we also document an exact, measurable situation when LTO actually helps. Having it help performance in theory is not worth the setup overhead, so we'll want to have a clear demonstration it helps perf. Although one caveat is that we should likely investigate using -fsanitize=cfi in production in the future and that requires LTO: (https://blog.trailofbits.com/2016/10/17/lets-talk-about-cfi-clang-edition/ and https://clang.llvm.org/docs/ControlFlowIntegrity.html).

Related:

springmeyer commented 6 years ago

now enabled after https://github.com/mapbox/node-cpp-skel/commit/93edf4f9c91d22b26a03e9e6c9d5b2a8a7a171bb

springmeyer commented 6 years ago

Ideally as part of this effort we also document an exact, measurable situation when LTO actually helps.

Never did this. Still worthwhile. The reason I merged without doing a perf validation benchmark is that:

  1. I noticed that adding -flto can catch ODR errors that are otherwise silent. So making sure we can compile with LTO increases the robustness of the code
  2. I wanted to support fsanitize=cfi which requires it