Details
-
Task
-
Status: Open (View Workflow)
-
Major
-
Resolution: Unresolved
-
None
Description
To avoid gcc bugs in the production software (such as MDEV-19709), it could be better to
compile with less aggressive optimization.
Some people state that -O2 is faster than -O3, due to more compact code (less inlining or loop unrolling) and as result less cache misses . See https://stackoverflow.com/a/19985801/547065 for example.
The task is about benchmarking, and giving recommendations. From the quality standpoint, we can assume that -O2 would have less bug, and it is better tested, since it is the optimization used most often for production software.
From performance standpoint, the right -O level needs to be measured.
Attachments
Issue Links
- relates to
-
-
- Closed
-
-
MDEV-19709 Bitmap<128>::merge etc may crash on older GCC versions
-
- Closed
-
-
-
- Open
-
Activity
We still have this patch in Debian with mariadb-10.5. Could you axel give some opinion on this? Or marko or danblack?
See https://salsa.debian.org/mariadb-team/mariadb-10.5/-/tree/master/debian/patches
Because Ondrej Sury who participated in packaging some years back did
various things that 90% were good and 10% were bad. I would not throw
away stuff unless I know for sure that it was a bad part, therefore
this issue and question. We need at least one C/C++ expert to say if
this optimization is good or bad. At least it works and has been used
for many years now.
otto, the packaging should match what is most tested. At the moment, what's in the codebase is the most tested. The only buildbot -O2 forcing is with asan/msan builds. So I'd drop the patch on the basis that O3 is more tested.
I checked out a recent 11.6 build on Buildbot: https://buildbot.mariadb.org/#/builders/148/builds/35625
Seems is is now doing both -O2 and -O3:
cd /home/buildbot/amd64-debian-10-deb-autobake/build/builddir/storage/innobase && /usr/lib/ccache/c++ -DBTR_CUR_ADAPT -DBTR_CUR_HASH_ADAPT -DHAVE_CONFIG_H -DHAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE=1 -DHAVE_OPENSSL -DHAVE_PMEM -DLINUX_NATIVE_AIO -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE=1 -I/home/buildbot/amd64-debian-10-deb-autobake/build/wsrep-lib/include -I/home/buildbot/amd64-debian-10-deb-autobake/build/wsrep-lib/wsrep-API/v26 -I/home/buildbot/amd64-debian-10-deb-autobake/build/builddir/include -I/home/buildbot/amd64-debian-10-deb-autobake/build/include/providers -I/home/buildbot/amd64-debian-10-deb-autobake/build/storage/innobase/include -I/home/buildbot/amd64-debian-10-deb-autobake/build/storage/innobase/handler -I/home/buildbot/amd64-debian-10-deb-autobake/build/libbinlogevents/include -I/home/buildbot/amd64-debian-10-deb-autobake/build/tpool -I/home/buildbot/amd64-debian-10-deb-autobake/build/include -I/home/buildbot/amd64-debian-10-deb-autobake/build/sql -g -O2 -fdebug-prefix-map=/home/buildbot/amd64-debian-10-deb-autobake/build=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -Wdate-time -D_FORTIFY_SOURCE=2 -pie -fPIC -fstack-protector --param=ssp-buffer-size=4 -Wconversion -Wno-sign-conversion -O3 -g -static-libgcc -fno-omit-frame-pointer -fno-strict-aliasing -Wno-uninitialized -fno-omit-frame-pointer -D_FORTIFY_SOURCE=2 -DDBUG_OFF -Wall -Wenum-compare -Wextra -Wformat-security -Wmissing-braces -Wno-format-truncation -Wno-init-self -Wno-nonnull-compare -Wno-unused-parameter -Wnon-virtual-dtor -Woverloaded-virtual -Wvla -Wwrite-strings -Wdate-time -D_FORTIFY_SOURCE=2 -fvisibility=hidden -std=gnu++11 -o CMakeFiles/innobase.dir/fts/fts0fts.cc.o -c /home/buildbot/amd64-debian-10-deb-autobake/build/storage/innobase/fts/fts0fts.cc
|
Same as above but spaces replaced with newlines for readability:
cd
|
/home/buildbot/amd64-debian-10-deb-autobake/build/builddir/storage/innobase
|
&&
|
/usr/lib/ccache/c++
|
-DBTR_CUR_ADAPT
|
-DBTR_CUR_HASH_ADAPT
|
-DHAVE_CONFIG_H
|
-DHAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE=1
|
-DHAVE_OPENSSL
|
-DHAVE_PMEM
|
-DLINUX_NATIVE_AIO
|
-D_FILE_OFFSET_BITS=64
|
-D_GNU_SOURCE=1
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/wsrep-lib/include
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/wsrep-lib/wsrep-API/v26
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/builddir/include
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/include/providers
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/storage/innobase/include
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/storage/innobase/handler
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/libbinlogevents/include
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/tpool
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/include
|
-I/home/buildbot/amd64-debian-10-deb-autobake/build/sql
|
-g
|
-O2
|
-fdebug-prefix-map=/home/buildbot/amd64-debian-10-deb-autobake/build=.
|
-fstack-protector-strong
|
-Wformat
|
-Werror=format-security
|
-Wdate-time
|
-D_FORTIFY_SOURCE=2
|
-Wdate-time
|
-D_FORTIFY_SOURCE=2
|
-pie
|
-fPIC
|
-fstack-protector
|
--param=ssp-buffer-size=4
|
-Wconversion
|
-Wno-sign-conversion
|
-O3
|
-g
|
-static-libgcc
|
-fno-omit-frame-pointer
|
-fno-strict-aliasing
|
-Wno-uninitialized
|
-fno-omit-frame-pointer
|
-D_FORTIFY_SOURCE=2
|
-DDBUG_OFF
|
-Wall
|
-Wenum-compare
|
-Wextra
|
-Wformat-security
|
-Wmissing-braces
|
-Wno-format-truncation
|
-Wno-init-self
|
-Wno-nonnull-compare
|
-Wno-unused-parameter
|
-Wnon-virtual-dtor
|
-Woverloaded-virtual
|
-Wvla
|
-Wwrite-strings
|
-Wdate-time
|
-D_FORTIFY_SOURCE=2
|
-fvisibility=hidden
|
-std=gnu++11
|
-o
|
CMakeFiles/innobase.dir/fts/fts0fts.cc.o
|
-c
|
/home/buildbot/amd64-debian-10-deb-autobake/build/storage/innobase/fts/fts0fts.cc
|
Note how many things are duplicated, and there is both -O2 and -O3 in use. This seems buggy, or at least counterproductive for long-term maintenance of the build.
I tried to check the Fedora build for comparison (https://buildbot.mariadb.org/#/builders/576/builds/10727), but it isn't running in verbose mode so the compilation flags are not visible.
In Debian we have been carrying the patch https://salsa.debian.org/mariadb-team/mariadb-10.4/-/blob/master/debian/patches/0025-Change-the-default-optimization-from-O3-to-O2-in-mys.patch for some while. Should it be upstreamed or deleted?