Skip to content

refactor(shred,storei): remove storei, replace dcache w/ wksp obj #4970

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lidatong
Copy link
Member

@lidatong lidatong commented May 1, 2025

This PR removes the storei tile from the firedancer topo.

Significantly, this requires replacing the shred_store dcache in the shred tile which houses the FEC sets. This was replaced with a new shared wksp object fec_sets. This conveniently allows shred and repair tiles to join the same shmem region, so repair can directly observe which FEC sets need repair (and request repairs for them). Currently, repair tile is maintaining an entirely separate copy of what the in-progress FEC sets are.

ripatel-fd
ripatel-fd previously approved these changes May 1, 2025
@lidatong lidatong marked this pull request as draft May 1, 2025 18:01
@@ -313,7 +311,6 @@ fd_topo_initialize( config_t * config ) {

/**/ fd_topob_link( topo, "stake_out", "stake_out", 128UL, 40UL + 40200UL * 40UL, 1UL );
/* See long comment in fd_shred.c for an explanation about the size of this dcache. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Also the comment

@lidatong lidatong force-pushed the chali/refactor/remove-storei branch from 0b2ed0e to aa1b61f Compare May 1, 2025 21:02
@lidatong lidatong changed the title refactor: remove storei refactor(shred,storei): remove storei, use wksp obj in shred for full fd May 1, 2025
@lidatong lidatong force-pushed the chali/refactor/remove-storei branch 4 times, most recently from 6e01daa to 10211e1 Compare May 1, 2025 22:48
@lidatong lidatong marked this pull request as ready for review May 1, 2025 22:48
@lidatong lidatong changed the title refactor(shred,storei): remove storei, use wksp obj in shred for full fd refactor(shred,storei): remove storei, use wksp obj for shred fec sets May 1, 2025
@lidatong lidatong changed the title refactor(shred,storei): remove storei, use wksp obj for shred fec sets refactor(shred,storei): remove storei, replace dcache w/ wksp obj May 1, 2025
@lidatong lidatong force-pushed the chali/refactor/remove-storei branch from 10211e1 to 4b0d3d4 Compare May 1, 2025 22:51
@lidatong
Copy link
Member Author

lidatong commented May 1, 2025

@ripatel-fd @mmcgee-jump can i get a re-review
@ptaffet-jump can you also take a look -- mainly fd_shred_tile and topo-related changes to that?

@lidatong
Copy link
Member Author

lidatong commented May 1, 2025

added description

/**/ fd_topob_tile_in( topo, "arch_f", 0UL, "metric_in", "store_feeder", 0UL, FD_TOPOB_RELIABLE, FD_TOPOB_POLLED );
fd_topob_wksp( topo, "replay_feeder" );
fd_topob_link( topo, "replay_feeder", "replay_feeder", 65536UL, 4UL*FD_SHRED_STORE_MTU, 4UL+config->tiles.shred.max_pending_shred_sets );
/**/ fd_topob_tile_out( topo, "storei", 0UL, "replay_feeder", 0UL );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be replay

ctx->repair_out_chunk = ctx->repair_out_chunk0;
FD_TEST( fd_dcache_compact_is_safe( ctx->repair_out_mem, repair_out->dcache, repair_out->mtu, repair_out->depth ) );
ulong fec_sets_obj_id = fd_pod_queryf_ulong( topo->props, ULONG_MAX, "fec_sets" );
if( FD_LIKELY( fec_sets_obj_id == ULONG_MAX ) ) FD_LOG_ERR(( "invalid firedancer topo" ));
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be unlikely

@lidatong lidatong force-pushed the chali/refactor/remove-storei branch from 4b0d3d4 to 395e283 Compare May 2, 2025 17:06
@ripatel-fd ripatel-fd dismissed two-heart’s stale review May 2, 2025 19:29

Changes addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants