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

flamenco: back input regions and bincode valloc with spad #3423

Merged
merged 1 commit into from
Dec 19, 2024
Merged

flamenco: back input regions and bincode valloc with spad #3423

merged 1 commit into from
Dec 19, 2024

Conversation

yufeng-jump
Copy link
Contributor

No description provided.

@yufeng-jump yufeng-jump force-pushed the yufeng/input-region branch 7 times, most recently from c593b37 to 7b296ab Compare November 19, 2024 16:16
@yufeng-jump yufeng-jump force-pushed the yufeng/input-region branch 3 times, most recently from cff5656 to 763dfc8 Compare November 21, 2024 19:47
@yufeng-jump yufeng-jump changed the title flamenco: size out input regions and migrate them to spad flamenco: back input regions and bincode valloc with spad Nov 21, 2024
@yufeng-jump yufeng-jump force-pushed the yufeng/input-region branch 6 times, most recently from 7e4c8fc to 23ccf0d Compare November 22, 2024 19:01
@yufeng-jump yufeng-jump force-pushed the yufeng/input-region branch 4 times, most recently from b187aec to 26d622f Compare December 9, 2024 20:25
@yufeng-jump yufeng-jump force-pushed the yufeng/input-region branch 6 times, most recently from b1503ec to 4715366 Compare December 17, 2024 17:30
ibhatt-jumptrading
ibhatt-jumptrading reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@ibhatt-jumptrading ibhatt-jumptrading left a comment

Choose a reason for hiding this comment

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

I think mostly lgtm, I think it would be a lot better if we just stored a handle to the spad for the instr_ctx and txn_ctx instead of having to handle and pass around an spad pointer and a scratch valloc

src/flamenco/runtime/fd_runtime_spad.c Outdated
@@ -0,0 +1,26 @@
#include "fd_runtime_spad.h"

Copy link
Contributor

Choose a reason for hiding this comment

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

this is nice

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we add this to util and just generally expose a valloc for spad? There's nothing necessarily runtime specific about this

src/flamenco/runtime/program/fd_bpf_loader_program.c
@@ -236,11 +237,13 @@ calculate_heap_cost( ulong heap_size, ulong heap_cost, int * err ) {
int
fd_deploy_program( fd_exec_instr_ctx_t * instr_ctx,
uchar const * programdata,
ulong programdata_size ) {
ulong programdata_size,
fd_valloc_t valloc /* Majority of use cases this is a txn ctx spad under the hood; for native to bpf migration this is scratch backed since it's an epoch level event. */
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to bound out the epoch level events too. Probably makes sense to do in a follow up PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

src/flamenco/runtime/context/fd_exec_instr_ctx.h Outdated
@@ -25,7 +25,7 @@ struct __attribute__((aligned(8UL))) fd_exec_instr_ctx {

fd_funk_txn_t * funk_txn;
fd_acc_mgr_t * acc_mgr;
fd_valloc_t valloc;
fd_valloc_t valloc; /* Scratch-backed valloc TODO: migrate to transaction spad */
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just set the instr ctx valloc to the spad valloc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some instruction level test harnesses don't give us spads and instead give us libc malloc valloc and etc. We can do that change in a future PR? I'll update the comment to reflect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I renamed this to test_only_valloc and left a comment to make it clear.

src/flamenco/runtime/context/fd_exec_txn_ctx.h Outdated Show resolved Hide resolved
src/flamenco/runtime/tests/fd_exec_sol_compat.c Outdated Show resolved Hide resolved
src/flamenco/runtime/tests/fd_exec_instr_test.c
@@ -1657,6 +1670,7 @@ fd_exec_vm_syscall_test_run( fd_exec_instr_test_runner_t * runner,
if( !fd_exec_test_instr_context_create( runner, ctx, input_instr_ctx, alloc, skip_extra_checks ) )
goto error;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure we can just use spad here too (like in the valloc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's migrate these non critical path test harness allocations in a future PR, and keep this PR about live or replay transaction execution?

src/flamenco/runtime/fd_runtime.c Outdated Show resolved Hide resolved
src/flamenco/runtime/fd_runtime.h
@@ -95,6 +97,170 @@ FD_STATIC_ASSERT( FD_ACCOUNT_REC_ALIGN>=FD_ACCOUNT_REC_DATA_ALIGN, account_rec_d
FD_STATIC_ASSERT( (offsetof(fd_account_rec_t, meta)%FD_ACCOUNT_META_ALIGN)==0, account_rec_meta_offset );
FD_STATIC_ASSERT( (offsetof(fd_account_rec_t, data)%FD_ACCOUNT_REC_DATA_ALIGN)==0, account_rec_data_offset );

#define MAX_PERMITTED_DATA_INCREASE (10240UL) // 10KB
Copy link
Contributor

Choose a reason for hiding this comment

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

🐐

src/flamenco/runtime/program/fd_bpf_loader_serialization.h Outdated Show resolved Hide resolved
@yufeng-jump yufeng-jump force-pushed the yufeng/input-region branch 2 times, most recently from 31496cf to 2a82177 Compare December 18, 2024 23:09
@yufeng-jump yufeng-jump force-pushed the yufeng/input-region branch 3 times, most recently from 2a24732 to 55217cf Compare December 19, 2024 22:33
@yufeng-jump
flamenco: back input regions and bincode valloc with spad
594a1ce 
Also back miscellaneous single transaction execution scoped
allocations with spad.

The valloc handle in txn_ctx and instr_ctx are gone.
@yufeng-jump yufeng-jump added this pull request to the merge queue Dec 19, 2024
Merged via the queue into main with commit 0abec17 Dec 19, 2024
10 of 11 checks passed
@yufeng-jump yufeng-jump deleted the yufeng/input-region branch December 19, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibhatt-jumptrading ibhatt-jumptrading ibhatt-jumptrading approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@yufeng-jump @ibhatt-jumptrading