Skip to content
This repository has been archived by the owner on Jul 10, 2023. It is now read-only.

all tests are failing #74

Closed
ignatenkobrain opened this issue Feb 27, 2017 · 12 comments
Closed

all tests are failing #74

ignatenkobrain opened this issue Feb 27, 2017 · 12 comments

Comments

@ignatenkobrain
Copy link

running 2 tests
test test_boxed_slice ... FAILED
test test_heap_size ... FAILED

failures:

---- test_boxed_slice stdout ----
	thread 'test_boxed_slice' panicked at 'assertion failed: `(left == right)` (left: `24`, right: `16`)', tests/tests.rs:170
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- test_heap_size stdout ----
	thread 'test_heap_size' panicked at 'assertion failed: `(left == right)` (left: `24`, right: `8`)', tests/tests.rs:126


failures:
    test_boxed_slice
    test_heap_size

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured

Not sure which information I should include as well.

@SimonSapin
Copy link
Member

What platform is this on? I can’t reproduce on Linux x86_64.

@ignatenkobrain
Copy link
Author

Fedora Rawhide (i.e. Linux x86_64).

@SimonSapin
Copy link
Member

How did you get Rust? Do you know if your Rust standard library is using jemalloc?

@ignatenkobrain
Copy link
Author

@SimonSapin dnf install rust and no, we don't use jemalloc.

@SimonSapin
Copy link
Member

This crate links against the fn malloc_usable_size(ptr: *const c_void) -> usize symbol and uses it to ask the allocator how much space is really used for a given allocation. The test assumes that when 8 or 16 bytes were requested, the usable size is the same as the requested size, which is what jemalloc does. It looks like your system allocator rounds small allocations up to 24 bytes.

The only way I can imagine removing the test’s assumption about allocation strategies is replacing e.g. result == 16 with result >= 16, but that also makes the tests much less useful.

Is it important that these tests pass for you? I’m tempted to remain with "tests assume jemalloc".

By the way, is "we" here the Fedora package maintainers for rustc? Why make a choice different from upstream about jemalloc?

@ignatenkobrain
Copy link
Author

@SimonSapin "we" == "Rust SIG" which includes package maintainers for rustc.

If you are interested about some history:

(10:51:26 PM) jistone: luke_nukem, some people complained that I disabled rust jemalloc, but I think it's the right call
(10:51:56 PM) King_InuYasha: imo, you should not be using jemalloc unless there's a very good reason on Linux
(10:52:02 PM) luke_nukem: jistone: I can't really find any clear-cut case for or against it tbh
(10:52:13 PM) jistone: they'll probably ditch it upstream once custom-alloc is stable
(10:52:39 PM) luke_nukem: jistone: That seems likely. Unless they think having a default fallback is needed
(10:52:41 PM) jistone: https:/rust-lang/rust/issues/36963
(10:53:02 PM) luke_nukem: Oh, cool
(10:54:21 PM) King_InuYasha: luke_nukem: like I said, there's not a good reason to use jemalloc on Linux
(10:54:40 PM) King_InuYasha: jemalloc is really only needed if you're on a deficient platform (like something not using glibc or msvcrt)
(10:54:42 PM) jistone: King_InuYasha, there are *some* workloads that do better with jemalloc
(10:54:50 PM) jistone: I just don't think it's a good default choice

About importance of tests... We do run cargo test during packaging and I would be happy if it would work. But I can easily add || : to ignore the real outcome.

@SimonSapin
Copy link
Member

(10:52:41 PM) jistone: rust-lang/rust#36963

For what it’s worth, the PR implementing this was closed for the moment: rust-lang/rust#38820 (comment)

Are you packaging this crate? Can you have package-specific patches? What do you think of having const ASSUME_JEMALLOC: bool = true; in the tests that you could patch to false?

@ignatenkobrain
Copy link
Author

ignatenkobrain commented Mar 30, 2017

Are you packaging this crate?

Yep.

Can you have package-specific patches?

Sure!

What do you think of having const ASSUME_JEMALLOC: bool = true; in the tests that you could patch to false?

Would be nice to have it like that.

SimonSapin added a commit that referenced this issue Mar 30, 2017
Fix #74

Distributions packages can maintain a local patch
that sets ASSUME_JEMALLOC to false.
@SimonSapin
Copy link
Member

How does #79 look?

@nox
Copy link
Contributor

nox commented Mar 30, 2017

@ignatenkobrain Can you pass arbitrary --cfg arguments to crates?

@ignatenkobrain
Copy link
Author

@nox definitely

@ignatenkobrain
Copy link
Author

(if cargo can pass it)

SimonSapin added a commit that referenced this issue Mar 30, 2017
Distributions packages can use `cargo test --features flexible-tests`

Fix #74
bors-servo pushed a commit that referenced this issue Mar 30, 2017
Add opt in for weaker tests, for non-jemalloc allocators.

Fix #74

Distributions packages can use `cargo test --features flexible-tests`

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/heapsize/79)
<!-- Reviewable:end -->
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants