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

Memory bloat #44

Open
est31 opened this Issue Sep 16, 2016 · 23 comments

Comments

Projects
None yet
6 participants
@est31

est31 commented Sep 16, 2016

The code (basing on the example from the readme)

#![recursion_limit = "1024"]
#[macro_use]
extern crate error_chain;

mod errors {
    error_chain! { }
}

fn main () {
    use errors::Result as ChainResult;
    use std::mem::size_of;
    println!("size of vanilla result {}", size_of::<Result<(), ()>>());
    println!("size of error_chain result {}", size_of::<ChainResult<()>>());
}

outputs:

size of vanilla result 1
size of error_chain result 56

I might be wrong, but isn't ChainResult just "yet another enum", which means those 55 extra bytes have to be copied over when returning, even in the success event? That would mean quite a performance overhead over normal Result in hot code, especially where only the Ok case which may be minimal is encountered, no?

Maybe the situation can be improved by making the backtrace functionality optional, as in possible to opt out of at compile time?

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Oct 1, 2016

@kali has made a very interesting overview over the origin of the 56 bytes: #45 (comment)

est31 commented Oct 1, 2016

@kali has made a very interesting overview over the origin of the 56 bytes: #45 (comment)

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Oct 1, 2016

This doesn't seem like a great comparison - Result<(), ()> carries no data aside from the discriminant. No one is using Result<(), ()> (if they are, they should probably be using bool or a new enum). How does an error_chain generated result compare to say io::Result, carrying the same information?

withoutboats commented Oct 1, 2016

This doesn't seem like a great comparison - Result<(), ()> carries no data aside from the discriminant. No one is using Result<(), ()> (if they are, they should probably be using bool or a new enum). How does an error_chain generated result compare to say io::Result, carrying the same information?

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Oct 2, 2016

if they are, they should probably be using bool or a new enum

Result<(), ()> is very useful as it can be used together with the try macro, and it carries the information for the reader with it that the operation is failable. Bool can mean anything, you have to read the docs to find out what it means. Documenting your API over the type system is one of the things Rust is about. Also, it gives the user a warning that it needs to be used, unlike bool.

Regardless of whether Result<(),()> is considered something to avoid or not, which can be surely discussed about, I only wanted to measure what the additional overhead was of the error-chain error.

And about io::Result: io::Result<()> uses 24 bytes, which is still less than half of what the error-chain Result is carrying.

est31 commented Oct 2, 2016

if they are, they should probably be using bool or a new enum

Result<(), ()> is very useful as it can be used together with the try macro, and it carries the information for the reader with it that the operation is failable. Bool can mean anything, you have to read the docs to find out what it means. Documenting your API over the type system is one of the things Rust is about. Also, it gives the user a warning that it needs to be used, unlike bool.

Regardless of whether Result<(),()> is considered something to avoid or not, which can be surely discussed about, I only wanted to measure what the additional overhead was of the error-chain error.

And about io::Result: io::Result<()> uses 24 bytes, which is still less than half of what the error-chain Result is carrying.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Oct 2, 2016

Regardless of whether Result<(),()> is considered something to avoid or not, which can be surely discussed about, I only wanted to measure what the additional overhead was of the error-chain error.

OK but you've used a type which has 0 bytes of data for the non-discriminant portion. All you've documented is that the largest variant of the null error chain is 55 bytes. That doesn't mean that the equivalent of io::Result is 79 bytes, it might still be 55 bytes large.

withoutboats commented Oct 2, 2016

Regardless of whether Result<(),()> is considered something to avoid or not, which can be surely discussed about, I only wanted to measure what the additional overhead was of the error-chain error.

OK but you've used a type which has 0 bytes of data for the non-discriminant portion. All you've documented is that the largest variant of the null error chain is 55 bytes. That doesn't mean that the equivalent of io::Result is 79 bytes, it might still be 55 bytes large.

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 17, 2016

Collaborator

Not really an issue, closing.

Collaborator

Yamakaky commented Nov 17, 2016

Not really an issue, closing.

@Yamakaky Yamakaky closed this Nov 17, 2016

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Nov 17, 2016

This is a pretty major issue of this crate, as its against the "performance speed ease of use pick three" motto of Rust, by improving ease of use by heavily impairing performance.

est31 commented Nov 17, 2016

This is a pretty major issue of this crate, as its against the "performance speed ease of use pick three" motto of Rust, by improving ease of use by heavily impairing performance.

@Yamakaky Yamakaky reopened this Nov 19, 2016

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 19, 2016

Collaborator

The solution would be to make the Msg variant optional. Any idea about the macro syntax?

Collaborator

Yamakaky commented Nov 19, 2016

The solution would be to make the Msg variant optional. Any idea about the macro syntax?

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky
Collaborator

Yamakaky commented Nov 19, 2016

@colin-kiegel

This comment has been minimized.

Show comment
Hide comment
@colin-kiegel

colin-kiegel Nov 22, 2016

If we assume that errors are less likely than the happy path, we can optimize the memory footprint for the happy path if we heap-allocate most of the error payload.

If we look at Result<T, E>, then T might be as small as (). So it might be desirable to get E as small as a pointer, i.e. boxing everything E = Box<SomeError> without erasing the type information (i.e. no trait object). In situations where errors are unlikely, the error-path may be expensive as long as the happy-path is optimized.

But on the other hand there may be situations where such heap allocations are not desirable, e.g. (i) when errors happen frequently or (ii) when compiling with #[no_std] and heap allocations are not possible or (iii) error handling in oom situations.

So maybe error_chain could offer different memory layouts which implement the same interfaces and are therefore completely exchangeable (when looked at from the outside).

colin-kiegel commented Nov 22, 2016

If we assume that errors are less likely than the happy path, we can optimize the memory footprint for the happy path if we heap-allocate most of the error payload.

If we look at Result<T, E>, then T might be as small as (). So it might be desirable to get E as small as a pointer, i.e. boxing everything E = Box<SomeError> without erasing the type information (i.e. no trait object). In situations where errors are unlikely, the error-path may be expensive as long as the happy-path is optimized.

But on the other hand there may be situations where such heap allocations are not desirable, e.g. (i) when errors happen frequently or (ii) when compiling with #[no_std] and heap allocations are not possible or (iii) error handling in oom situations.

So maybe error_chain could offer different memory layouts which implement the same interfaces and are therefore completely exchangeable (when looked at from the outside).

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 22, 2016

Collaborator

Hum, interesting. Any idea for a the syntax?

Collaborator

Yamakaky commented Nov 22, 2016

Hum, interesting. Any idea for a the syntax?

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Nov 22, 2016

I think the first question is: do you want this behavior to be controlled by syntax in the macro invocation, or by a crate feature? A crate feature would be easier to implement & IMO probably easier to use (less new syntax to learn), but wouldn't allow you to scope the behavior to an individual invocation. I'm not sure how often people are defining multiple error_chains in their crate, much less they have a need to different performance properties between them.

withoutboats commented Nov 22, 2016

I think the first question is: do you want this behavior to be controlled by syntax in the macro invocation, or by a crate feature? A crate feature would be easier to implement & IMO probably easier to use (less new syntax to learn), but wouldn't allow you to scope the behavior to an individual invocation. I'm not sure how often people are defining multiple error_chains in their crate, much less they have a need to different performance properties between them.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Nov 22, 2016

A crate feature would be enough for me, too. Anything that makes the memory footprint smaller :)

est31 commented Nov 22, 2016

A crate feature would be enough for me, too. Anything that makes the memory footprint smaller :)

@colin-kiegel

This comment has been minimized.

Show comment
Hide comment
@colin-kiegel

colin-kiegel Nov 22, 2016

I think you could start with a crate feature. That should be sufficient in most cases. A syntax could still be introduced later if there is demand and the crate feature would then only set the default behaviour.

colin-kiegel commented Nov 22, 2016

I think you could start with a crate feature. That should be sufficient in most cases. A syntax could still be introduced later if there is demand and the crate feature would then only set the default behaviour.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Nov 22, 2016

Someone correct me if we don't guarantee this yet, but I think if you box the error, Result<()>s will be subject to the NPO, and be 1 pointer in size. The Ok case will be represented by a null pointer. This seems pretty 👍.

withoutboats commented Nov 22, 2016

Someone correct me if we don't guarantee this yet, but I think if you box the error, Result<()>s will be subject to the NPO, and be 1 pointer in size. The Ok case will be represented by a null pointer. This seems pretty 👍.

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 22, 2016

Collaborator

I started the implementation with the feature, but I get errors like Box<Error>: std::convert::From<std::io::Error> not satisfied. I guess I have to use a tuple struct?

Collaborator

Yamakaky commented Nov 22, 2016

I started the implementation with the feature, but I get errors like Box<Error>: std::convert::From<std::io::Error> not satisfied. I guess I have to use a tuple struct?

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Nov 22, 2016

@Yamakaky Have you tried implementing From<E: std::error::Error> for Box<Error>? Box is exempted from some of the normal orphan rules and so it might work. (Seems to work in playpen: https://is.gd/fjbjZS)

Edit: Looking at all the impls for the error type, you probably will need a tuple struct, because some of them like Deref for example will conflict with std impls.

withoutboats commented Nov 22, 2016

@Yamakaky Have you tried implementing From<E: std::error::Error> for Box<Error>? Box is exempted from some of the normal orphan rules and so it might work. (Seems to work in playpen: https://is.gd/fjbjZS)

Edit: Looking at all the impls for the error type, you probably will need a tuple struct, because some of them like Deref for example will conflict with std impls.

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 22, 2016

Collaborator

Oh, cool!

Collaborator

Yamakaky commented Nov 22, 2016

Oh, cool!

Yamakaky added a commit that referenced this issue Nov 22, 2016

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 22, 2016

Collaborator

Please review #69

Collaborator

Yamakaky commented Nov 22, 2016

Please review #69

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 23, 2016

Collaborator
Result<()>: 8
  (): 0
  Error: 8
Collaborator

Yamakaky commented Nov 23, 2016

Result<()>: 8
  (): 0
  Error: 8
@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Nov 23, 2016

Awesome!

est31 commented Nov 23, 2016

Awesome!

Yamakaky added a commit that referenced this issue Nov 23, 2016

Yamakaky added a commit that referenced this issue Nov 23, 2016

@brson brson referenced this issue Jul 24, 2017

Open

Resolve issues from libz blitz crate evaluation #190

11 of 25 tasks complete

@Yamakaky Yamakaky added this to the 1.0 milestone Jul 25, 2017

@cgm616

This comment has been minimized.

Show comment
Hide comment
@cgm616

cgm616 Nov 4, 2017

Is there still work to be done on this issue?

cgm616 commented Nov 4, 2017

Is there still work to be done on this issue?

@willir

This comment has been minimized.

Show comment
Hide comment
@willir

willir Jan 25, 2018

Was this feature implemented? If not, why it was abandoned?

willir commented Jan 25, 2018

Was this feature implemented? If not, why it was abandoned?

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Jan 25, 2018

Collaborator

Honestly, I don't really work on a Rust project anymore, so I haven't been working on error-chain for some time...

Collaborator

Yamakaky commented Jan 25, 2018

Honestly, I don't really work on a Rust project anymore, so I haven't been working on error-chain for some time...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment