Skip to content
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.

Sign up for GitHub

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

Jump to bottom

shred: notify replay tile of completed shreds (do not merge) #4361

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

shred: notify replay tile of completed shreds (do not merge) #4361

wants to merge 2 commits into from
+223 −38

Conversation

emwang-jump
Copy link
Contributor

No description provided.

mmcgee-jump
mmcgee-jump reviewed Mar 3, 2025
View reviewed changes
src/app/fdctl/run/tiles/fd_shred.c Outdated

fd_stem_publish( stem, STORE_OUT_IDX, new_sig, fd_laddr_to_chunk( ctx->store_out_mem, s34+3UL ), sz3, 0UL, ctx->tsorig, tspub );

/* Send to replay tile */
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't add a 2nd publish here, which will break Frankendancer.

The publish is "either or" (either shred_store, or shred_replay), so you can just make sure in unprivileged_init you either have one shred_store or one shred_replay, and then set STORE_OUT_IDX (maybe rename SHRED_OUT_IDX?) to that link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I have a change that conditions the second publish - but I think we want to keep both the shred_store and shred_replay link for firedancer for now to roll this change out slowly

Copy link
Member

Choose a reason for hiding this comment

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

we can ifdef that link too

Copy link
Member

Choose a reason for hiding this comment

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

actually we're changing the sig so dont think we can reuse the link like this @mmcgee-jump

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't want to use the ifdef, Richie is going to remove that pretty soon. The tile should work for either topology without recompilation.

Copy link
Member

Choose a reason for hiding this comment

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

well we need to figure out a sig that works for both then. it looks like all the FFI store needs is a single bit sig field

Copy link
Member

Choose a reason for hiding this comment

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

You don't want to use the ifdef, Richie is going to remove that pretty soon.

why?

The tile should work for either topology without recompilation.

i dont see how we can make this work with the shred tile

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell you could do this change in two lines of code by just creating a "shred_store" link in the fd_firedancer topology that goes to replay instead of store, and then updating the sig so it works for both consumers. Then it can be compiled once and linked by either library, too, and doesn't pollute the file with a bunch of ifdefs.

Copy link
Member

Choose a reason for hiding this comment

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

yeah thats what i meant in my comment above about changing the sig of msgs on the link. but i dont see how we can modify the shred tile's stem publish or blcokstore_insert without an ifdef

Copy link
Contributor

Choose a reason for hiding this comment

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

Well at a minimum it can be an if rather than an ifdef. The tiles are library code built in disco, so it doesn't make sense to push application knowledge (frankendancer vs full firedancer) into there.

Basically you should just build this as generic library functionality in the tile, probably I would add an argument to the tile topo or whatever ulong blockstore_obj_id and if this is not ULONG_MAX, you store shreds directly into there before broadcasting. Nothing else really needs to change except the signature?

@emwang-jump emwang-jump marked this pull request as ready for review March 3, 2025 18:48
@emwang-jump emwang-jump force-pushed the emwang/shred-replay-link branch 2 times, most recently from 6770793 to ec45d1c Compare March 3, 2025 19:07
@emwang-jump emwang-jump changed the title shred replay link shred: notify replay tile of completed shreds Mar 3, 2025
@emwang-jump emwang-jump force-pushed the emwang/shred-replay-link branch 2 times, most recently from 5ed3302 to 000f6fb Compare March 3, 2025 22:52
lidatong
lidatong reviewed Mar 3, 2025
View reviewed changes
src/app/fdctl/run/tiles/fd_replay.c Outdated
Comment on lines 526 to 529
ulong slot = fd_disco_shred_sig_slot( sig );
ushort parent_off = fd_disco_shred_sig_parent_off( sig );
uint fec_idx = fd_disco_shred_sig_fec_idx( sig );
uchar flags = fd_disco_shred_sig_flags( sig );
Copy link
Member

Choose a reason for hiding this comment

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

fmt

src/app/fdctl/run/tiles/fd_shred.c Outdated
@@ -91,6 +95,7 @@
#define STORE_OUT_IDX 0
#define NET_OUT_IDX 1
#define SIGN_OUT_IDX 2
#define MULTI_OUT_IDX 3
Copy link
Member

Choose a reason for hiding this comment

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

replay_out?

src/app/fdctl/run/tiles/fd_shred.c Outdated
#if FD_HAS_NO_AGAVE
/* Store shreds to blockstore */
for( ulong i=0UL; i<set->data_shred_cnt; i++ ) {
fd_shred_t const * data_shred = (fd_shred_t const *)set->data_shreds[ i ];
Copy link
Member

Choose a reason for hiding this comment

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

need type pun

src/disco/fd_disco_base.h Outdated
@@ -121,6 +121,29 @@ fd_disco_replay_sig( ulong slot,
FD_FN_CONST static inline ulong fd_disco_replay_sig_flags( ulong sig ) { return (sig & 0xFFUL); }
FD_FN_CONST static inline ulong fd_disco_replay_sig_slot( ulong sig ) { return (sig >> 8); }

FD_FN_CONST static inline ulong
fd_disco_shred_sig( ulong slot,
Copy link
Member

Choose a reason for hiding this comment

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

replay_sig

src/disco/fd_disco_base.h Outdated
| slot[32:63] | parent_off[24:31] | shred_idx[9:23] | shred_flags[8:1] | is_turbine[0:0] |
*/
return ((slot & 0xFFFFFFFF) << 32 )
| ((ulong)(parent_off & 0xFF) << 24 )
Copy link
Member

Choose a reason for hiding this comment

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

overflow check might be able to do something fancy with fd_bits

@emwang-jump emwang-jump force-pushed the emwang/shred-replay-link branch from 995caf2 to 7353796 Compare March 3, 2025 23:10
@lidatong lidatong changed the title shred: notify replay tile of completed shreds shred: notify replay tile of completed shreds (do not merge) Mar 3, 2025
@emwang-jump emwang-jump force-pushed the emwang/shred-replay-link branch 2 times, most recently from 50eb493 to 3421893 Compare March 4, 2025 20:39
@emwang-jump emwang-jump force-pushed the emwang/shred-replay-link branch from 3421893 to cb66392 Compare March 4, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidatong lidatong lidatong left review comments

@mmcgee-jump mmcgee-jump mmcgee-jump left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emwang-jump @lidatong @mmcgee-jump