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

Basic setup for benchmarks #74

Merged
merged 1 commit into from
Aug 3, 2022
Merged

Conversation

franziskuskiefer
Copy link
Member

This needs cleanup and better integration. It's only a basic setup to show how it is supposed to work.

#15 #36

@franziskuskiefer
Copy link
Member Author

@duesee can you review this so we can get it in?

Copy link
Contributor

@duesee duesee left a comment

Choose a reason for hiding this comment

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

Generally this looks good to me!

What I am unsure about: Similar to the case that we have to think about running build --instrument before test --instrument we now introduce colliding workflows. From my understanding, it can happen that a build --benchmarks run overrides the output of a build --tests run, which may lead to confusing errors. Thus, we need to be a bit careful what to run when.

Would it be difficult to output the benchmarks into a separate folder build/Debug/benchmark? If so, I still think it is okay for now as it is more important to get some numbers.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
mach Show resolved Hide resolved
Copy link
Member Author

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Yeah this is certainly not done yet. It's just adding the basic framework.
Let's get this in and address all the issues once the testing is done.

CMakeLists.txt Show resolved Hide resolved
mach Show resolved Hide resolved
This needs cleanup and better integration. It's only a basic setup to show how it is supposed to work.
@duesee duesee force-pushed the franziskus/benchmarks-setup branch from 1de7d3f to 94b470a Compare August 3, 2022 06:41
Copy link
Contributor

@duesee duesee left a comment

Choose a reason for hiding this comment

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

I've rebased on main. Let's get this in 👍

@franziskuskiefer franziskuskiefer enabled auto-merge (rebase) August 3, 2022 06:43
@franziskuskiefer
Copy link
Member Author

I've rebased on main. Let's get this in 👍

thanks, but please stop force pushing on other people's branches

@franziskuskiefer franziskuskiefer merged commit 67a4551 into main Aug 3, 2022
@franziskuskiefer franziskuskiefer deleted the franziskus/benchmarks-setup branch August 3, 2022 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants