Use FD_DCHECK instead of FD_TMPL_USE_HANDHOLDING#10186
Conversation
Performance Measurements ⏳
|
Greptile SummaryThis PR migrates all
Confidence Score: 3/5Mostly mechanical refactor but three files have debug-only code blocks that now run unconditionally in production builds, which may cause performance regressions on hot paths. The vast majority of changes (deque, set, pool, treap, heap, prq, dlist, slist, smallset) are clean 1:1 migrations with no behavioral change. However, fd_map.c, fd_map_dynamic.c, and fd_map_chain.c have code that was previously entirely compiled out in non-handholding builds but now always executes — specifically probe chain validation loops and duplicate key queries. These are on hot paths (map remove/insert) where unnecessary work matters. src/util/tmpl/fd_map.c, src/util/tmpl/fd_map_dynamic.c, and src/util/tmpl/fd_map_chain.c need attention — their remove() and idx_insert() functions have debug-only code blocks that are no longer guarded. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant map as fd_map remove()
participant dcheck as FD_DCHECK_CRIT
Note over Caller,dcheck: FD_DCHECK_STYLE==1 (Debug)
Caller->>map: remove(map, entry)
map->>dcheck: validate entry key
dcheck-->>map: CRIT if invalid
map->>map: walk probe chain (validation loop)
map->>dcheck: validate each slot
dcheck-->>map: CRIT if not found
map->>map: perform actual removal
Note over Caller,dcheck: FD_DCHECK_STYLE==0 (Production)
Caller->>map: remove(map, entry)
map->>dcheck: ((void)(c)) - no-op
map->>map: walk probe chain (loop still runs!)
map->>dcheck: ((void)(c)) - no-op
map->>map: perform actual removal
Note right of map: Extra O(probe_len) overhead
Reviews (1): Last reviewed commit: "Use FD_DCHECK instead of FD_TMPL_USE_HAN..." | Re-trigger Greptile |
![greptile-jt[bot]](https://avatars.githubusercontent.com/u/92124214?s=60&v=4){kind=small}
| { | ||
| MAP_KEY_T _key = entry->MAP_KEY; | ||
| MAP_HASH_T _hash = MAP_(key_hash)( _key ); | ||
| ulong _slot = MAP_(private_start)( _hash ); | ||
| for(;;) { | ||
| if( FD_UNLIKELY( MAP_(key_inval)( map[_slot].MAP_KEY ) ) ) FD_LOG_CRIT(( "entry is not in the map" )); | ||
| FD_DCHECK_CRIT( !MAP_(key_inval)( map[_slot].MAP_KEY ), "entry is not in the map" ); | ||
| if( FD_LIKELY( &map[_slot]==entry ) ) break; | ||
| _slot = MAP_(private_next)( _slot ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block was previously inside #if FD_TMPL_USE_HANDHOLDING and the comment on line 378 explicitly says "walks the probe chain a second time, acceptable for a debug-only check." After this refactor, the loop structure (variable declarations, for-loop with break, MAP_(private_next) calls) is always compiled and executed, even in production builds where FD_DCHECK_STYLE==0.
With FD_DCHECK_STYLE==0, FD_DCHECK_CRIT(c,m) becomes ((void)(c)), so the inner assertion is a no-op. However, the surrounding loop itself — which walks the probe chain until &map[_slot]==entry — is still executed. While a sufficiently aggressive optimizer might determine this is dead code (since _slot is unused after the block), this is not guaranteed for a loop with data-dependent bounds and memory reads.
This should be guarded with #if FD_DCHECK_STYLE==1 to preserve the original intent of being debug-only, or the entire block should be wrapped in an FD_DCHECK_ALERT-like pattern that compiles out entirely in production.
| { | ||
| ulong _slot_mask = hdr->slot_mask; | ||
| MAP_KEY_T _key = entry->MAP_KEY; | ||
| MAP_HASH_T _hash = MAP_(key_hash)( _key, hdr->seed ); | ||
| ulong _slot = MAP_(private_start)( _hash, _slot_mask ); | ||
| for(;;) { | ||
| if( FD_UNLIKELY( MAP_(key_inval)( map[_slot].MAP_KEY ) ) ) FD_LOG_CRIT(( "entry is not in the map" )); | ||
| FD_DCHECK_CRIT( !MAP_(key_inval)( map[_slot].MAP_KEY ), "entry is not in the map" ); | ||
| if( FD_LIKELY( &map[_slot]==entry ) ) break; | ||
| _slot = MAP_(private_next)( _slot, _slot_mask ); | ||
| } | ||
| } |
There was a problem hiding this comment.
fd_map.c
Same concern as in fd_map.c: the probe chain validation loop that was previously guarded by #if FD_TMPL_USE_HANDHOLDING is now always compiled. The comment on line 433 says "Acceptable for a debug-only check." With FD_DCHECK_STYLE==0, the FD_DCHECK_CRIT calls become no-ops but the loop itself still executes, adding O(probe_length) overhead to every remove() call in production builds.
This block should be guarded with #if FD_DCHECK_STYLE==1 to maintain the original intent.
| # if !MAP_MULTI | ||
| FD_DCHECK_CRIT( MAP_(private_idx_is_null)( MAP_(idx_query)( join, &pool[ ele_idx ].MAP_KEY, MAP_(private_idx_null)(), pool ) ), | ||
| "ele_idx already in map" ); | ||
| # endif |
There was a problem hiding this comment.
Previously this was guarded by #if FD_TMPL_USE_HANDHOLDING && !MAP_MULTI. Now the #if FD_TMPL_USE_HANDHOLDING guard is removed, leaving only #if !MAP_MULTI. With FD_DCHECK_STYLE==0, FD_DCHECK_CRIT(c,m) expands to ((void)(c)), which means MAP_(idx_query)(...) — a full hash chain traversal — is always evaluated in production for every insert on non-multi maps. The result is discarded, but the traversal still executes.
Whether the compiler can elide this call depends on whether MAP_(idx_query) is provably side-effect-free. To preserve the original intent, this should be:
| # if !MAP_MULTI | |
| FD_DCHECK_CRIT( MAP_(private_idx_is_null)( MAP_(idx_query)( join, &pool[ ele_idx ].MAP_KEY, MAP_(private_idx_null)(), pool ) ), | |
| "ele_idx already in map" ); | |
| # endif | |
| # if !MAP_MULTI && FD_DCHECK_STYLE | |
| FD_DCHECK_CRIT( MAP_(private_idx_is_null)( MAP_(idx_query)( join, &pool[ ele_idx ].MAP_KEY, MAP_(private_idx_null)(), pool ) ), | |
| "ele_idx already in map" ); | |
| # endif |
No description provided.