DoumanAsh / xxhash-rust

Rust implementation of xxhash
Boost Software License 1.0
203 stars 20 forks source link

Drop half of the tests in test-vectors.rs #19

Closed dkg closed 2 years ago

dkg commented 2 years ago

test-vectors.rs consumes a lot of RAM. When compiling it, in /proc/$(pidof rustc)/status on an amd64 machine, I saw:

VmPeak: 6484308 kB

This causes failures on 32-bit platforms, which don't have the address space for such a large memory set.

By dropping the latter half of these tests, I ended up with less RAM pressure from rustc:

VmPeak: 2409188 kB

i've left the final test just to confirm its value, but i think the semantic value of the shorter test nearly equivalent.

Fixes: #16

DoumanAsh commented 2 years ago

Does allocator delays freeing memory in your case? It sounds weird that it doesn't free memory immediately. In any case the better approach would be to just avoid allocations all together, it can be achieved quite trivially with some static buffer

dkg commented 2 years ago

I have no idea what rustc is doing in terms of memory allocator disposal when it compiles test-vector.rs. How would i tell?

If you've got a better suggestion to reduce the amount of RAM used by rustc when compiling test-vectors.rs, i'm open to it!

I wasn't particularly sure about the cause of the memory overload, so i also tried moving the manifesto out into a separate file over on my break-out-manifesto branch -- but that didn't change anything related to PeakVM during compilation -- still over 6GiB. But the variant in this PR does reduce the PeakVM, so i can only assume that rustc is allocating so much RAM because of all the assert_eq! macros happening in one single function.

I don't know enough about rust internals to be able to tell what the underlying issue is here. my tests are being done on amd64 with the rustc debian pacakge of 1.58.1+dfsg1-1.

To be clear, I'm triggering the test with:

cargo test --no-default-features --features xxh3,const_xxh3 --verbose
DoumanAsh commented 2 years ago

Yeah I get it now compiler seems to be rather pathetic when it comes to optimizing own memory usage, probably because it is optimized for speed at expense of everything else. I'll re-write test to not rely on so many literal strings

dkg commented 2 years ago

Thanks for taking a look, @DoumanAsh ! I've just uploaded 0.8.5-2 to debian unstable with the patch from this PR in it. I'll try to keep an eye on the debian ci and report back here when i know whether the 32-bit platforms succeed.

DoumanAsh commented 2 years ago

@dkg You can try this PR https://github.com/DoumanAsh/xxhash-rust/pull/20

I moved all literals into separate files so that compiler wouldn't load shit tons of string into memory

DoumanAsh commented 2 years ago

Closed in favor of https://github.com/DoumanAsh/xxhash-rust/pull/20