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

Update mini-benchmark to use constant transactions #3514

Merged
merged 1 commit into from
Oct 6, 2024

Conversation

robinnewhouse
Copy link
Contributor

Description

Mini-benchmark was using total time instead of total transactions
(events) to measure performance. This could give inconsistent results,
particulary when used to count CPU cycles. This changes the sysbench
command to use events x threads in each benchmark job, making the
benchmark more informative.

When perf [1] is available and enabled, mini-benchmark will measure how
many CPU cycles are required to do a fixed amount of work/queries. If
perf is unavailable, mini-benchmark will measure the peak queries per
second (QPS) during the execution of this work, regardless of the
duration.

[1] https://perf.wiki.kernel.org

Release Notes

No release notes necessary.

How can this PR be tested?

Testing is done in GitLab CI. A passing example can be seen at https://salsa.debian.org/robinnewhouse/mariadb-server-mirror/-/pipelines.

Basing the PR against the correct MariaDB version

  • 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.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@LinuxJedi
Copy link
Contributor

Hi @robinnewhouse,

In general this looks good, I'm not sure about 10.11 for it. This is because it is a behaviour change and that script is installed as part of packages.

@robinnewhouse
Copy link
Contributor Author

Hi @robinnewhouse,

In general this looks good, I'm not sure about 10.11 for it. This is because it is a behaviour change and that script is installed as part of packages.

True, but I would call this undesirable behaviour since it gives confusing results as is. But true, it is a change in behaviour so I'm happy to rebase to main.

@robinnewhouse robinnewhouse changed the base branch from 10.11 to main September 20, 2024 20:39
@ottok ottok self-requested a review October 2, 2024 18:43
Copy link
Contributor

@ottok ottok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ottok
Copy link
Contributor

ottok commented Oct 2, 2024

Can you please rebase on latest 'main' so we can see if CI passed and this can be merged?

@robinnewhouse
Copy link
Contributor Author

Can you please rebase on latest 'main' so we can see if CI passed and this can be merged?

Thanks for approving Otto. This is currently based on the latest main. The last commit before this on main was 8478a06 on Sep 19, 2024.

Mini-benchmark was using total time instead of total transactions
(events) to measure performance. This could give inconsistent results,
particulary when used to count CPU cycles. This changes the sysbench
command to use events x threads in each benchmark job, making the
benchmark more informative.

When `perf` [1] is available and enabled, mini-benchmark will measure how
many CPU cycles are required to do a fixed amount of work/queries. If
`perf` is unavailable, mini-benchmark will measure the peak queries per
second (QPS) during the execution of this work, regardless of the
duration.

[1] https://perf.wiki.kernel.org
@ottok ottok merged commit 4016c90 into MariaDB:main Oct 6, 2024
14 of 15 checks passed
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.

4 participants