Migrate UNODB_DETAIL_LIKELY/UNODB_DETAIL_UNLIKELY to C++20 [[likely]]/[[unlikely]]#653
Migrate UNODB_DETAIL_LIKELY/UNODB_DETAIL_UNLIKELY to C++20 [[likely]]/[[unlikely]]#653laurynas-biveinis wants to merge 1 commit intomasterfrom
Conversation
WalkthroughThis pull request introduces a comprehensive update across multiple files to replace macro-based branch prediction hints with C++20 attributes Changes
Sequence DiagramsequenceDiagram
participant Code as Original Code
participant Compiler as Modern Compiler
participant Attr as C++20 Attributes
Code->>Compiler: Use macro-based hints
Compiler->>Attr: Replace with standard attributes
Attr-->>Compiler: Provide branch prediction hints
Compiler->>Code: Optimize code execution
Possibly related PRs
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
…/[[unlikely]] Leave the macros around for the cases the C++20 statement attributes don't cover (i.e. subexpressions, return statement).
laurynas-biveinis
force-pushed
the
c++20-likely
branch
from
d186a28 to
539ee4b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
olc_art.cpp (1)
Line range hint
666-690: Consistent error handling in synchronization paths.The migration maintains consistent error handling with [[unlikely]] attributes on all synchronization checks. This is correct as these represent error paths that should rarely occur in a well-functioning system.
However, consider:
- Adding debug logging in these paths to help troubleshoot when they do occur
- Documenting the expected frequency of these conditions
- Adding metrics to track how often these paths are taken
Also applies to: 702-721, 729-741
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
CPPLINT.cfg(1 hunks)art.cpp(10 hunks)art.hpp(4 hunks)art_internal_impl.hpp(1 hunks)global.hpp(3 hunks)heap.hpp(2 hunks)olc_art.cpp(47 hunks)olc_art.hpp(4 hunks)optimistic_lock.hpp(5 hunks)qsbr.cpp(9 hunks)qsbr.hpp(3 hunks)test/qsbr_test_utils.cpp(2 hunks)test_heap.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (106)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with ASan, Release, ON, clang, 12)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with ASan, Release, ON, clang, 12)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with ASan, Release, ON, clang, 12)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (GCC 10 Debug, Debug, gcc, 10)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with ASan, Release, ON, clang, 12)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (GCC 10 Debug, Debug, gcc, 10)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with ASan, Release, ON, clang, 12)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (GCC 10 Debug, Debug, gcc, 10)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with ASan, Release, ON, clang, 12)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (GCC 10 Debug, Debug, gcc, 10)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with ASan, Release, ON, clang, 12)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (GCC 10 Debug, Debug, gcc, 10)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with ASan, Release, ON, clang, 12)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (GCC 10 Debug, Debug, gcc, 10)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with ASan, Release, ON, clang, 12)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (GCC 10 Debug, Debug, gcc, 10)
- GitHub Check: build (clang 19 Debug static analysis, ubuntu-22.04, Debug, clang, ON)
- GitHub Check: build (clang 19 Debug, ubuntu-22.04, Debug, clang)
- GitHub Check: build (clang 12 Release with UBSan, Release, ON, clang, 12)
- GitHub Check: build (clang 12 Release with TSan, Release, ON, clang, 12)
🔇 Additional comments (21)
CPPLINT.cfg (1)
10-11: LGTM! Configuration updated to handle C++20 attributes.The added filter prevents false positives from cpplint when using [[likely]]/[[unlikely]] attributes, and the comment clearly explains the purpose.
heap.hpp (1)
48-49: LGTM! Correctly migrated to C++20 attributes.The error handling paths are properly marked as [[unlikely]], maintaining the original optimization hints while improving readability. The LCOV exclusion markers are correctly preserved.
Also applies to: 61-62
test_heap.cpp (1)
32-33: LGTM! Successfully migrated memory allocation check.The success path is correctly marked as [[likely]], which is appropriate for memory allocation operations where success is the expected outcome.
qsbr.cpp (3)
155-157: LGTM! Compare-exchange operations correctly marked.All compare-exchange success paths are consistently marked as [[likely]], which is the expected pattern for optimistic concurrency control.
Also applies to: 218-220, 236-238, 273-275, 314-316, 478-480
214-214: LGTM! Thread state transitions properly annotated.Thread registration, unregistration, and epoch change conditions are appropriately marked with [[likely]]/[[unlikely]] based on their expected frequency.
Also applies to: 267-267, 301-301, 320-320, 330-330
448-453: LGTM! Single-thread mode handling properly marked.The non-single-thread mode path is correctly marked as [[likely]], with appropriate handling of the orphaned requests condition.
global.hpp (1)
105-106: LGTM! Clear documentation of macro usage.The added comments effectively document when to use the macros (subexpressions, return statements, ternary operators) vs C++20 attributes, which will help prevent misuse.
Also applies to: 120-121
art.hpp (1)
576-577: LGTM! Correct migration to C++20 attributes.The macro replacements follow the proper C++20 attribute syntax, maintaining the same branch prediction hints while improving readability. The attribute placement is correct - after the condition but before the statement.
Also applies to: 585-586, 601-602, 636-637
olc_art.hpp (1)
489-490: LGTM! Consistent migration pattern.The changes follow the same correct pattern as in art.hpp, ensuring consistency across the codebase. The attribute placement and usage are correct.
Also applies to: 498-499, 526-527, 535-536, 581-582, 599-600
qsbr.hpp (1)
16-21: LGTM! Additional IWYU pragma directives.The added IWYU pragma directives help control include behavior, though unrelated to the branch prediction migration.
olc_art.cpp (1)
604-604: Verify branch prediction hints in critical path.While the migration syntax is correct, please verify that these branch prediction hints align with the actual behavior in production:
- Line 604: Is the null check truly likely to be false?
- Lines 631-643: Are these error conditions truly unlikely in practice?
Consider adding comments explaining the rationale for these hints to help future maintainers.
Also applies to: 631-643
✅ Verification successful
Branch prediction hints are correctly placed
The hints align with the actual behavior:
cached_leaf == nullptris correctly marked[[likely]]as leaves are immediately moved into the tree after creation- Error conditions are correctly marked
[[unlikely]]as they represent exceptional cases in the optimistic locking mechanism🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the frequency of these conditions in production logs # Note: This is a template as actual logs may not be available in the sandbox # Check for null leaf creation frequency rg "created new leaf" /var/log/app.log | wc -l # Check for error conditions frequency rg "must_restart|capacity reached" /var/log/app.log | wc -lLength of output: 228
Script:
#!/bin/bash # Find create_leaf_if_needed implementation and context ast-grep --pattern 'create_leaf_if_needed($$$) { $$$ }' # Look for related error handling and comments rg -B 2 -A 2 "must_restart|capacity reached" # Search for documentation about leaf creation rg -i "leaf creation|create.*leaf" -g "*.md"Length of output: 10048
Script:
#!/bin/bash # Search for leaf caching patterns and usage rg -B 3 -A 3 "cached_leaf" # Look for leaf creation patterns ast-grep --pattern 'leaf($$$) { $$$ }'Length of output: 6698
optimistic_lock.hpp (3)
267-268: LGTM: Correct migration of unlikely branch prediction.The conversion from
UNODB_DETAIL_UNLIKELY(!result)toif (!result) [[unlikely]]is correct and maintains the same semantics. The attribute placement follows C++20 standards.
453-454: LGTM: Proper attribute placement for debug condition.The unlikely attribute is correctly placed for the debug-only condition checking. This maintains the original macro's intent while using modern C++20 syntax.
463-464: LGTM: Appropriate use of likely attribute.The conversion from
UNODB_DETAIL_LIKELY(result)toif (result) [[likely]]is correct. This is a common pattern in lock implementations where the success path is more likely.art.cpp (5)
283-284: LGTM: Correct attribute placement for root null check.The conversion from macro to attribute for the root null check is appropriate. This is a common pattern where the root being null is an uncommon case.
319-320: LGTM: Proper migration for initialization check.The unlikely attribute is correctly placed for the root null initialization check. This maintains the original optimization hint while using modern syntax.
334-335: LGTM: Appropriate attribute for key comparison.The unlikely attribute is correctly placed for the key equality check. This is appropriate as key collisions should be rare in a well-distributed tree.
386-387: LGTM: Proper attribute for removal edge case.The unlikely attribute is correctly placed for the root null check in remove operation. This maintains consistency with other similar checks.
422-423: LGTM: Correct attribute for removal result check.The unlikely attribute is appropriately placed for the removal result check. This follows the pattern where failure cases are marked as unlikely.
art_internal_impl.hpp (1)
163-165: LGTM! Clean migration to C++20 attribute.The replacement of
UNODB_DETAIL_UNLIKELYmacro with C++20's[[unlikely]]attribute is correct and maintains the same optimization hint for the compiler.test/qsbr_test_utils.cpp (1)
1-1: LGTM! Clean maintenance updates.The changes are purely maintenance-related:
- Updated copyright year to 2025
- Reorganized IWYU pragma directives
Also applies to: 13-16
|
1 participant
Leave the macros around for the cases the C++20 statement attributes don't
cover (i.e. subexpressions, return statement).
Summary by CodeRabbit
Release Notes
Style
[[likely]]and[[unlikely]]attributes instead of macros across multiple filesDocumentation
Chores