-
Notifications
You must be signed in to change notification settings - Fork 39
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
add -flto=thin to mac aarch64 build cflags for ez 10% gain #469
add -flto=thin to mac aarch64 build cflags for ez 10% gain #469
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem approving this for macos/clang since afaik, there are no issues with -flto and including debug symbols in the build (unlike with gcc). That - missing debug symbols in release - may be a reason not to accept this for linux, but I'm not sure. Would leave that decision up to @joemfb.
I'm not sure what you mean. Currently on linux, debug symbols seem to be intact, at least in my local builds. According to
Unless I'm missing something? |
I'm unsure whether this is a bug with gcc, gdb, or perhaps what is more likely - that I am just Debug info is clearly intact on linux with the -flto option. That said, in gdb, anything that would There are definitely some expected changes to the dwarf. For instance, the functions
If I look at the dwarf info in more detail at, let's say u3m_pave, nothing really seems out of place
I really have no idea what's causing the bad behavior in gdb then. @fighet-parnet, any ideas? Can gdb version 13.1 fyi. Additionally, tried fixing the DWARF version with -gdwarf-5, or -gdwarf-3, |
ha, figured it out after reviewing https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html. Will update later |
Yeah so following up on the gdb weirdness I experienced. Basically looks like we need a
in .bazelrc. The gnu optimizations docs on -flto state " If any of the input files at link time were built with debug info generation enabled the link will enable debug info generation as well." So idk why this is necessary. Further note this warning: " Any elaborate debug info settings like the dwarf level -gdwarf-5 need to be explicitly repeated at the linker command line and mixing different settings in different translation units is discouraged. " It doesn't look like there's any problem with the Anyway, without the explicit P.S. look at this:
@fighet-parnet I think you were surprised to find that enabling -flto with |
Okay, @barter-simsum what do you think about this appropriately grug-brained way of adding copts with quite a bit more finer-grained control than before? |
8dbc3f3
to
ac6d9c7
Compare
Also note that this enables LTO on linux, but only on our code so by my previous testing this should work fine (though I haven't built this on linux yet.) |
@fighet-parnet fantastic. Looks like this solves the issue I ran into which was that by removing .bazelrc debug/release configurations and moving them to a dual cc_library, none of the copts specified with |
@fighet-parnet One thing I noticed is that the vere_library and vere_binary |
This adds a macro "vere_library" which supports our concepts of debug and release builds, and gives finer-grained control over which copts/linkopts are passed and when. Takes advantage of bazel's "compilation_mode={dbg,opt}" to control debug/optimized builds.
fef5f2d
to
f7a75b5
Compare
Ah yes, for some reason I thought |
Ok, I think the following changes would make a bit more sense. In top-level BUILD.bazel: 1) Just define release and debug -- side note, why precisely is this necessary? It seems like we're basically aliasing
2) Do the macos/linux dispatch in bazel/common_settings.bzl instead of the something like this:
|
https://bazel.build/docs/configurable-attributes#and-chaining I could move the config_settings (or just the LTO ones) out of the root BUILD but that seemed like a good spot for them. We're aliasing the compilation_mode because there doesn't seem to be a method to dispatch on |
Ah, that's too bad. Didn't realize select statements had that limitation. And LGETM |
Oh, one last thing. INSTALL.md needs to be updated with the new build commands. s/--config=dbg/--compilation_mode=dbg I'd update this myself, but it's on a branch you own |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aed
Adds somewhere between 5-10% improvement to wall time.
I've ran all tests, booted several fakeships and comets with this installed, and all seems to work properly.
Mac x86 is coming, I just don't have access to an intel mac right now [but soon...].
Linux will come later once I figure out some bugs.
The following is time to boot a fakeship from a brass pill:
180/164 -> 1.09
~barter-simsum
Brass pill boot:
x86 linux without lto: ~140s
x86 linux with lto: ~128s
~ 8.6% improvement - note we may be able to squeeze a bit more out if we can
apply
-flto
to all dependencies and not just urbit binaries. This is currentlyan issue for x86 linux though due to some weird uninvestigated behavior with
libsigsegv
For those curious, the following hints at what was inlined