Created attachment 2618 Add -Bsymbolic-functions to XDT_FEATURE_LINKER_OPTS. Although removing the visibility checks from several modules makes the whole thing easier to understand, it does increase the PLT overhead a bit. Fortunately recent versions of ld support -Bsymbolic-functions which will decrease PLT overhead of shared libraries. It works fine here and I think it safe to use (also used by OOo). -Bsymbolic however could result in problems, so better not touch that.
Hmm... it does have one potentially-unwanted side-effect: you can't override symbols in the library later using LD_PRELOAD or similar. We might not care for some libraries, but maybe we would for others. (Though one could make the argument that this is bad practice and should be discouraged, it can be useful for debugging in some cases.) Another option might be to have some opt 'levels' that the macro can take, similar to how XDT_FEATURE_DEBUG works... but that feels like overdoing it a bit. Regardless of this, maybe we should disable linker opts when the debug level is 'full', maybe even 'yes' also. Need to think about this a bit.
Created attachment 2619 Add -Bsymbolic-functions to XDT_FEATURE_LINKER_OPTS, v2 Disabling the opts when debugging is active sounds like a good idea. I've disabled it when debugging is yes or full, and it throws an error when $enable_debug is not defined (self check if we call XDT_FEATURE_LINKER_OPTS before XDT_FEATURE_DEBUG).
Created attachment 2620 Add -Bsymbolic-functions to XDT_FEATURE_LINKER_OPTS, v2.1 Whoops, forgot the save before creating the patch...
I'll look at this tomorrow, but just a quick thought to toss out before I try to go back to bed: Would it make sense to AC_REQUIRE() XDT_FEATURE_DEBUG at the top of XDT_FEATURE_LINKER_OPTS? That way you don't even need to specify the former if you do the latter. Though I guess that could make the definition get emitted twice if they're both included. Is there a way to tell m4/autoconf "only expand this macro if it hasn't been expanded before, otherwise expand to nothing"? The problem with your idea of checking $enable_debug is that it doesn't allow using XDT_FEATURE_LINKER_OPTS without _DEBUG. While personally I think not using _DEBUG is silly, maybe someone might want the linker opts and not have a debug flag. At any rate, since _LINKER_OPTS has been around for such a short time, I think it's totally fine to require _DEBUG before _LINKER_OPTS in the next released version.
Another issue I just thought of... you probably only want this flag when building shared libraries, not when building applications. See: https://bugs.launchpad.net/ubuntu/+source/samba/+bug/234901 That just presents one example using gethostbyname(), but I imagine there are other cases. Of course XDT_FEATURE_LINKER_OPTS is useful for both apps and shared libs: --as-needed is useful for both, and -O1 fine to use for both (it currently only has an effect on shared libs, but may in the future do something nice for apps too). -Bsymbolic and -Bsymbolic-functions, however, shouldn't be used with apps. Ignoring that bit, packages like xfconf that have both a library and an application also hit this problem. Any thoughts on how to fix this? I guess we could only add safe flags to LDFLAGS, and do something like XDT_LIBRARY_LDFLAGS for library-only flags like this, but that strikes me as ugly and requires that the developer add that variable to their Makefile.am for the library target only, which is probably easy to forget (of course, forgetting doesn't break anything, so maybe it's not a big deal). And also I found this (see 2009-03-20): http://wiki.services.openoffice.org/wiki/Performance/Meetings/2009_03 Unless something changed since March, they actually decided for OOo *not* to use -Bsymbolic-functions.
I wouldn't have any problems with library specific LDFLAGS, since I can't think of another way to handle this in a package like xfconf/exo too. Configure in ooo looks for bsymbolic, so it looks like it is included in the source: http://svn.services.openoffice.org/ooo/trunk/configure.in (see also set_soenv.in) Anyway, with the library specific ldflags and disable all the ld flags when debugging; I think we should just give it a try. We can easily disable this at any time and while we're still early in the development cycle we can give it some good testing.
Hmm, I dunno... I still dislike this. Why are we doing this again? What do you mean about *removing* the visibility stuff? Given the relatively small number of relocatable symbols in our libraries, esp with gnuc visibility enabled, this seems like a risk with very little payoff. Any cold-cache benchmarks of load time with and without -Bsymbolic-functions? I think this is a case of needing to convince me that it's useful and doesn't break things up-front, not "it might be a good idea, so let's try it."
The previous alias stuff we used works in 2 ways: it handles public visibility of functions and reduces the PLT overhead for internal calls. However this is a somewhat complicated and a lot of times people forgot the update the symbols file. So, right now we handle the first part by using -export-symbols-regex and mark all private functions with _ or optionally use a .ver file. This technique is recommended by people like Drepper and the binutils folks. The other part is the internal calls. The alias code creates files to make aliases for the public functions starting with IA__ so all function calls inside the library are not going over the PLT, this is what -Bsymbolic-functions could do for us. I admit I did not know about the internal PLT stuff when removing this from some of our libraries and if -Bsymbolic-functions will be rejected I'll put the alias code back in the libraries. I only prefer -Bsymbolic-functions since it's less complicated. No benchmarks done on this, but you can see the result of -Bsymbolic-functions when using size on the library: text data bss dec hex filename 263976 7280 560 271816 425c8 libexo-1.so.normal 259416 6368 560 266344 41068 libexo-1.so.with.Bsymbolic.functions
For reference, compiled with visibility and the aliasdef technique, gives this: text data bss dec hex filename 261372 6320 560 268252 417dc /opt/xfce/lib/libexo-1.so.with.alias
After talking to Nick about this on IRC, I understand the situation as: We had alias code in exo before to create IA_ aliases for exo functions so that when an exo function called another exo function it wouldn't have to go through the PLT. -Bsymbolic-functions provides the same thing and with it we wouldn't have to manage aliases manually anymore. -Bsymbolic-functions however can break applications if passed to them when linking to e.g. exo. So, like the alias code we had for exo which was only applied to the library, not to any applications, we want to pass -Bsymbolic-functions only to libraries, right? I guess I'd be ok with an additional variable for library makefiles. The alias code is a real pain so I'd be happy to be able to get rid of it without losing functionality. A new makefile variable sounds like a fair trade-off.
Honestly, why bother? We're talking about a tiny tiny tiny reduction in binary size and a probably negligible performance increase, and the risk is breaking things in unknown ways by using a linker flag that clearly we don't completely understand. The visibility and alias stuff is a little annoying, but it's a known quantity and is something well-used by other projects... and we already have (had?) it set up. If you really want to find ways to increase performance, profile your app and fix bottlenecks. Unless I see real-world benchmarks showing that -Bsymbolic-functions actually has a material effect on performance, I don't see a reason to use it.
(In reply to comment #11) > Honestly, why bother? We're talking about a tiny tiny tiny reduction in binary > size and a probably negligible performance increase, and the risk is breaking > things in unknown ways by using a linker flag that clearly we don't completely > understand. > > The visibility and alias stuff is a little annoying, but it's a known quantity > and is something well-used by other projects... and we already have (had?) it > set up. AFAIK, we've always had it for thunar-vfs, exo and thunarx. We recently removed it from thunarx and from exo. Nick just added it back to exo. The aliasdef stuff is kinda ugly but since we're planning to keep the .symbols file around anyway, I guess it'd be ok to add it back to thunarx as well and continue to use it. > If you really want to find ways to increase performance, profile your app and > fix bottlenecks. Unless I see real-world benchmarks showing that > -Bsymbolic-functions actually has a material effect on performance, I don't see > a reason to use it. I'm wondering about this, too. The GLib guys told me that PLT lookups and relocations are a real concern to them which is why they are stuffing everything into GIO nowadays instead of separate GLib modules. But they couldn't back this argument up with numbers either.
My final goal was using a .ver file (which is more or less like the symbols file in our case) and pass that to -Wl,--version-script (instead of the -export-symbols-regex, which creates such a file on the fly). Then we have a 'symbols' file to maintain/check the api, which is directly passed to the linker and can also be used for the abicheck.sh test, -Bsymbolic-functions would've solved the internal PLT stuff. That is the simplest and also the most effective setup from a performance (whether that is measurable or not) and maintenance point of view. Anyway, already restored (and fixed) most of the alias stuff, but I do get a feeling out of all this that some people don't care because it works for them and so you don't have to look forward and try something. The performance thing is an easy one to toss because we all know it is hard to measure, but for that same bit of extra performance we also use the alias code.
Is it actually hard to measure? It's true that I don't really want to change it because what we have seems to work fine, and there's some risk in changing it. But it's a question of the benefit: if you're going to propose a change, then the burden is on your shoulders to prove that there's actually a benefit. Do you know how to benchmark this? If not, then you're just making a baseless claim about some unknown performance case. The only evidence of improvement you've provided is a 6% decrease (less than 2kB) in binary size for a single (relatively small 260kB) library. Not only is it an unreliable sample, I just don't think saving 6% (assuming it's 6% for every single library, which I doubt) of our total shared library footprint is all that impressive. If you prefer to use --version-script, why not just generate the .ver file from the .symbols file (or vice versa)? Also note that -Bsymbolic isn't about reducing PLT entries. It does do that as a side-effect, but its purpose is to bind symbol definitions so they can't be overridden. The modern/correct way of defining the public ABI is through visibility attributes. The version script method is also correct, though maybe "the old way" (not that there's anything wrong with that). -Bsymbolic has nothing to do with ABI. The safest way to reduce PLT entries is with the aliasing trick. Performance wise, I can't really say too much, but I'd guess there's less of a win here than you'd think. To call shared lib functions, the application *still* has to call into the GOT. Just with -Bsymbolic, instead of resolving the function address with GOT+PLT, it should get resolved with GOT+a static offset. This offset still needs to be determined, so you're not speeding up initial application load time (the relocation still has to happen anyway). As for actual calls to the function, I'm not sure how the runtime linker handles this. Either it will more or less generate a PLT on the fly (which is why the .so size is smaller), or it'll patch references in the binary at callsites with GOT+offset either at load time or on first call. If it does the former, I don't think there's really any benefit. If the latter, successive calls to the function should be faster, but I'm not sure how much. Anyway, that's all hand-wavy from memory and intuition (haven't looked at this stuff in years), so I might be off on how that works. But still, that doesn't really change the fact that the benefit seems negligible (pending proof either way), and what we have works well enough already. Now, if you're saying the alias stuff is a pain to set up, I'd certainly agree. But that doesn't justify throwing away the stuff that's already set up. I believe the script that generates the alias .c/.h files is in perl... if that could be rewritten in portable sh, we could stuff that in x-d-t as well, and then just require that the maintainer: 1. Put the macro in configure.ac 2. Maintain a file with a list of exported symbols 3. Put a rule substitution in the Makefile.am (similar to @INTLTOOL_DESKTOP_RULE@) that generates the alias .c/.h files 4. Include the files in the appropriate .c files. That would maintain the status quo with regard to functionality, but make setting up new libraries easier. The only annoying bit is #4, but maybe we can come up with a workaround for that, too.