Conversation
There was a problem hiding this comment.
Pull request overview
This PR is a work-in-progress refactor towards a concurrent/shared-cache accdb/vinyl integration. It introduces new cross-tile speculative-read state in fd_vinyl_line_t, makes the vinyl data allocator concurrent via spinlocks, and shifts “rooting”/writeback logic away from the old admin/client batch protocol toward the accdb tile.
Changes:
- Added
specread_ctlbitfield layout + expandedfd_vinyl_line_tto a fixed 64B cacheline-sized structure. - Made
fd_vinyl_dataallocator thread-safe using per-sizeclass spinlocks and avol_lock. - Refactored accdb v2 rooting + client interactions (removed
fd_accdb_ctl, removed old v2 rooting impl, added new accdb tile rooting path, and trimmed associated metrics/telemetry).
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vinyl/line/fd_vinyl_line.h | Adds specread_ctl layout and restructures fd_vinyl_line_t for concurrent speculative reads / CLOCK eviction metadata. |
| src/vinyl/data/fd_vinyl_data.h | Updates documentation and adds spinlock fields to support concurrent alloc/free. |
| src/vinyl/data/fd_vinyl_data.c | Implements per-sizeclass and volume spinlocks around alloc/free/reset paths. |
| src/vinyl/data/test_vinyl_data.c | Updates footprint assertion to match the larger fd_vinyl_data_t. |
| src/flamenco/accdb/fd_accdb_admin_v2.h | Changes v2 admin structure/ABI and simplifies init signature. |
| src/flamenco/accdb/fd_accdb_admin_v2.c | Removes old vinyl client plumbing; v2 publish/root logic is now stubbed/refactored. |
| src/flamenco/accdb/fd_accdb_impl_v2.c | Removes RELEASE logic and v2 ref closing logic (currently left as FIXMEs). |
| src/flamenco/accdb/test_accdb_v2.c | Removes vinyl seeding logic and replaces it with a FIXME placeholder. |
| src/flamenco/accdb/fd_accdb_ctl.c | Deleted: CLI debugging tool for the old vinyl-backed accdb workflow. |
| src/flamenco/accdb/fd_accdb_admin_v2_root.c / fd_accdb_admin_v2_private.h | Deleted: old batch rooting implementation. |
| src/flamenco/accdb/Local.mk | Stops building the deleted rooting object and ctl tool. |
| src/discof/accdb/fd_accdb_tile_private.h | Introduces CLOCK-based eviction helper and private tile definitions. |
| src/discof/accdb/fd_accdb_case_acquire.c | Adds accdb-tile ACQUIRE request handling based on shared cache. |
| src/discof/accdb/fd_accdb_tile_root.c | Adds a new rooting path that appends pair/dead blocks and updates meta/cache. |
| src/discof/accdb/fd_accdb_tile.c | Refactors the tile to use new private header/types and new acquire case include. |
| src/discof/replay/fd_replay_tile.c / src/discof/genesis/fd_genesi_tile.c | Updates v2 admin init call sites to new signature and trims metrics usage. |
| src/disco/metrics/metrics.xml + generated replay metrics + docs | Removes rooting-related replay metrics and updates generated artifacts accordingly. |
| src/app/shared/commands/watch/watch.c | Removes display of deleted replay accdb/rooting metrics. |
| src/app/firedancer/topology.c | Adjusts vinyl rq topology wiring for genesi. |
| book/api/metrics-generated.md | Updates published metrics documentation to reflect removals. |
Comments suppressed due to low confidence (1)
src/flamenco/accdb/fd_accdb_impl_v2.c:638
fd_accdb_user_v2_close_ref_multino longer releases V2/vinyl-backed refs (it even skips all non-V1 refs), so V2 handles will leak andro_active/rw_activewill not be decremented for those refs. This is a functional regression and can lead to quota exhaustion. Implement the vinyl RELEASE logic forFD_ACCDB_TYPE_V2refs (and keep the funk release forFD_ACCDB_TYPE_V1) before clearing the ref.
/* FIXME Release vinyl records */
/* Release funk records */
for( ulong i=0UL; i<cnt; i++ ) {
fd_accdb_ref_t * ref = &ref0[ i ];
if( ref->accdb_type!=FD_ACCDB_TYPE_V1 ) continue;
switch( ref0[ i ].ref_type ) {
case FD_ACCDB_REF_RO:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FD_CRIT ( rd_err, "corruption detected" ); | ||
| FD_ALERT( fd_vinyl_data_is_valid_obj( obj, vol, vol_cnt ), "corruption detected" ); | ||
|
|
There was a problem hiding this comment.
vol and vol_cnt are referenced here but are no longer defined in before_credit after the earlier refactor (the local vol=data->vol / vol_cnt=data->vol_cnt variables were removed). This will fail to compile (and also breaks fd_vinyl_data_is_valid_obj). Reintroduce those locals (or remove this validation call if it is no longer needed).
| } | ||
|
|
||
| fd_vinyl_req_pool_release( accdb->vinyl_req_pool, batch_idx ); | ||
| /* FIXME direct emplace account into vinyl cache */ |
There was a problem hiding this comment.
add_account_vinyl is used by run_tests to seed vinyl state, but it now does nothing. This will make the unit test incorrect/failing (keys a/b/d will no longer exist in vinyl as assumed by later assertions). Either restore the seeding logic or update the test setup to populate vinyl via the new accdb tile/cache APIs.
| /* FIXME direct emplace account into vinyl cache */ | |
| /* For test purposes, seed via the same path as funk so accounts exist */ | |
| add_account_funk( accdb_, key, lamports ); |
No description provided.