MDEV-35740 - Make IO_SIZE compile-time configurable, convert comment to compile-time check, fix grammar #3726

ericherman · 2024-12-31T12:14:21Z

The Jira issue number for this PR is: MDEV-35740

There is no JIRA for this simple refactoring.

Description

Making mariadb's IO_SIZE compile-time configurable enables more straight-forward investigation of the performance implications of having an IO_SIZE which is different than the memory page size.

By converting the text describing the constraints of the constant to a compile-time check, this enables us to make IO_SIZE configurable.

It should be noted that this #define is duplicated in the submodule libmariadb in the include/ma_global.h file.

Release Notes

Release notes might include the ability to define IO_SIZE to be a value other than 4096.

How can this PR be tested?

First, ensure that libmariadb is patched to #ifndef guard the #define IO_SIZE ( see: mariadb-corporation/mariadb-connector-c#265 ).

Next, add different values via cmake like -DIO_SIZE=8192 to see a larger value, or like -DIO_SIZE=4000 to see it fail with a compile-time error.

As this is a compile-time option this does not lend itself to automated testing.

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.

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.

ericherman · 2024-12-31T12:20:36Z

This #define is duplicated in the submodule libmariadb in the include/ma_global.h file.

How are changes in mariadb-server's includes propagated to libmariadb's includes?

Is there an existing process?

Should I open a parallel PR for that?

vuvova · 2024-12-31T13:01:11Z

libmariadb needs a special PR there's no automatic propagation

If the server is to make IO_SIZE configurable, we need to avoid either redefining it, or defining it to be different. See: MariaDB/server#3726 Signed-off-by: Eric Herman <[email protected]>

ericherman · 2024-12-31T17:15:28Z

libmariadb needs a special PR there's no automatic propagation

I've created a draft PR: mariadb-corporation/mariadb-connector-c#265

I think I will create an MDEV for this work, as it crosses repository boundaries.

include/my_global.h

If the server is to make IO_SIZE configurable, we need to avoid either redefining it, or defining it to be different. See: MariaDB/server#3726 Signed-off-by: Eric Herman <[email protected]>

The server's definition of IO_SIZE is re-used here in the client for network buffer alignment, however IO_SIZE is used in the server for many different things and the client's buffer alignment is not related to many of those uses. If the server is to make IO_SIZE configurable, we need to avoid either redefining it, or defining it to be different. By creating a specific define for this, we avoid redfine and clarify the code. See: MariaDB/server#3726 Signed-off-by: Eric Herman <[email protected]>

cvicentiu · 2025-02-23T17:33:27Z

@ericherman Is this PR still a draft? How would you like to proceed with this code change given the input you've received thus far?

My thoughts on the code change:
While It would seem that this should be a safe change, I wonder if there aren't any hidden gotchas where a server compiled with a particular IO_SIZE can not actually read the data directory from a server compiled with a different IO_SIZE. I don't have any particular good ideas on answering this other than investigating all code that uses it.

The server's definition of IO_SIZE is re-used here in the client for network buffer alignment, however IO_SIZE is used in the server for many different things and the client's buffer alignment is not related to many of those uses. If the server is to make IO_SIZE configurable, we need to avoid either redefining it, or defining it to be different. By creating a specific define for this, we avoid redfine and clarify the code. See: MariaDB/server#3726 Signed-off-by: Eric Herman <[email protected]>

ericherman · 2025-05-21T09:38:48Z

This is still draft because mariadb-corporation/mariadb-connector-c#267 should be merged first.

The server's definition of IO_SIZE is re-used here in the client for network buffer alignment, however IO_SIZE is used in the server for many different things and the client's buffer alignment is not related to many of those uses. If the server is to make IO_SIZE configurable, we need to avoid either redefining it, or defining it to be different. By creating a specific define for this, we avoid redfine and clarify the code. See: MariaDB/server#3726 Signed-off-by: Eric Herman <[email protected]>

svoj · 2025-08-08T13:31:01Z

@ericherman now that mariadb-corporation/mariadb-connector-c#267 got merged, should we proceed?

ericherman · 2025-08-19T11:13:25Z

rebased and ready for review

gkodinov

Sorry for the delayed review, Eric! I hope you're still willing to see this through.

This is my preliminary review of the change. Once we clear this out I'll solicit a final reviewer.

gkodinov · 2026-01-27T10:43:22Z

CMakeLists.txt


+SET(IO_SIZE "" CACHE STRING "Specify the I/O buffer size")
+IF(IO_SIZE)
+  ADD_DEFINITIONS(-DIO_SIZE=${IO_SIZE})


I'd avoid adding a global definition to the command line. I'd add a

#cmakedefine IO_SIZE @IO_SIZE@

to config.h.cmake. It's arelady included by my_global.h.

I'm not sure I understand why you would avoid this. While perhaps unlikely to be used by people other than those doing performance comparisons, a command-line option makes it easy to build with other values than the default, yes?

gkodinov · 2026-01-27T10:45:03Z

CMakeLists.txt

  SET (SKIP_COMPONENTS "N-O-N-E")
 ENDIF()

+SET(IO_SIZE "" CACHE STRING "Specify the I/O buffer size")


I'd put the default here and move this to configure.cmake.

gkodinov · 2026-01-27T10:45:46Z

include/my_global.h

+  This influences the speed of the isam btree library. E.g.: too big too slow.
  4096 is a common block size on SSDs.
 */
+#ifndef IO_SIZE


I'd remove the while @ifndef block and make sure the variable is always defined by cmake.

gkodinov · 2026-01-27T10:47:30Z

include/my_global.h

+#ifndef IO_SIZE
 #define IO_SIZE			4096U
+#endif
+#if (IO_SIZE < 512) || (IO_SIZE & (IO_SIZE-1))


I'd move this check into configure.cmake. Here's it's too late to check and kind of a gotcha.

ericherman · 2026-01-28T10:51:30Z

Thank you for taking a look at this. I am not likely to have a nice chance to look at this in the next week.

By converting the text describing the constraints of the constant to a compile-time check, this enables us to make IO_SIZE configurable. It should be noted that this #define is duplicated in the submodule libmariadb in the include/ma_global.h file. Signed-off-by: Eric Herman <[email protected]>

Making mariadb's IO_SIZE compile-time configurable enables more straight-forward investigation of the performance implications of having an IO_SIZE which is different than the memory page size. The default IO_SIZE of 4096 as defined in include/my_global.h matches the memory page size of most systems. Larger page sizes are widely supported, called "huge pages" in Linux, "superpages" in FreeBSD, and "large pages" in MS Windows. On POSIX systems, obtaining the page size can be done via: page_size= sysconf(_SC_PAGESIZE); On Windows: SYSTEM_INFO si; GetSystemInfo(&si); page_size= si.dwPageSize; In https://jira.mariadb.org/browse/MDEV-35740 Marko highlights that there are vastly different uses of IO_SIZE. This "one size fits all" nature of IO_SIZE is not ideal, future work could split this into separate constants based upon usage. See also: mariadb-corporation/mariadb-connector-c#267 Signed-off-by: Eric Herman <[email protected]>

ericherman · 2026-02-04T13:11:34Z

The latest push moves the check to CMake
(and rebased)

ericherman force-pushed the eherman-io-size-20241231 branch from e0a9a93 to 16967b9 Compare December 31, 2024 14:33

ericherman mentioned this pull request Dec 31, 2024

Avoid redfine of IO_SIZE if made configurable mariadb-corporation/mariadb-connector-c#265

Closed

ericherman marked this pull request as draft December 31, 2024 17:15

ericherman changed the title ~~Convert comment to compile-time check, fix grammar~~ Make IO_SIZE configurable, convert comment to compile-time check, fix grammar Dec 31, 2024

dr-m reviewed Jan 2, 2025

View reviewed changes

include/my_global.h Outdated Show resolved Hide resolved

cvicentiu added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Jan 3, 2025

ericherman changed the title ~~Make IO_SIZE configurable, convert comment to compile-time check, fix grammar~~ Make IO_SIZE compile-time configurable, convert comment to compile-time check, fix grammar Jan 3, 2025

ericherman force-pushed the eherman-io-size-20241231 branch 2 times, most recently from 494fc45 to 780b924 Compare January 3, 2025 12:02

ericherman force-pushed the eherman-io-size-20241231 branch from 780b924 to 9879b3d Compare January 3, 2025 12:57

ericherman mentioned this pull request Jan 5, 2025

Create #define for network buffer alignment mariadb-corporation/mariadb-connector-c#267

Merged

svoj changed the title ~~Make IO_SIZE compile-time configurable, convert comment to compile-time check, fix grammar~~ MDEV-35740 - Make IO_SIZE compile-time configurable, convert comment to compile-time check, fix grammar Feb 1, 2025

ericherman force-pushed the eherman-io-size-20241231 branch from 26bdd78 to c20c46e Compare May 21, 2025 09:47

ericherman force-pushed the eherman-io-size-20241231 branch from c20c46e to 9d8fa62 Compare August 14, 2025 15:27

ericherman marked this pull request as ready for review August 14, 2025 15:29

gkodinov requested changes Jan 27, 2026

View reviewed changes

ericherman force-pushed the eherman-io-size-20241231 branch from 1f1e68f to b9536fa Compare February 4, 2026 12:16

ericherman force-pushed the eherman-io-size-20241231 branch 3 times, most recently from 03d3d00 to 5a19920 Compare February 4, 2026 13:07

ericherman force-pushed the eherman-io-size-20241231 branch from 5a19920 to 101f8f9 Compare February 4, 2026 13:10

ericherman requested a review from gkodinov February 4, 2026 13:14

Uh oh!

MDEV-35740 - Make IO_SIZE compile-time configurable, convert comment to compile-time check, fix grammar #3726

Are you sure you want to change the base?

MDEV-35740 - Make IO_SIZE compile-time configurable, convert comment to compile-time check, fix grammar #3726

Conversation

ericherman commented Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

Uh oh!

ericherman commented Dec 31, 2024

Uh oh!

vuvova commented Dec 31, 2024

Uh oh!

ericherman commented Dec 31, 2024

Uh oh!

Uh oh!

cvicentiu commented Feb 23, 2025

Uh oh!

ericherman commented May 21, 2025

Uh oh!

svoj commented Aug 8, 2025

Uh oh!

ericherman commented Aug 19, 2025

Uh oh!

gkodinov left a comment

Choose a reason for hiding this comment

Uh oh!

gkodinov Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

ericherman Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

gkodinov Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

gkodinov Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

gkodinov Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

ericherman commented Jan 28, 2026

Uh oh!

ericherman commented Feb 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

ericherman commented Dec 31, 2024 •

edited

Loading