Re: [Maria-developers] Latest libmysqld.so patch
Sergei Golubchik <serg@askmonty.org> writes: Thanks for review! What I would now like to do is to discuss how to proceeed (where to push this basically). On the one hand, the patch got rather more intrusive than I first wanted. The main issue is that now the mysys/mystrings/dbug code is linked dynamically by default, so these additional libmysys.so etc. libraries must be installed by packagers etc. for things to work; similarly extra -lmysys etc. is needed when using libmysqld.a. On the other hand, I think this is generally an improvement, linking code dynamically rather than statically. Saves duplicating code and so on, and dynamic linking is where the world is moving (has moved, really). I even tested the overhead of dynamic linking, it is two clock cycles per function call into a different library (I plan to write a short blog about that). So I think it is mainly a question on where we want to risk pushing this: 5.1, 5.2, or 5.3? We could even add it to the list of patches in the .debs for 5.1. (BTW, Gentoo people seems to have picked up the patch already in their MySQL and MariaDB 5.1 packages). And btw, I would like your help/hints on how to extend mysql_config to provide the right information for how to link. With those general remarks, here follow comments, inline:
Can I branch this your tree from somewhere ?
bzr+ssh://bazaar.launchpad.net/~knielsen/maria/mariadb-5.1-mwl74-libmysqld.so/
The plugin may optionally specify @plugin_static_if_no_embedded@ in CFLAGS/CXXFLAGS for the static target; this will avoid needlessly compiling with -fPIC if no libmysqld is being built.
I failed to understand this paragraph :(
With this patch, every source file for a plugin is compiled twice, once with -fPIC and once without. However, if we are not building libmysqld, then the -fPIC versions will never be used. In this case, @plugin_static_if_no_embedded@ will add -static to CFLAGS, avoiding the needless compilations with -fPIC. Otherwise it will be empty. Hope this was clearer. [There is BTW another similar issue: when building a convenience library, libtool will again compile sources both with and without -fPIC. But it only ever uses the -fPIC objects in the convenience library, the objects without -fPIC are not used. I even tested the newest libtool version, and also found an email thread discussing this (and agreeing that the problem is there)].
This patch only does what is necessary to build libmysqld.so. There are some more cleanups that are possible now that we are using libtool more fully, which will be done in subsequent patches:
- In libmysql_r/, we should be able to just link libmysys.la etc, instead of symlinking and re-compiling sources into the directory.
- In libmysql/, we can similarly avoid symlinking and recompiling sources if we instead build a libmysys_nothread.la library with appropriate CFLAGS and link that.
I'd rather not build libmysql at all. Or, at least, not by default.
Right, let's add this on top of this patch once we find out where we want to push it? If we link libmysql_r with libmysys, it will also solve another issue I have (with conflicts between C and assembler versions of bmovXXX); I postponed this issue until we decided about linking libmysql_r with mysys etc (it seems you agree we should do this).
=== modified file 'config/ac-macros/plugins.m4' --- config/ac-macros/plugins.m4 2010-09-18 07:53:48 +0000 +++ config/ac-macros/plugins.m4 2010-09-27 12:41:05 +0000 @@ -115,18 +115,32 @@ dnl ------------------------------------ dnl Macro: MYSQL_PLUGIN_STATIC dnl dnl SYNOPSIS -dnl MYSQL_PLUGIN_STATIC([name],[libmyplugin.a]) +dnl MYSQL_PLUGIN_STATIC([name],[libmyplugin.a],[libmyplugin_embedded.a]) dnl dnl DESCRIPTION -dnl Declare the name for the static library +dnl Declare the name for the static library +dnl +dnl Third argument is optional, only needed for special plugins that depend +dnl on server internals and have source files that must be compiled specially +dnl with -DEMBEDDED_LIBRARY for embedded server. If specified, the third +dnl argument is used to link embedded server instead of the second.
Does that mean that all plugin files will be rebuilt with -DEMBEDDED_LIBRARY if libmyplugin_embedded.a is specified ?
No. It means that the plugin Makefile.am has the possibility to specify a different library to be used for embedded. Later in the patch I modify all plugins that need it (ie. they before specified MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS) so that they build a convenience library (once) with all the source files that do not depend on internals. And compile the few source files that do depend on internals in two versions with or without -DEMBEDDED_LIBRARY. And then link those together to two different libraries, one for server, one for embedded. With this, it's basically up to the plugin Makefile.am to do what it wants.
-AC_DEFUN([MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS],[ - MYSQL_REQUIRE_PLUGIN([$1]) - m4_define([MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS_]AS_TR_CPP([$1]), [$2]) -])
I thought we agreed on IRC to keep it. Do you mean, you tried it and it didn't work?
Yes. I tried quite hard to find a way to keep compatibility. But failed :-( In the end I could not think of anything sensible to do for MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS(), so I removed it. This is related to your question below, about keeping compatibility so that eg. PBXT could still work unmodified between MySQL and MariaDB. I thought a lot about how to do this, but in the end failed. The hardest problem is that to link libmysqld.so with libtool, we need plugins to provide libXXX.la files in MYSQL_PLUGIN_STATIC(). And those libXXX.la simply will not work with the magic loop in MySQL sources that extracts objects from libXXX.a, removes duplicates, and links them back again. At least I couldn't see how to do it. So in the end I gave up on this compatibility. Sad, however a small comfort is that - This only hits plugins that say MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS(), which ideally they shouldn't anyway. - This only hits static plugins, which in any case need an incompatible maria_declare_plugin() added in the source (though this can be handled with #ifdef; I wonder if there is m4/automake equivalent ifdef that could help here?) If you have an idea on how to achieve compatibility and do something sensible for MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS(), then let me know ...
+# __MYSQL_EMIT_CHECK_PLUGIN arguments: +# +# 1 - plugin identifying name +# 2 - plugin identifying name, with `-' replaced by `_' +# 3 - plugin long name +# 4 - plugin description +# 5 - mysql_plugin_define (eg. WITH_xxx_STORAGE_ENGINE) +# 6 - directory +# 7 - static target (if supports static build) +# 8 - dynamic target (if supports dynamic build) +# 9 - mandatory flag +# 10 - disabled flag +# 11 - static target for libmysqld (if different from mysqld)
perhaps "if different from $7" ?
Fixed, thanks.
+# 12 - actions
better use 'dnl' here, not '#'
because '#'-lines will go into configure script, where they will mean nothing, as the macro will be already expanded.
Fixed
+dnl If not building libmysqld embedded server, then there is no need to build +dnl shared object versions of static plugins.
heh :) There, on the contrary, I'd rather use '#' for comments, not 'dnl'
Ok, I see, thanks for helping an m4 newbee :-) Fixed.
=== modified file 'configure.in' --- configure.in 2010-08-27 14:12:44 +0000 +++ configure.in 2010-09-13 20:17:09 +0000 @@ -2846,9 +2845,6 @@ if test "$with_server" != "no" -o "$THRE then AC_DEFINE([THREAD], [1], [Define if you want to have threaded code. This may be undef on client code]) - # Avoid _PROGRAMS names - THREAD_LOBJECTS="thr_alarm.o thr_lock.o thr_mutex.o thr_rwlock.o my_pthread.o my_thr_init.o mf_keycache.o mf_keycaches.o waiting_threads.o" - AC_SUBST(THREAD_LOBJECTS)
What was that? Definitions for mysys/Makefile.am to know what to build (or not) depending on whether threads are enabled ?
Yes. This is fixing an inconsistency in the original sources. These source files should only be compiled when threads are enabled. There was code to deal with this in two places: here in configure.in, and in mysys/Makefile.am. Each place handled a different set of files, and there was overlap (maybe due to merge error?) Anyway, with pure static libraries, this overlap did not cause problems apparently, but with libtool and .so linking, it caused a failure. So I decided to clean it up to be done in one place only.
=== modified file 'dbug/Makefile.am' --- dbug/Makefile.am 2010-08-27 14:12:44 +0000 +++ dbug/Makefile.am 2010-09-28 11:23:14 +0000 @@ -16,10 +16,10 @@ # MA 02111-1307, USA
INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include -LDADD = libdbug.a ../mysys/libmysys.a ../strings/libmystrings.a -pkglib_LIBRARIES = libdbug.a +LDADD = libdbug.la ../mysys/libmysys.la ../strings/libmystrings.la
better to use $(top_builddir)/libmysys.la etc (here and below)
Fixed (throughout the patch)
How's the build time - new, libmysql.so vs. the old libmysqld.a ?
Thanks for remind me, I checked. These are on a quad-core with no ccache: BUILD/compile-pentium64-valgrind-max: Before: 3m19.454s After: 4m18.947s So build time _is_ increased somewhat ...
Paul would apparently want his engine to work both with MySQL and MariaDB. Could we somehow make it possible (applies to other engines too)?
(See discussion above) Thanks, - Kristian.
Hi, Kristian! On Sep 30, Kristian Nielsen wrote:
Sergei Golubchik <serg@askmonty.org> writes:
Thanks for review!
What I would now like to do is to discuss how to proceeed (where to push this basically).
On the one hand, the patch got rather more intrusive than I first wanted. The main issue is that now the mysys/mystrings/dbug code is linked dynamically by default, so these additional libmysys.so etc. libraries must be installed by packagers etc. for things to work; similarly extra -lmysys etc. is needed when using libmysqld.a.
I've fixed that - now libmysys/mystrings/dbug/vio are built statically again. And they're included in libmysqld. See two patches attached. But they're still built with -fPIC, otherwise libmysqld.so cannot take them. Unless --disable-shared was used, that is. I had to hack around automake - as it didn't want to install static convenience libraries. I hope it'll go away eventually - I don't see why we should install them at all.
On the other hand, I think this is generally an improvement, linking code dynamically rather than statically. Saves duplicating code and so on, and dynamic linking is where the world is moving (has moved, really).
I agree. But perhaps we'd better postpone this (making them dynamic) to 5.3. Besides, let's see what cmake based builds bring in 5.5.
And btw, I would like your help/hints on how to extend mysql_config to provide the right information for how to link.
Looks fine the way it is. We may want to document that for libtool enabled builds it's better to link with /usr/lib/mysql/libmysqld.la instead of `mysql_config --libmysqld-libs` because libtool knows all the dependencies and required linker options.
The plugin may optionally specify @plugin_static_if_no_embedded@ in CFLAGS/CXXFLAGS for the static target; this will avoid needlessly compiling with -fPIC if no libmysqld is being built.
With this patch, every source file for a plugin is compiled twice, once with -fPIC and once without.
However, if we are not building libmysqld, then the -fPIC versions will never be used.
In this case, @plugin_static_if_no_embedded@ will add -static to CFLAGS, avoiding the needless compilations with -fPIC. Otherwise it will be empty.
I'm not a big fan of providing @plugin_static_if_no_embedded@, @plugin_embedded_defs@ etc - and allowing plugins to use them. I'd rather prefer us to append the flags automatically, where needed. But let's not spend too much time on the perfect solution as we're moving to cmake based build system anyway.
I'd rather not build libmysql at all. Or, at least, not by default.
Right, let's add this on top of this patch once we find out where we want to push it?
ok
If we link libmysql_r with libmysys, it will also solve another issue I have (with conflicts between C and assembler versions of bmovXXX); I postponed this issue until we decided about linking libmysql_r with mysys etc (it seems you agree we should do this).
ok
-AC_DEFUN([MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS],[ - MYSQL_REQUIRE_PLUGIN([$1]) - m4_define([MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS_]AS_TR_CPP([$1]), [$2]) -])
I thought we agreed on IRC to keep it. Do you mean, you tried it and it didn't work?
Yes.
I tried quite hard to find a way to keep compatibility. But failed :-(
So in the end I gave up on this compatibility. Sad, however a small comfort is that
- This only hits plugins that say MYSQL_PLUGIN_DEPENDS_ON_MYSQL_INTERNALS(), which ideally they shouldn't anyway.
- This only hits static plugins, which in any case need an incompatible maria_declare_plugin() added in the source (though this can be handled with #ifdef; I wonder if there is m4/automake equivalent ifdef that could help here?)
okay. because 1. I don't want us to spend much time on this, as cmake based build system is coming (yup, I've said it for the third time :) 2. I only care that pbxt is affected, but pbxt is in our tree so we can apply our changes - not in the Paul's tree - and merge his changed periodically while keeping ours (bzr can handle that). I would prefer us to use vanilla pbxt, but as these are only changes in Makefile.am and plug.in, and taking 1 into account (didn't want to repeat it again :), I'm think it's fine to keep modified Makefile.am and plug.in in storage/pbxt in our tree. Assuming Paul doesn't mind. Regards, Sergei
participants (2)
-
Kristian Nielsen -
Sergei Golubchik