Skip to content
This repository has been archived by the owner on Oct 30, 2021. It is now read-only.

Refactor followup: enable LTO #117

Closed
springmeyer opened this issue Feb 23, 2018 · 7 comments
Closed

Refactor followup: enable LTO #117

springmeyer opened this issue Feb 23, 2018 · 7 comments

Comments

@springmeyer
Copy link
Contributor

springmeyer commented Feb 23, 2018

After #116 lands, because #166 splits into multiple .cpp files, we should enable LTO for the build.

Learn more about LTO via https://github.com/mapbox/cpp/blob/lto-glossary-entry/glossary.md#link-time-optimization

/cc @aaaandrea @apendleton

refs mapbox/cpp#49

@apendleton
Copy link
Contributor

@springmeyer it looks from cursory Googling like lld already supports LTO, so just to be sure I'm understanding: this should just be adding -flto to cppflags and ldflags in our binding.gyp, right?

@springmeyer
Copy link
Contributor Author

@apendleton I'll get a working example going in node-cpp-skel next and then ping you when ready. Up to now I've used binutils-gold for linking and not lld and I'd like to give lld a shot.

@springmeyer
Copy link
Contributor Author

@apendleton I'll get a working example going in node-cpp-skel next

Here is a working example you can follow: mapbox/vtquery#63

Up to now I've used binutils-gold for linking and not lld and I'd like to give lld a shot.

Ideally need to do this mapbox/mason#561 for easy lld usage, so I think sticking with binutils linker (per the vtquery example PR above) is ideal.

@apendleton
Copy link
Contributor

@springmeyer we gave this a go and it seems not to be working, I think because we're trying to link to previously-built static libraries from Mason that were compiled with a different LLVM version (rocksdb, etc.). We get this error:

/home/travis/build/mapbox/carmen-cache/mason_packages/.link/bin/ld: error: LLVM gold plugin has failed to create LTO module: Invalid value (Producer: 'LLVM5.0.0git-30c4647' Reader: 'LLVM 3.9.1')

see for reference the job: https://travis-ci.org/mapbox/carmen-cache/jobs/346996737

Thoughts on how to proceed?

@springmeyer
Copy link
Contributor Author

@apendleton yes, LTO requires a consistent major clang++ version to be used. That is my oversight: so due to the external dep we'll need to:

So, I recommend you continue the cleanup work without this LTO addition, to make sure you'r not blocked. We can hit it as a followup once the mason fix is available.

@springmeyer
Copy link
Contributor Author

Mason fix is looking good at #119

@apendleton
Copy link
Contributor

Done in #116

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

No branches or pull requests

2 participants