MDEV-34848 Remove overhead from unnecessary MDL_context_owner inheritance by kevdn · Pull Request #4808 · MariaDB/server

kevdn · 2026-03-15T09:04:47Z

Optimize MDL by using THD* directly instead of using abstract MDL_context_owner* interface, use THD* directly to avoid virtual function call overhead in MDL wait loops.

The MDL subsystem only interacts with THD objects, so using THD* directly eliminates unnecessary virtual function calls for enter_cond, exit_cond, is_killed, and notify_shared_lock.

Instead of using abstract MDL_context_owner* interface, use THD* directly to avoid virtual function call overhead in MDL wait loops. The MDL subsystem only interacts with THD objects, so using THD* directly eliminates unnecessary virtual function calls for enter_cond, exit_cond, is_killed, and notify_shared_lock.

CLAassistant · 2026-03-15T09:04:55Z

All committers have signed the CLA.

Copilot

Pull request overview

This PR refactors parts of the MDL/THD interaction by switching several APIs from the MDL_context_owner* abstraction to direct THD* usage.

Changes:

Updated notify_shared_lock() to take THD* instead of MDL_context_owner* and adjusted call sites/implementation accordingly.
Updated MDL_wait::timed_wait() and MDL_context ownership plumbing to use THD* directly.
Simplified a few get_thd()-mediated field accesses (e.g., rgi_slave, thd_is_connected).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
sql/sql_class.h	Changes `THD::notify_shared_lock()` signature to accept `THD*`.
sql/sql_class.cc	Adjusts `THD::notify_shared_lock()` implementation for new parameter type.
sql/mdl.h	Updates `MDL_context_owner::notify_shared_lock()`, `MDL_wait::timed_wait()`, and `MDL_context` owner storage/accessors to use `THD*`.
sql/mdl.cc	Updates `MDL_wait::timed_wait()` implementation and related THD accesses for the new `THD*` owner type.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

sql/mdl.h

sql/sql_class.h

sql/mdl.cc

sql/sql_class.cc

FooBarrior

I would actually prefer the opposite: architecturally, we should focus on decomposing thd, because it's bloated.

I understand though, that we might want to make de-architecturizing optimizations, if it's worth it.

If this is supposed to be a such, is there any benchmark demonstrating the commitment?

- Update documentation comments to reflect THD* usage - Remove redundant ctx_in_use alias in notify_shared_lock() - Add note about backward compatibility of MDL_context_owner interface Co-Authored-By: kevdn <lehuukhoa12@gmail.com>

vaintroub · 2026-03-16T09:07:56Z

Agree with @FooBarrior . For the unmeasurable performance benefit of removing virtual function we'll plant this THD dependency everywhere. We should actually de-THD as much as we can, it is too important class, which purpose nobody can explain well.

gkodinov

Thank you for working on this! This is a preliminary review.

First, on the technicalities: Please squash your two commits into a single one and provide a commit message to it that compiles with CODING_STANDARDS.md.

Secondly, I kind of agree with the two developers that have commented that you need a strong case to remove layering like you did. A benchmark or profile info would help. And even in that case, there probably are alternatives to removing a base class for THD.

svoj · 2026-03-16T12:16:48Z

MDL_context_owner was implemented by mysql/mysql-server@b9766fe68bab, MySQL Bug #59309 with the purpose of "doing standalone unit testing of the MDL module". MariaDB doesn't adapt neither gunit test framework nor mdl tests specifically.

enter_cond(), exit_cond(), is_killed() are not owned by the MDL subsystem. notify_shared_lock() is used by MDL exclusively, but still it doesn't relate to MDL. All of them were historically part of THD. Having them encapsulated as MDL_context_owner is misleading.

Said the above, I see mysql/mysql-server@b9766fe68bab as a step towards a wrong direction and I'm happy to see it reverted.

I won't blindly buy the "performance benefit of removing virtual function". But still I see it as a valid encapsulation cleanup and it must make THD class 48(?) bytes smaller, which you guys are after.

svoj

Looks, nice. A few requests inline.

sql/mdl.h

sql/sql_class.cc

sql/sql_class.h

gkodinov · 2026-03-17T09:34:56Z

MDL_context_owner was implemented by mysql/mysql-server@b9766fe68bab, MySQL Bug #59309 with the purpose of "doing standalone unit testing of the MDL module". MariaDB doesn't adapt neither gunit test framework nor mdl tests specifically.

enter_cond(), exit_cond(), is_killed() are not owned by the MDL subsystem. notify_shared_lock() is used by MDL exclusively, but still it doesn't relate to MDL. All of them were historically part of THD. Having them encapsulated as MDL_context_owner is misleading.

Said the above, I see mysql/mysql-server@b9766fe68bab as a step towards a wrong direction and I'm happy to see it reverted.

I won't blindly buy the "performance benefit of removing virtual function". But still I see it as a valid encapsulation cleanup and it must make THD class 48(?) bytes smaller, which you guys are after.

Svoj,

We either want MDL_context_owner or we don't. I will not participate in that debate. It's an architecture decision IMHO.

If we want it, the whole diff should be about avoiding virtual functions and it being useful only as a THD base class.

If we don't however, why not remove it altogether?

In both cases, I believe the PR needs work.

svoj · 2026-03-17T09:39:58Z

MDL_context_owner was implemented by mysql/mysql-server@b9766fe68bab, MySQL Bug #59309 with the purpose of "doing standalone unit testing of the MDL module". MariaDB doesn't adapt neither gunit test framework nor mdl tests specifically.
enter_cond(), exit_cond(), is_killed() are not owned by the MDL subsystem. notify_shared_lock() is used by MDL exclusively, but still it doesn't relate to MDL. All of them were historically part of THD. Having them encapsulated as MDL_context_owner is misleading.
Said the above, I see mysql/mysql-server@b9766fe68bab as a step towards a wrong direction and I'm happy to see it reverted.
I won't blindly buy the "performance benefit of removing virtual function". But still I see it as a valid encapsulation cleanup and it must make THD class 48(?) bytes smaller, which you guys are after.

Svoj,

We either want MDL_context_owner or we don't. I will not participate in that debate. It's an architecture decision IMHO.

If we want it, the whole diff should be about avoiding virtual functions and it being useful only as a THD base class.

If we don't however, why not remove it altogether?

In both cases, I believe the PR needs work.

@gkodinov we don't need MDL_context_owner. Removing it altogether is what I suggested in my review: #4808 (comment). Yes, PR needs work.

- Remove MDL_context_owner class from mdl.h - Remove inheritance from MDL_context_owner in THD class - Remove override keywords from THD methods - Rename ctx_in_use parameter to in_use in notify_shared_lock()

kevdn · 2026-03-18T17:29:13Z

Hi @svoj , I have resolved your comments.

BTW, @svoj @gkodinov @vaintroub @FooBarrior , I ran sysbench to compare the original vs optimized versions.

Benchmark Results

Environment

macOS (Apple Silicon)
32 threads
10 tables × 10,000 rows
oltp_read_write.lua workload

Command

sysbench /opt/homebrew/share/sysbench/oltp_read_write.lua \
  --mysql-host=127.0.0.1 --mysql-port=3309 --mysql-user=root \
  --mysql-db=sbtest --tables=10 --table-size=10000 \
  --threads=32 --time=60 run

Original

Transactions: 3,394/sec  
Avg latency: 9.42 ms

Optimized (after removing `MDL_context_owner`)

Transactions: 3,032/sec  
Avg latency: 10.55 ms

Summary

The results are broadly similar, with differences (~5–10%) falling within expected run-to-run variance for this workload. There is no clear indication of a performance improvement or regression from this change.

Copilot

Pull request overview

This PR removes the MDL_context_owner abstraction and updates the metadata locking (MDL) subsystem to use THD* directly as the MDL context owner, simplifying the ownership model and related call paths.

Changes:

Removed the MDL_context_owner interface and THD inheritance from it.
Updated MDL APIs and internal state to store/pass THD* owners (MDL_context, MDL_wait::timed_wait, THD::notify_shared_lock).
Adjusted MDL wait/notify logic and comments to reflect the new THD*-based ownership.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
sql/sql_class.h	Drops `MDL_context_owner` inheritance; updates MDL-related methods/signatures on `THD`.
sql/sql_class.cc	Updates `THD::notify_shared_lock` implementation to take `THD*` directly.
sql/mdl.h	Removes `MDL_context_owner`; changes MDL owner storage and public APIs to use `THD*`.
sql/mdl.cc	Updates MDL wait/notify and owner access to use `THD*` (including WSREP/debug sync call sites).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

vaintroub · 2026-03-18T17:38:52Z

Summary

The results are broadly similar, with differences (~5–10%) falling within expected run-to-run variance for this workload. There is no clear indication of a performance improvement or regression from this change.

I don't think one can produce anything meaningful with read-write benchmark.
You can try though, by reducing overhead. oltp_point_select, cached is a benchmark that does not write, and should quickly complete. Also, do not use TCP, use --socket, it is way cheaper. Still, there would not be anything measurable, from such change. how much can it save, 100 instructions per query, that ballpark.

vaintroub · 2026-03-18T17:39:38Z

Summary

The results are broadly similar, with differences (~5–10%) falling within expected run-to-run variance for this workload. There is no clear indication of a performance improvement or regression from this change.

I don't think one can produce anything that can be measured, with read-write benchmark. You can try though, by reducing overhead. say not read_write, but oltp_point_select, cached(in memory) is a benchmark that does not write, and should produce much higher TPS. Also, do not use TCP, use --socket, it is way cheaper. Still, there would not be anything measurable, from such change. how much can it save, 100 instructions per query, that ballpark.

kevdn · 2026-03-18T18:08:17Z

Summary

The results are broadly similar, with differences (~5–10%) falling within expected run-to-run variance for this workload. There is no clear indication of a performance improvement or regression from this change.

I don't think one can produce anything meaningful with read-write benchmark. You can try though, by reducing overhead. oltp_point_select, cached is a benchmark that does not write, and should quickly complete. Also, do not use TCP, use --socket, it is way cheaper. Still, there would not be anything measurable, from such change. how much can it save, 100 instructions per query, that ballpark.

Ok, I will try another benchmark

svoj

@kevdn thanks for updating your patch. We're almost there. Please squash commits into one and update commit message, then force push to your mdev-34848-mdl-optimize branch.

I suggest that commit message:

gives some background about MDL_context_owner and why it was wrong, feel free to copy first two paragraphs of #4808 (comment)
advertises the patch as a cleanup rather than optimization

Benchmarking is optional: me and @vaintroub are not expecting any meaningful performance impact from this change.

svoj · 2026-03-18T21:01:01Z

sql/sql_class.h

+                          bool needs_non_slave_abort);

-  // End implementation of MDL_context_owner interface.
+  // End of MDL-related methods.


No, these methods don't relate to MDL. They rather form some sort of session state interface, not MDL. Please remove this line, it is misleading.

svoj · 2026-03-18T21:07:26Z

sql/mdl.h

@@ -59,67 +59,6 @@ typedef unsigned short mdl_bitmap_t;
 */
 #define EXIT_COND(S) exit_cond(S, __func__, __FILE__, __LINE__)


It'd be nice if you could also move ENTER_COND() and EXIT_COND() macros to sql_class.h, somewhere around THD_ENTER_COND() and THD_EXIT_COND().

svoj · 2026-03-18T21:19:45Z

sql/mdl.h

  void rollback_to_savepoint(const MDL_savepoint &mdl_savepoint);

-  MDL_context_owner *get_owner() { return m_owner; }
+  THD *get_owner() { return m_owner; }


This is essentially equivalent to MDL_context::get_thd(). Please remove this method and adjust callers to use get_thd().

Copilot AI review requested due to automatic review settings March 15, 2026 09:04

Copilot started reviewing on behalf of kevdn March 15, 2026 09:05 View session

Copilot AI reviewed Mar 15, 2026

View reviewed changes

sql/mdl.h Outdated Show resolved Hide resolved

sql/sql_class.h Outdated Show resolved Hide resolved

sql/mdl.cc Show resolved Hide resolved

sql/sql_class.cc Outdated Show resolved Hide resolved

FooBarrior reviewed Mar 16, 2026

View reviewed changes

MDEV-34848 Optimize MDL by using THD* directly

50eca41

- Update documentation comments to reflect THD* usage - Remove redundant ctx_in_use alias in notify_shared_lock() - Add note about backward compatibility of MDL_context_owner interface Co-Authored-By: kevdn <lehuukhoa12@gmail.com>

gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Mar 16, 2026

gkodinov requested changes Mar 16, 2026

View reviewed changes

svoj requested changes Mar 16, 2026

View reviewed changes

sql/mdl.h Outdated Show resolved Hide resolved

sql/sql_class.cc Outdated Show resolved Hide resolved

sql/sql_class.h Outdated Show resolved Hide resolved

MDEV-34848 Remove MDL_context_owner abstraction entirely

eef475f

- Remove MDL_context_owner class from mdl.h - Remove inheritance from MDL_context_owner in THD class - Remove override keywords from THD methods - Rename ctx_in_use parameter to in_use in notify_shared_lock()

kevdn requested review from FooBarrior, Copilot, gkodinov and svoj March 18, 2026 17:30

Copilot started reviewing on behalf of kevdn March 18, 2026 17:31 View session

Copilot AI reviewed Mar 18, 2026

View reviewed changes

kevdn closed this Mar 18, 2026

kevdn reopened this Mar 18, 2026

svoj requested changes Mar 18, 2026

View reviewed changes

		@@ -59,67 +59,6 @@ typedef unsigned short mdl_bitmap_t;
		*/
		#define EXIT_COND(S) exit_cond(S, __func__, __FILE__, __LINE__)

Uh oh!

Conversation

kevdn commented Mar 15, 2026

Uh oh!

CLAassistant commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FooBarrior left a comment

Choose a reason for hiding this comment

Uh oh!

vaintroub commented Mar 16, 2026

Uh oh!

gkodinov left a comment

Choose a reason for hiding this comment

Uh oh!

svoj commented Mar 16, 2026

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gkodinov commented Mar 17, 2026

Uh oh!

svoj commented Mar 17, 2026

Uh oh!

kevdn commented Mar 18, 2026

Benchmark Results

Original

Optimized (after removing MDL_context_owner)

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

vaintroub commented Mar 18, 2026

Summary

Uh oh!

vaintroub commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

kevdn commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

svoj Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

svoj Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

svoj Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

CLAassistant commented Mar 15, 2026 •

edited

Loading

Optimized (after removing `MDL_context_owner`)

vaintroub commented Mar 18, 2026 •

edited

Loading

kevdn commented Mar 18, 2026 •

edited

Loading