Drop assume-asserting logging macros #4286
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Placing optimization hints everywhere interferes with debugging of RelWithDebInfo builds, since the debugger won't be able to reliably inspect variables or control flow. These hints are better placed on an individual basis after identifying bottlenecks in a profiler.
neobrain
force-pushed
the
refactor_dont_assume
branch
from
775d29f to
da58e6a
Compare
|
(For context, this came up during review of #4271 (comment), where I noticed how widely these macros had been applied.) |
|
+1 to this, I've never liked this distinction and miss just regular old asserts. (I'd also like to rename this macro to something short and lowercase, probably |
Sonicadvance1
approved these changes
Jan 21, 2025
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replacing debug-only assertions with
__builtin_assumein optimizing builds seems like a free performance-win at first, but it turns out to be a footgun with no practical upside: The binary size reduction I measured is 0.03% (a factor of 0.0003).One big problem is that these hints interfere with debugging of non-assertion builds. Since the compiler generates code as-if the condition were true, a debugger will print wrong variable values or have erratic source stepping behavior. This can even apply to Debug builds, since FEX uses a dedicated
ASSERTIONS_ENABLEDmacro.Even debug printfs will print misleading information, which isn't obvious when your assumption is hidden in an innocent-looking macro that's barely distinguished from the non-hinting one.
As a future guideline, optimization hints should instead be placed intentionally after identifying bottlenecks in a profiler to communicate where hotspots are located and what invariants their efficient execution relies on.