Frommi / miniz_oxide

Rust replacement for miniz
MIT License
174 stars 49 forks source link

LZOxide too large for Windows #21

Closed mvdnes closed 6 years ago

mvdnes commented 6 years ago

On Windows, the following code produces a stack overflow:

extern crate miniz_oxide;

use miniz_oxide::deflate::core::CompressorOxide;

struct Foo {
    bar: CompressorOxide,
}

fn main() {
    let foo = Foo { bar: CompressorOxide::new(0), };
}

Specifically, I get this error: thread 'main' has overflowed its stack. This does not happen in release mode, however.

I think the structure LZOxide may be too large, as the overflow happens in it's new() funtion.

Perhaps it is a good idea to allocate this data on the heap?

oyvindln commented 6 years ago

Yea, there are some buffers which take up a fair bit of space. I believe the api used through flate2 actually does allocate on the heap, however without optimizations compiled code ends up with a lot of redundant stack copies, so it's easy to get overflows. Even with optimizations the current rustc/llvm does a bad job at getting rid of stack copies of more complex objects. This is a bit frustrating when dealing with statically sized buffers since if you use vec you lose the static length info which helps the compiler avoid bounds checks, but if you use boxed arrays you risk blowing the stack.

miniz_oxide is originally a port of miniz, which is written in C, and thus designed around the structures be allocated with malloc or similar, so having everything in one big struct makes sense rather than the internal buffers being analogous to Vec in rust (and also promises not doing any dynamic memory allocation internally). For rust this is not really ideal though, due to the mentioned issues, so trying to put stuff on the heap while trying to avoid stack copies seems reasonable.

mvdnes commented 6 years ago

In the latest master, this issue does not seem to be a problem any more. At least on Windows 10 x64 using

rustc 1.27.0 (3eda71b00 2018-06-19)
binary: rustc
commit-hash: 3eda71b00ad48d7bf4eef4c443e7f611fd061418
commit-date: 2018-06-19
host: x86_64-pc-windows-msvc
release: 1.27.0
LLVM version: 6.0

I get no more stack overflows using this rather in contrast to 0.1.2.

Could you publish a new version to crates.io?

oyvindln commented 6 years ago

Yeah, moved some stuff to the heap in f907ff7a536d652e00208fc869839c9ea166fc01. There are some other minor fixes in master too, so hopefully @Frommi can upload a new version soon.

Frommi commented 6 years ago

0.1.3 uploaded.

mvdnes commented 6 years ago

The testcase I provided works now, so I think this resolves it. Thanks!