Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Make tests pass on nightly again #9953

Closed
ordian opened this issue Nov 22, 2018 · 9 comments
Closed

Make tests pass on nightly again #9953

ordian opened this issue Nov 22, 2018 · 9 comments
Assignees
Labels
M5-dependencies 🖇 Dependencies. P5-sometimesoon 🌲 Issue is worth doing soon.
Milestone

Comments

@ordian
Copy link
Collaborator

ordian commented Nov 22, 2018

I guess the problem is that rust nightly no longer uses jemalloc and we're using heapsize crate, which assumes it does (cc rust-lang/rust/issues/56069). A temporary solution would be to use jemalloc as a global allocator (https://github.com/alexcrichton/jemallocator ?).

@ordian ordian added P5-sometimesoon 🌲 Issue is worth doing soon. M5-dependencies 🖇 Dependencies. labels Nov 22, 2018
@ordian ordian added this to the 2.3 milestone Nov 22, 2018
@ordian ordian changed the title Make nightly compile again Make tests pass on nightly again Nov 22, 2018
@ordian
Copy link
Collaborator Author

ordian commented Nov 22, 2018

This is worth doing soon, since this change will make its way into the rust stable eventually.
I remember @tomaka and @cheme had some thoughts on this.

@cheme
Copy link
Contributor

cheme commented Nov 22, 2018

For short term I can put jemalloc as default allocator with crate jemallocator, I currently need to fork heapsize with very few changes:

diff --git a/Cargo.toml b/Cargo.toml
index ec2747a..b3c2c0e 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -7,6 +7,10 @@ license = "MIT/Apache-2.0"
 repository = "https://github.com/servo/heapsize"
 build = "build.rs"
 
+
+[dependencies]
+jemallocator = "0.1.9"
+
 [target.'cfg(windows)'.dependencies]
 winapi = { version = "0.3.4", features = ["std", "heapapi"] }
 
diff --git a/src/lib.rs b/src/lib.rs
index 7eecbce..85e85e0 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -3,6 +3,13 @@
 #[cfg(target_os = "windows")]
 extern crate winapi;
 
+#[cfg(not(target_os = "windows"))]
+extern crate jemallocator;
+
+#[cfg(not(target_os = "windows"))]
+#[global_allocator]
+static ALLOC: jemallocator::Jemalloc = jemallocator::Jemalloc;
+
 #[cfg(target_os = "windows")]
 use winapi::um::heapapi::{GetProcessHeap, HeapSize, HeapValidate};
 use std::borrow::Cow;
@@ -36,15 +43,7 @@ pub unsafe fn heap_size_of<T>(ptr: *const T) -> usize {
 
 #[cfg(not(target_os = "windows"))]
 unsafe fn heap_size_of_impl(ptr: *const c_void) -> usize {
-    // The C prototype is `je_malloc_usable_size(JEMALLOC_USABLE_SIZE_CONST void *ptr)`. On some
-    // platforms `JEMALLOC_USABLE_SIZE_CONST` is `const` and on some it is empty. But in practice
-    // this function doesn't modify the contents of the block that `ptr` points to, so we use
-    // `*const c_void` here.
-    extern "C" {
-               #[cfg_attr(any(prefixed_jemalloc, target_os = "macos", target_os = "ios", target_os = "android"), link_name = "je_malloc_usable_size")]
-        fn malloc_usable_size(ptr: *const c_void) -> usize;
-    }
-    malloc_usable_size(ptr)
+    jemallocator::usable_size(ptr)
 }

I do not believe it will be a good long term solution (I do not really like the idea to get specific allocator things).

Maybe running more approximate allocation evalution (without querying the allocator).
Edit: Making functionalities requiring to heapsize optional (and bind heapsize use to jemalloc global alloc) could be another option. But I need to check what it implies (if it is even possible).

@tomaka
Copy link
Contributor

tomaka commented Nov 23, 2018

The good long term solution is to kill the heapsize crate.

@5chdn
Copy link
Contributor

5chdn commented Nov 30, 2018

This is worth doing soon, since this change will make its way into the rust stable eventually.

This is our window to address it:

  • 1.32 beta: Dec 6th
  • 1.32 stable: Jan 17th

Ideally, we have a fix before Christmas

@5chdn
Copy link
Contributor

5chdn commented Dec 5, 2018

not related but relevant? #9167 #10013

@ordian
Copy link
Collaborator Author

ordian commented Dec 5, 2018

not related but relevant? #9167 #10013

It's relevant in a sense that if/when we'll get rid of heapsize crate, it will resolve those issues as well.

@arkpar
Copy link
Collaborator

arkpar commented Dec 6, 2018

We certainly want to continue using jemalloc on supported platforms. It is much faster than the system allocator.

@cheme
Copy link
Contributor

cheme commented Dec 6, 2018

@arkpar that should not be a problem (I intend to allow setting global allocator on a PR I am currently working on).

Regarding the 10024 PR I believe the test limit does not matter that much and this change should be fine. (being able to switch to the default allocator might prove being useful in the future).

@5chdn 5chdn modified the milestones: 2.3, 2.4 Jan 10, 2019
@5chdn 5chdn modified the milestones: 2.4, 2.5 Feb 21, 2019
@soc1c soc1c modified the milestones: 2.5, 2.6 Apr 2, 2019
@ordian
Copy link
Collaborator Author

ordian commented Jun 19, 2019

closed by #10432 (heapsize part)

@ordian ordian closed this as completed Jun 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
M5-dependencies 🖇 Dependencies. P5-sometimesoon 🌲 Issue is worth doing soon.
Projects
None yet
Development

No branches or pull requests

6 participants