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

[tools] Add always_optimize option for drake_cc_library #22380

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

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jan 2, 2025

Use it in our low-level OBB code.

Motivated by #22370. In that instance, this brings one test case out of a larger test program down from 11.0 seconds to 3.4 seconds of wall time.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added the release notes: none This pull request should not be mentioned in the release notes label Jan 2, 2025
@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri and +@sherm1 and +@xuchenhan-tri design feedback, please.

Sometimes in our Debug builds our higher-layer test programs are astonishingly slow. Here we see it with OBB but we've also see in with RotationMatrix and RigidTransform. I think for inner-loop, low-level libraries we should probably not use O0 by default, since it is such a penalty.

What do you think about this idea?

We could fiddle with the flags a little bit, maybe keep DRAKE_ASSERT turned on but eigen assertions turned off? Or keep it de-optimized during its local unit test, but optimized when used outside of the package?

Mostly looking for the kind of UX concepts we want here for our developers (and CI).

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

I like this. It may not solve all the things, but it's easy to at least experiment with.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: LGTM missing from assignees rpoyner-tri(platform),sherm1(platform),xuchenhan-tri(platform)

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri 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 in general. I like keeping the DRAKE_ASSERT on, but I’m not sure how much it eats into the gain? Another thing that’s worth discussing here is what’s our policy for allowing turning the flag on.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: LGTM missing from assignees rpoyner-tri(platform),sherm1(platform),xuchenhan-tri(platform)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

As written (-DNDEBUG) all the asserts are disarmed. I suspect that's necessary to achieve enough performance gain -- I'm thinking about the many super-expensive DRAKE_ASSERT_VOIDs we have at low levels that run eigenvalue checks and the like.

This option makes sense to me for CI where some Debug tests are excessively time consuming. In a Debug workflow during development though it would be nice to be able to conveniently turn it off. Could we have a --debug_everything build flag that would override?

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: LGTM missing from assignees rpoyner-tri(platform),sherm1(platform),xuchenhan-tri(platform)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants