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

Space efficient panic #4306

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

Space efficient panic #4306

wants to merge 2 commits into from

Conversation

LucasSte
Copy link

@LucasSte LucasSte commented Jan 6, 2025

Problem

Our implementation of custom_panic can consume up to 25kb in contracts. This happens because it relies on the format! macro and, consequently, on std::fmt::write. They include many more functions in the contract and utilize dynamic dispatch, a technique that hinders compiler and link side optimizations for size reduction.

Summary of Changes

I implemented a new custom_panic that functions independently with only two syscalls. It needs the stabilization of fmt::Arguments::as_str, which is happening in Rust 1.84 (see rust-lang/rust#132511).

Size comparison

Take this simple contract as an example:

entrypoint!(process_instruction);

fn process_instruction(
    _program_id: &Pubkey,
    accounts: &[AccountInfo],
    instruction_data: &[u8],
) -> ProgramResult {
    Ok(())
}

The binary size has whooping 18888 bytes (18kb).
The contract with an empty custom_panic function has 11536 bytes (11kb), so panic is consuming 7352 bytes.
The contract with my new implementation has 11800 bytes (11kb), so my implementation has 264 bytes.

New error messages

The members of fmt::Arguments are all private, so I cannot build custom panic messages during runtime as Rust does (think about the error you get when you access an invalid index from a vector: we can only know the index and the vector length during execution time). These messages will be elided in the new panic implementation (see examples below). Error messages whose content is known at compile time will still be shown normally.

The formatting is also different. It is more efficient to call sol_log multiple times than to format a string.

Accessing an invalid index from a vector:

OLD:

'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S invoke [1]'
'Program log: panicked at src/lib.rs:19:13:\nindex out of bounds: the len is 44 but the index is 85034'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S consumed 2495 of 200000 compute units'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S failed: SBF program panicked'

NEW:

'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S invoke [1]'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S consumed 546 of 200000 compute units'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S failed: SBF program Panicked in src/lib.rs at 19:13'
Calling unwrap on a None:

OLD:

'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S invoke [1]'
'Program log: panicked at src/lib.rs:21:15:\ncalled `Option::unwrap()` on a `None` value'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S consumed 2118 of 200000 compute units'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S failed: SBF program panicked'

NEW:

'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S invoke [1]'
'Program log: called `Option::unwrap()` on a `None` value'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S consumed 643 of 200000 compute units'
'Program C6CNmEDPNNKQwrH8BmAYAux33D1PuzJSNSbXyRWwv27S failed: SBF program Panicked in src/lib.rs at 21:15'

@LucasSte LucasSte force-pushed the panic-2 branch 2 times, most recently from 67312c9 to 06c5ca8 Compare January 7, 2025 13:47
@LucasSte LucasSte changed the title Implement space-efficient panic Space efficient panic Jan 7, 2025
Copy link

mergify bot commented Jan 9, 2025

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@LucasSte LucasSte force-pushed the panic-2 branch 4 times, most recently from eb4f563 to ab6c1ba Compare January 10, 2025 17:56
@LucasSte LucasSte requested review from febo and joncinque January 10, 2025 20:19
@LucasSte LucasSte marked this pull request as ready for review January 10, 2025 20:19
@LucasSte LucasSte requested a review from a team as a code owner January 10, 2025 20:19
@Lichtso
Copy link

Lichtso commented Jan 14, 2025

Adding new syscalls would require a SIMD. But maybe there is a way to keep using the existing syscalls but with a new panic handler?

@LucasSte
Copy link
Author

Adding new syscalls would require a SIMD. But maybe there is a way to keep using the existing syscalls but with a new panic handler?

I'm not adding a new syscall. sol_panic is registered, but was not exposed in the SDK.

@LucasSte
Copy link
Author

result.register_function("sol_panic_", codes::SOL_PANIC, SyscallPanic::vm)?;

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.

2 participants