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

[RFC][libc++][benchmark] Enable benchmark optimizations. #132445

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mordante
Copy link
Member

Before we moved our benchmark suite from a stand-alone suite to the current lit based suite the default was to run benchmarks optimized. With the change the default is to run not optimized. I've been bitten several times by this issue and need to spend time to look how to run these test optimized.

I feel this is a regression and also makes it harder for other people to do the right thing. I feel the default should be to do the right thing. I agree this somewhat differs from the idea that the test suite should be run with the same flags. Still I feel having the right default makes it easier to onboard in the project and not to remember these oddities.

This patch adds a new default optimization (bikeshedding is welcome) that keeps the main tests as if specified optimization=none and the benchmarks as if specified optimization=speed.
The other values keep their exisiting behaviour. (When the test are optimized for speed or size the benchmarks will inherit this flag without any changes.) The benchmarks can still be not opimized by using optimization=none.

This patch will need more cleanups and documentation.

Before we moved our benchmark suite from a stand-alone suite to the
current lit based suite the default was to run benchmarks optimized.
With the change the default is to run not optimized. I've been bitten
several times by this issue and need to spend time to look how to run
these test optimized.

I feel this is a regression and also makes it harder for other people to
do the right thing. I feel the default should be to do the right thing.
I agree this somewhat differs from the idea that the test suite should
be run with the same flags. Still I feel having the right default makes
it easier to onboard in the project and not to remember these oddities.

This patch adds a new default optimization (bikeshedding is welcome)
that keeps the main tests as if specified optimization=none and the
benchmarks as if specified optimization=speed.
The other values keep their exisiting behaviour. (When the test are
optimized for speed or size the benchmarks will inherit this flag
without any changes.) The benchmarks can still be not opimized by using
optimization=none.

This patch will need more cleanups and documentation.
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

I feel like this is clearly documented in https://libcxx.llvm.org/TestingLibcxx.html#benchmarks, but I guess making the default closer to what we usually want is not a bad thing. Overall I think this patch makes sense, please ping me again when it's ready for a final review (it seems like not much is missing).

type=str,
help="The optimization level to use when compiling the test suite.",
default="none",
default="default", # defaults, test -> none, benchmarks -> speed
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
default="default", # defaults, test -> none, benchmarks -> speed
help="The optimization level to use when compiling the test suite. 'default' is to enable no optimizations for tests and to optimize benchmarks for speed.",
default="default", # defaults, test -> none, benchmarks -> speed

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