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

Automate version comparison #260

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ikabadzhov
Copy link
Contributor

This is a small script to compare performance between 2 different patches/commits.
The script will take care of building 2 ROOT versions and running the desired benchmarks.
And finally https://github.com/google/benchmark/blob/main/tools/compare.py compares both.

@ikabadzhov ikabadzhov requested a review from eguiraud November 1, 2022 17:23
@ikabadzhov ikabadzhov self-assigned this Nov 1, 2022
Copy link
Member

@eguiraud eguiraud left a comment

Choose a reason for hiding this comment

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

Hi @ikabadzhov , it looks like you added too many files to this commit, including some under gbench? Also can we directly use the compare.py that comes with gbench rather than copying it into rootbench?

Copy link
Member

@eguiraud eguiraud left a comment

Choose a reason for hiding this comment

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

I am not sure how this script behaves if called multiple times in a row (possibly with different arguments)?

It would also be good to have it log what it's doing :)


Example usage:
```
sh rootbench_compare.sh -b latest-stable -c 9e7933c9bddba3f6381ba2b8c11539c46e8cc2f4 \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sh rootbench_compare.sh -b latest-stable -c 9e7933c9bddba3f6381ba2b8c11539c46e8cc2f4 \
bash rootbench_compare.sh -b latest-stable -c 9e7933c9bddba3f6381ba2b8c11539c46e8cc2f4 \

Comment on lines +10 to +16
2. "b" - branch name of the baseline (default is `master`)
3. "c" - commit hash of the baseline (if none provided, the latest commit is picked)
4. "R" - owner of the baseline (default is `root-project`)
5. "B" - branch name of the baseline (default is `master`)
6. "C" - commit hash of the baseline (if none provided, the latest commit is picked)
7. "t" - regex to be passed to rootbench ctest (default empty string, hence all tests)
8. "n" - cores to use (default empty string, hence all cores of the machine)
Copy link
Member

Choose a reason for hiding this comment

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

is it intentional that the description of b/B, c/C and r/R are the same?

Comment on lines +10 to +11
2. "b" - branch name of the baseline (default is `master`)
3. "c" - commit hash of the baseline (if none provided, the latest commit is picked)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't branch name and commit hash be mutually exclusive?

@@ -0,0 +1,91 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

git reset --hard ${commit_baseline}
fi
mkdir rbuild && cd rbuild
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_FLAGS="-O2 -g -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer" ..
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_FLAGS="-O2 -g -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer" ..
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_FLAGS="-fno-omit-frame-pointer -mno-omit-leaf-frame-pointer" ..

git reset --hard ${commit_compared}
fi
mkdir rbuild && cd rbuild
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_FLAGS="-O2 -g -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer" ..
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_FLAGS="-O2 -g -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer" ..
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_FLAGS="-fno-omit-frame-pointer -mno-omit-leaf-frame-pointer" ..


source baseline_root/rbuild/bin/thisroot.sh
mkdir baseline_bench && cd baseline_bench
cmake -Drootbench-datafiles=ON -Drootbench-out-format-json=ON ../../..
Copy link
Member

Choose a reason for hiding this comment

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

this assumes that the script is being executed from the root directory of the rootbench repo. we should probably check that this is the case at the very beginning of the script.

esac
done

git clone https://github.com/${root_owner_baseline}/root --branch ${branch_baseline} baseline_root
Copy link
Member

Choose a reason for hiding this comment

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

we should only git clone if the repository is not already present. also consider performing a sparse checkout (but I don't know how that interacts with executions of this script after the first)

@@ -0,0 +1,429 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

I thought compare.py comes with standard google benchmark. Is this compare.py different?

@vgvassilev
Copy link
Member

vgvassilev commented Nov 2, 2022

Thanks for working on this. That's really exciting. Could you add documentation/script which demonstrates how to use these tools to do git bisect based on a performance regression?

@eguiraud
Copy link
Member

eguiraud commented Nov 3, 2022

Hi Vassil,

Could you add documentation/script which demonstrates how to use these tools to do git bisect based on a performance regression?

This is not something you can do with this script as it is, might be something that can be built on top in a future PR

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.

3 participants