coral-xyz / anchor

⚓ Solana Sealevel Framework
https://anchor-lang.com
Apache License 2.0
3.36k stars 1.25k forks source link

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

Open nabeel99 opened 2 months ago

nabeel99 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

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

nabeel99 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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:

https://github.com/coral-xyz/anchor/blob/c96846fce27b1eccd30c810546cd9daf4be2cbbf/tests/bench/tests/stack-memory.ts#L13-L14

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 commented 1 month ago

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%.