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.

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

Feat: Add Option to move init constraint codegen into separate functions #2920

Open
nabeel99 opened this issue Apr 20, 2024 · 8 comments
Open
Labels

Comments

@nabeel99
Copy link
Contributor

Problem :
As described in #2915 , Init constraints take up a lot of stack space, a medium number of them inside a single derive accounts would cause stack violated errors, the solution is to use composite structs but the issue is it leads to account duplication, Is there anything in particular that blocks the above feature request ? if not I would like to contribute this feature or atleast make it optional for people who need it , can use it.

@acheroncrypto
Copy link
Collaborator

Yeah, reducing stack usage is one of the top priorities now. Always creating a separate functions might even be better for Anchor. If so, we wouldn't need to make this optional.

Is there anything in particular that blocks the above feature request ? if not I would like to contribute this feature or atleast make it optional for people who need it , can use it.

I don't think there is anything particular that blocks this feature request. However, as with any performance issue, the problem must be identified and benchmarked properly first, rather than just making the codegen changes.

@nabeel99
Copy link
Contributor Author

nabeel99 commented Apr 20, 2024

I don't think there is anything particular that blocks this feature request. However, as with any performance issue, the problem must be identified and benchmarked properly first, rather than just making the codegen changes.

how would that process looks like, care to point an example of this previously done i can follow , as the first part of it is done right ? its identified, what would an acceptable benchmark look like for the above issue ? as far as i understand its not possible to debug stack usage (i am not sure on this) if so how would a benchmark work ?
Just an overview on the process , so i can open a pr and get this done.

@acheroncrypto
Copy link
Collaborator

It's not an exact science, but something like the following would help:

  • Compare the current changes (with the improved stack memory usage for constraints) to the last published version(s).
  • Keep everything else the same (e.g. Solana version)
  • Potentially demonstrate the ability to use more constraints without hitting stack errors

This could be done in tests/bench, which have similar tests that compare various benchmarks between published versions.

@nabeel99
Copy link
Contributor Author

nabeel99 commented Apr 21, 2024

@acheroncrypto reviewing the output code and the anchor lang internal generation code for constrains, it seems the code output is wrapped in a seperate scope by being wrapped in a scope({... init code}) if so shdnt this already alleviate stack space size issues since once the scope ends the stack space allocated within that scope is returned ?
My question : shdnt a seperate scope function the same way as a seperate function(ik it gets allocated a fresh frame)
but isnt

let init_inside_seperate_scope = {..constriant code in scope};
vs
let init_seperate_function = seperate_init_function(...params);

basically the same thing from a stack size p.o.v, after the scope closes everything inside is dropped and deallocated(unless am wrong here)?
The relevant code :

#[derive(Accounts)]
pub struct Initialize<'info> {
    #[account(mut)]
    pub payer: Signer<'info>,
    #[account(init,payer=payer,space=10)]
    pub dummy_a: Account<'info, DummyA>,
    pub system_program: Program<'info, System>,

Produces -> (only dummy_a code shown)

let __anchor_rent = Rent::get()?;
        let dummy_a =
        ///start of the init constraint scope
          {
            let actual_field = AsRef::<AccountInfo>::as_ref(&dummy_a);
            let actual_owner = actual_field.owner;
            let space = 10;
            let pa: anchor_lang::accounts::account::Account<DummyA> =
            if !false || actual_owner == &anchor_lang::solana_program::system_program::ID {
                    let __current_lamports = dummy_a.lamports();
                    if __current_lamports == 0 {
                        let space = space;
                        let lamports = __anchor_rent.minimum_balance(space);
                        let cpi_accounts = anchor_lang::system_program::CreateAccount {
                            from: payer.to_account_info(),
                            to: dummy_a.to_account_info(),
                        };
                        let cpi_context = anchor_lang::context::CpiContext::new(
                            system_program.to_account_info(),
                            cpi_accounts,
                        );
                        anchor_lang::system_program::create_account(
                            cpi_context.with_signer(&[]),
                            lamports,
                            space as u64,
                            __program_id,
                        )?;
                    } 
                    /// removed the else code to shorten the code for reading purposes
                    match anchor_lang::accounts::account::Account::try_from_unchecked(&dummy_a) {
                        Ok(val) => val,
                        Err(e) => return Err(e.with_account_name("dummy_a")),
                    }
                } else {
                    match anchor_lang::accounts::account::Account::try_from(&dummy_a) {
                        Ok(val) => val,
                        Err(e) => return Err(e.with_account_name("dummy_a")),
                    }
                };
           
            pa
        }; 
        ///end of the init constraint scope
        ///shdnt the stack space used by the above init constraint be freed at this point 
        
... `other init constraints`
          ```

@nabeel99
Copy link
Contributor Author

@acheroncrypto it would be really easy to figure this out via experimentations if there is a way to measure stack space usage , i dont seem to know if there is any, would you happen to know of any ? if so it should help a ton in this specific scenario.

@nabeel99
Copy link
Contributor Author

@acheroncrypto reviewing the output code and the anchor lang internal generation code for constrains, it seems the code output is wrapped in a seperate scope by being wrapped in a scope({... init code}) if so shdnt this already alleviate stack space size issues since once the scope ends the stack space allocated within that scope is returned ? My question : shdnt a seperate scope function the same way as a seperate function(ik it gets allocated a fresh frame) but isnt

let init_inside_seperate_scope = {..constriant code in scope};
vs
let init_seperate_function = seperate_init_function(...params);

basically the same thing from a stack size p.o.v, after the scope closes everything inside is dropped and deallocated(unless am wrong here)?

was curious about the semantics around rust behavior of re-using of stack space, the conclusion i reached its not deterministic and sometimes might occur in some situation and might not , see here but solution is just to use modify the codegen to generate internal functions at compile time and use them to ensure a fresh solana stack frame is used.

@acheroncrypto
Copy link
Collaborator

@acheroncrypto it would be really easy to figure this out via experimentations if there is a way to measure stack space usage , i dont seem to know if there is any, would you happen to know of any ? if so it should help a ton in this specific scenario.

Allocating an array that blows the stack usually results in a build error:

`let stack_limit: [u16; 2048] = [1; 2048];`,
`msg!("{}", stack_limit[2047]);`,

It logs something like this:

Error: Function _ZN5bench9__private8__global13account_info117h88e5c10f03de9fddE
Stack offset of 4424 exceeded max offset of 4096 by 328 bytes

That example is for the instruction handler function of Anchor, but you can also do it with constraints. However, if this doesn't work, and you'd need to check the binary itself to see what's going on, which is not the best experience to say the least:

cargo build-sbf --dump

as curious about the semantics around rust behavior of re-using of stack space, the conclusion i reached its not deterministic and sometimes might occur in some situation and might not , see here but solution is just to use modify the codegen to generate internal functions at compile time and use them to ensure a fresh solana stack frame is used.

Yeah, I think it's very difficult to tell if the compiler optimizes memory related stuff or not. Only reliable way I can think of is to analyze the generated bytecode.

@nabeel99
Copy link
Contributor Author

nabeel99 commented May 19, 2024

hey @acheroncrypto as discussed in the thread #2939 , its possible to guarantee #[inline(never)] closures , by wrapping them in curly braces, is this ok to move forward , wrap all init code in closures and add the benchmarks ? as shown in the other thread i saw massive improvements in stack space usage close to 95%.

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

No branches or pull requests

2 participants