Skip to content

Adding benchmarks for marking files in the restore browser#883

Merged
arogge merged 6 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/restore-browser-marking-files-stress-test
Nov 3, 2021
Merged

Adding benchmarks for marking files in the restore browser#883
arogge merged 6 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/restore-browser-marking-files-stress-test

Conversation

@alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Jul 5, 2021

Description

This pull request introduces Benchmarks for developers to test and benchmark certain parts of the code when modifying performance or time critical code.
This is done with the help of the Google Benchmark tool.

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General
  • PR name is meaningful
  • Purpose of the PR is understood
  • Separate commit for this PR in the CHANGELOG.md, PR number referenced is same
  • Commit descriptions are understandable and well formatted
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR
  • bareos-check-sources --since-merge does not report any problems
  • git status should not report modifications in the source tree after building and testing

@arogge arogge self-assigned this Jul 22, 2021
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

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

The only to-be-discussed item is whether or not we want the benchmarks to run as tests. I believe it doesn't make sense. We should probably discuss this in one of the next devmeetings.

Other than that, this is awesome work. The CMake-Integration looks good, the code is clean and understandable, the benchmarks works flawlessly and proves that the algorithm scales nearly linearly, which is great.
I also really like the fantastic documentation you added.

btw. could you run devtools/bareos-check-sources --since=origin/master --modify on the branch, so the CMakeLists.txt is properly reformatted?

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/restore-browser-marking-files-stress-test branch 2 times, most recently from 07c0a5e to b2eec88 Compare October 6, 2021 11:03
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

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

I have made some suggestions concerning CMake.
If there is an option to enable building the benchmarks, we can and should probably fail if that is enabled and the required libraries are not available.
As far as I can tell, the tests are not added as ctest tests anymore, so there is no easy way (other than the manual way described in your documentation) to run them right now.

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/restore-browser-marking-files-stress-test branch 3 times, most recently from cdd999c to 2b21820 Compare October 14, 2021 14:40
@alaaeddineelamri alaaeddineelamri marked this pull request as ready for review October 21, 2021 09:27
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Still needs an entry in CHANGELOG.md.
bareos-check-sources complained about the CMakeLists.txt formatting, so I pushed a commit that fixes it - you may want to merge that one into one of your commits.

Alaa Eddine Elamri added 6 commits November 3, 2021 11:09
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/restore-browser-marking-files-stress-test branch from ec3c925 to 82cfc8d Compare November 3, 2021 10:50
@arogge arogge merged commit 2237de9 into bareos:master Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants