Parity-ethereum: Make tests pass on nightly again

Created on 22 Nov 2018  路  9Comments  路  Source: openethereum/parity-ethereum

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 ?).

M5-dependencies 馃枃 P5-sometimesoon 馃尣

Most helpful comment

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

All 9 comments

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.

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).

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

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

not related but relevant? #9167 #10013

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.

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

@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).

closed by #10432 (heapsize part)

Was this page helpful?
0 / 5 - 0 ratings

Related issues

mr-older picture mr-older  路  3Comments

stone212 picture stone212  路  3Comments

0x7CFE picture 0x7CFE  路  3Comments

BillSantos picture BillSantos  路  3Comments

jacogr picture jacogr  路  4Comments