Replace cmake with cc #58

Jake-Shadle · 2021-02-12T10:43:35Z

This PR replaces the usage of cmake with just straight usage of cc. It replicates some of the logic in the cmakelists but is simplified since this crate only builds a static lib.

Resolves: #51

maroider · 2021-02-12T12:15:19Z

README.md

 randomized allocation, encrypted free lists, etc. The performance penalty is usually
 around 10% according to [mimalloc](https://github.com/microsoft/mimalloc)
 own benchmarks.

 To disable secure mode, put in `Cargo.toml`:
-```rust
+
+```ini


Suggested change

```ini

```toml

This was actually intentional, some markdown renderers don't actually handle the toml code fence, whereas ini is usually recognized and tends to be highlighted correctly, at least for simpler code blocks such as this, but I can of course change it to be toml.

Huh, TIL. I have no stake in this either way. It just struck me as odd.

repi · 2021-02-21T13:35:37Z

Possible to get this merged in and publish a new release? Thanks!

repi · 2021-03-04T21:22:07Z

@octavonce any thoughts on getting this in? think it would be beneficial to most and we would be really happy getting rid of our fork

thomcc

So, I was working on this at one point and got too busy to continue (and forgot), so I'm thrilled to see it picked up. The version I had was more complex since it kept the cmake support under a flag, but I think this is better.

That said, there are a few nits I have.

The most important one is probably mimicing MI_BUILD_OBJECT rather than MI_BUILD_STATIC, since it has many upsides and basically no downsides compared to what we're doing here.

thomcc · 2021-03-04T22:29:54Z

libmimalloc-sys/build.rs

+    build.include("c_src/mimalloc/include");
+    build.files(
+        [
+            "alloc-aligned",
+            "alloc-posix",
+            "alloc",
+            "arena",
+            "bitmap",
+            "heap",
+            "init",
+            "options",
+            "os",
+            "page",
+            "random",
+            "region",
+            "segment",
+            "stats",
+        ]
+        .iter()
+        .map(|fname| format!("c_src/mimalloc/src/{}.c", fname)),
+    );


Suggested change

build.include("c_src/mimalloc/include");

build.files(

[

"alloc-aligned",

"alloc-posix",

"alloc",

"arena",

"bitmap",

"heap",

"init",

"options",

"os",

"page",

"random",

"region",

"segment",

"stats",

]

.iter()

.map(|fname| format!("c_src/mimalloc/src/{}.c", fname)),

);

build.include("c_src/mimalloc/include");

build.include("c_src/mimalloc/src");

build.file("c_src/mimalloc/src/static.c");

We should just compile https://github.com/microsoft/mimalloc/blob/master/src/static.c as a single compilation unit build. This mimics the MI_BUILD_OBJECT setting, not the MI_BUILD_STATIC setting, but the cc crate will produce a static lib from it anyway.

This has several benefits:

Faster builds and links, only one object file produced, all headers can only be parsed once.

Faster at runtime, compiler can see the whole source and inline across .o files (note that C code (unless you set up cross-lang LTO which is very nontrivial) doesn't benefit from rust's LTO, so this is very nice)

More robust against changes to the mimalloc C source, as we just have to worry about one .c file.

When static overriding is used, this will be more robust on some unix platforms, which is the reason MI_BUILD_OBJECT exists. While we're still using a .a file, and not a .o file, the way Rust code is linked means we still benefit here.

thomcc · 2021-03-04T22:42:24Z

libmimalloc-sys/build.rs

+
+    build.define("MI_STATIC_LIB", None);
+
+    let target_os = env::var("CARGO_CFG_TARGET_OS").expect("target_os not defined!");

    if cfg!(feature = "override") {


Nit: prefer std::env::var("CARGO_FEATURE_FOO").is_ok() to cfg!(feature = "foo") in build scripts, since using the latter requires the build script to be recompiled and not just rerun when features change. (not a huge deal, but no reason not to do this)

See https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts

(Thanks for doing this for CARGO_CFG_TARGET_* too! It's more important for those admittedly)

thomcc · 2021-03-04T22:43:28Z

libmimalloc-sys/build.rs

+        && target_os != "haiku"
+    {
+        if dynamic_tls {
+            build.flag_if_supported("-ftls-model=local-dynamic");


Honestly, this is dubious since it goes against the settings rustc uses, but I'll file another issue about it.

thomcc · 2021-03-04T22:54:05Z

libmimalloc-sys/build.rs

-        (OptLevel::Size, _) => CMakeBuildType::MinSizeRel,
-    })
-}
-
 fn main() {


It would be nice to add

println!("cargo:rerun-if-changed=c_src/mimalloc");

here, so that when changing the code in the -sys crate it doesnt rebuild each time (by default it's any change in the folder containing the build.rs).

thomcc · 2021-03-04T22:56:59Z

libmimalloc-sys/build.rs

+
+    build.define("MI_STATIC_LIB", None);
+
+    let target_os = env::var("CARGO_CFG_TARGET_OS").expect("target_os not defined!");

    if cfg!(feature = "override") {


Also, we should consider disabling the override on at least macOS (probably other platforms too...) until #41 is resolved.

(Actually, probably best to address that separately)

thomcc · 2021-03-04T23:02:17Z

libmimalloc-sys/build.rs

-            out_dir.push_str(win_folder);
-        }
+    // Remove heavy debug assertions etc
+    build.define("MI_DEBUG", "0");


We should eventually have an option to turn this on. That said, we should set build.define("NDEBUG", None) here too if we're doing this. Note that cmake will do this automatically, but cc does not.

Hmm, while mimalloc does use NDEBUG, it's not for assertions and just for a couple extra things. I'll file an issue with mimalloc, what we have here is fine actually.

thomcc · 2021-03-04T23:02:55Z

libmimalloc-sys/build.rs

-        cfg = cfg.define("MI_OVERRIDE", "OFF");
+        // Overriding malloc is only available on windows in shared mode, but we
+        // only ever build a static lib.
+        if target_os != "windows" {


This should check the target family, not target_os

thomcc · 2021-03-04T23:48:18Z

Here's a (tested at least on my machines) patch containing all my suggestions that are actually worth doing in this PR: https://gist.github.com/thomcc/3a499f95c68ad8c8698f61235b1a0fa5

Jake-Shadle added 3 commits February 12, 2021 11:24

Replace cmake with cc

7d4f2ec

Cleanup README to bump CI

098f752

oops

4f873cb

Jake-Shadle mentioned this pull request Feb 12, 2021

Embark's pull requests EmbarkStudios/rust-ecosystem#20

Open

26 tasks

maroider reviewed Feb 12, 2021

View reviewed changes

thomcc suggested changes Mar 4, 2021

View reviewed changes

This was referenced Mar 4, 2021

Fails to build on ARM64 (c6g AWS instance) #50

Open

Aarch64 build #57

Closed

thomcc mentioned this pull request Mar 5, 2021

set needed libs for windows gnu #56

Closed

octavonce merged commit 594a902 into purpleprotocol:master Mar 6, 2021

thomcc added a commit to thomcc/mimalloc_rust that referenced this pull request Mar 6, 2021

Apply build script suggestions from purpleprotocol#58 (comment)

2f8256c

thomcc mentioned this pull request Mar 6, 2021

Build script cleanups, adding missing extended functions. #63

Merged

hrydgard deleted the remove-cmake branch March 8, 2021 07:47

lblasc mentioned this pull request Mar 22, 2021

Unable to build dylib with mimalloc as global_allocator nix-community/crate2nix#174

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace cmake with cc #58

Replace cmake with cc #58

Jake-Shadle commented Feb 12, 2021

maroider Feb 12, 2021

Jake-Shadle Feb 12, 2021

maroider Feb 12, 2021

repi commented Feb 21, 2021

repi commented Mar 4, 2021

thomcc left a comment

thomcc Mar 4, 2021

thomcc Mar 4, 2021

thomcc Mar 4, 2021

thomcc Mar 4, 2021

thomcc Mar 4, 2021

thomcc Mar 4, 2021

thomcc Mar 4, 2021

thomcc Mar 4, 2021

thomcc Mar 4, 2021

thomcc commented Mar 4, 2021 •

edited

Replace cmake with cc #58

Replace cmake with cc #58

Conversation

Jake-Shadle commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repi commented Feb 21, 2021

repi commented Mar 4, 2021

thomcc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomcc commented Mar 4, 2021 • edited

thomcc commented Mar 4, 2021 •

edited