rust/core/error: Make `Error` struct be boxed so stack frames aren't very large

[serramatutu]

Problem

Currently the adbc_core::Error struct contains all the fields (message, status, vendor code etc), and it is directly embedded in adbc_core::Result<T>. That means regardless of sizeof(T), adbc_core::Result<T> is always at least sizeof(Error), which is large.

This makes applications pay a lot of stack frame space for the cold error handling path since every Result is big.

In concrete terms

On my ARM Mac:

So currently the error is 64 bytes large whereas a pointer is only 8.

Solution

Change it to something like

pub struct Error(Box<ErrorInner>);

pub struct ErrorInner {
    message: String,
    // ...
}

impl Error {
    pub fn message(&self) -> &str {
        &self.0.message
    }
    
    // ....
}

Now sizeof(Error) is effectively the size of a thin pointer. The downside is that this requires an extra allocation and dereference on every error, but I assume that's okay since errors are (hopefully) in the cold path.

Note that this will be a massive breaking change since currently all the fields in Error are pub so virtually everyone doing error handling based on those public fields will need to change their code to use accessors instead. We'll also need a constructor and people will need to move away from manually initializing the struct.

The current workaround

Write a wrapper to all methods of the driver manager that returns a custom error type that does the wrapping for you.

The Rust driverbase of ADBC Foundry already does that: https://github.com/adbc-drivers/driverbase-rs/blob/fe2738281cae9a2a92c59cc8c4d0822395187d04/driverbase/src/error.rs#L19

Edits

EDIT1: Added MacOS size comparison

[lidavidm]

I was thinking about this too. It'll be messy, but we do have a chance to fix this since technically we're still in 0.x. (You could perhaps argue that drivers should use their own error type internally and convert at the boundaries, but that's also a bit annoying.)

[lidavidm]

CC @eitsupi @paleolimbot for opinions as well!

[paleolimbot]

I think it's fine, and will probably not be that hard to adapt to.

For what it's worth, DataFusion's result is 40 bytes. I am surprised that there are deep stacks so populated with ADBC calls that it affects the stack size, but if it's easy to do and helpful I think it's a good move.

[serramatutu]

@paleolimbot worth noting: we don't have stacks that go that deep and it's not a dealbreaker for us. I'm just suggesting this as an improvement before 1.0, since after that it's a lot harder to make breaking changes like this.

[eitsupi]

I think it makes sense to make a breaking change for this now.