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

Open
wants to merge 2 commits into
base: 10.6
Choose a base branch
from

Conversation

MaxKellermann
Copy link

@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
Author

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

@MaxKellermann
Copy link
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 2 times, most recently from ce559a7 to ed11519 Compare September 21, 2024 08:55
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
@MaxKellermann
Copy link
Author

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

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.

2 participants