Skip to content
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

innobase/{lock0lock,dict0dict}: add noexcept to lock/unlock methods #3531

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

MaxKellermann
Copy link
Contributor

@MaxKellermann MaxKellermann commented Sep 20, 2024

  • The Jira issue number for this PR is: MDEV-34973

Description

MariaDB is compiled with C++ exceptions enabled, and that disallows
some optimizations (e.g. the stack must always be unwinding-safe). By
adding noexcept to functions that are guaranteed to never throw,
some of these optimizations can be regained. Low-level locking
functions that are called often are a good candidate for this.

This shrinks the executable a bit (tested with GCC 14 on aarch64):

text	  data	   bss	   dec	   hex	filename

24448910 2436488 9473185 36358583 22ac9b7 build/release/sql/mariadbd
24448474 2436488 9473601 36358563 22ac9a3 build/release/sql/mariadbd

(See #3529 (comment))

Release Notes

No runtime effect.

How can this PR be tested?

No runtime effect.

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@dr-m
Copy link
Contributor

dr-m commented Sep 20, 2024

Thank you! I knew that noexpect was introduced in C++11, but I didn’t study what it actually does. I was assuming that it mainly exists for documentation or API safety purposes, similar to override or private. There are indeed few throw in our code base, I guess it would be mostly an implicit std::bad_alloc in the few memory allocations that do not use a custom allocator. In InnoDB there could be a few uses of STL collections that would use a default allocator. Anywhere else, it could make sense to add noexpect.

I think that this one needs to be rebased on the 10.6 branch, where the locking primitives were refactored and which most our customers using, as part of the MariaDB Enterprise Server offering. Can you also file a MDEV for this?

@MaxKellermann
Copy link
Contributor Author

I created https://jira.mariadb.org/browse/MDEV-34973 and rebased the PR on branch 10.6

@MaxKellermann
Copy link
Contributor Author

I could not build the 10.5 branch because:

$ git submodule update --init --recursive
fatal: transport 'file' not allowed
fatal: Fetched in submodule path 'storage/columnstore/columnstore', but it did not contain 9f905c07b9c5ba68590a7cec24e71859d4082d56. Direct fetching of that commit failed.

... and my bet that I could just fix the merge conflict and be done didn't work out.

@MaxKellermann MaxKellermann force-pushed the lock_noexcept branch 3 times, most recently from ed11519 to e236afb Compare September 21, 2024 09:15
@MaxKellermann
Copy link
Contributor Author

Hooray for try'n'error with CI logs. It now builds.

@MaxKellermann
Copy link
Contributor Author

I could not build the 10.5 branch because:

$ git submodule update --init --recursive
fatal: transport 'file' not allowed
fatal: Fetched in submodule path 'storage/columnstore/columnstore', but it did not contain 9f905c07b9c5ba68590a7cec24e71859d4082d56. Direct fetching of that commit failed.

By the way, I solved this problem. Just in case anybody else stumbles over this and finds this page: it was because I renamed the origin remote (git remote rename origin upstream), because I wanted to point a new origin to my own repository. But the git submodule update command then uses the top-level remote name for fetches on submodules, but in the submodules, the remote was still named origin. Therefore git tried to fetch from a directory called upstream, leading first to the error transport 'file' not allowed and then to a failure to find the referenced commit. This is a weird error condition!

MariaDB is compiled with C++ exceptions enabled, and that disallows
some optimizations (e.g. the stack must always be unwinding-safe).  By
adding `noexcept` to functions that are guaranteed to never throw,
some of these optimizations can be regained.  Low-level locking
functions that are called often are a good candidate for this.

This shrinks the executable a bit (tested with GCC 14 on aarch64):

    text	  data	   bss	   dec	   hex	filename
 24448910	2436488	9473185	36358583	22ac9b7	build/release/sql/mariadbd
 24448622	2436488	9473537	36358647	22ac9f7	build/release/sql/mariadbd
Another chance for cutting back overhead due to C++ exceptions being
enabled; the `dict_sys_t` class is a good candidate because its
locking methods are called frequently.

Binary size reduction this time:

    text	  data	   bss	   dec	   hex	filename
 24448622	2436488	9473537	36358647	22ac9f7	build/release/sql/mariadbd
 24448474	2436488	9473601	36358563	22ac9a3	build/release/sql/mariadbd
@vuvova
Copy link
Member

vuvova commented Sep 23, 2024

@MaxKellermann I've reported it to git developers a year ago, https://www.spinics.net/lists/git/msg462320.html
Still, unfortunately, unfixed. I'm gradually moving away from git submodule update to

git submodule foreach 'git fetch origin $sha1; git checkout FETCH_HEAD'

@FooBarrior
Copy link
Contributor

@MaxKellermann did you see the difference in the binary output with/without noexcept?
We use -fno-exceptions and I suppose it should supersed noexcept, besides otherwise we'd have to add it to 90% of functions.

@MaxKellermann
Copy link
Contributor Author

@MaxKellermann did you see the difference in the binary output with/without noexcept?

Yes and I described those change in the commit message & PR text.

We use -fno-exceptions and I suppose it should supersed noexcept, besides otherwise we'd have to add it to 90% of functions.

With -fno-exceptions, noexcept is implied, true so far. But I didn't see -fno-exceptions when I compiled MariaDB and MariaDB does use C++ exceptions, so it's not possible to disable them.

@dr-m
Copy link
Contributor

dr-m commented Oct 1, 2024

I see -fno-exceptions in cmake/build_configurations/mysql_release.cmake, but it is inside something that looks like a reference to a pre-clang Intel compiler, probably pre-C++11 as well. It also occurs in a few BUILD/compile-* scripts and some storage engines. Interestingly, there is this code in include/my_global.h:

#if defined(__GNUC) && defined(__EXCEPTIONS)
#error "Please add -fno-exceptions to CXXFLAGS and reconfigure/recompile"
#endif

The intention is clear, but the implementation is incorrect. Yes, GCC would define __cpp_exceptions and __EXCEPTIONS if and only if -fno-exceptions was not specified. But, GCC does not define __GNUC; it defines __GNUC__ instead. One can easily check this by the following:

touch t.cc
g++ -E -dM -fno-exceptions t.cc|grep -i except
g++ -E -dM t.cc|grep -i except

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ MaxKellermann
❌ dr-m
You have signed the CLA already but the status is still pending? Let us recheck it.

@dr-m dr-m enabled auto-merge (rebase) October 1, 2024 06:31
@dr-m dr-m merged commit 6715e4d into MariaDB:10.6 Oct 1, 2024
9 of 12 checks passed
@MaxKellermann MaxKellermann deleted the lock_noexcept branch October 8, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants