Skip to content

Replace cmake with cc #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 6, 2021
Merged

Replace cmake with cc #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d4f2ec
Replace cmake with cc
Jake-Shadle Feb 12, 2021
098f752
Cleanup README to bump CI
Jake-Shadle Feb 12, 2021
4f873cb
oops
Jake-Shadle Feb 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,18 @@ static GLOBAL: MiMalloc = MiMalloc;

## Requirements

[CMake](https://cmake.org/) and a __C__ compiler are required for building
[mimalloc](https://github.com/microsoft/mimalloc) with cargo.
A __C__ compiler is required for building [mimalloc](https://github.com/microsoft/mimalloc) with cargo.

## Usage without secure mode

By default this library builds mimalloc in secure mode. This add guard pages,
By default this library builds mimalloc in secure mode. This add guard pages,
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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```ini
```toml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[dependencies]
mimalloc = { version = "*", default-features = false }
```
Expand Down
2 changes: 1 addition & 1 deletion libmimalloc-sys/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ links = "mimalloc"
cty = { version = "0.2", optional = true }

[build-dependencies]
cmake = "0.1"
cc = "1.0"

[features]
secure = []
Expand Down
198 changes: 47 additions & 151 deletions libmimalloc-sys/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,168 +1,64 @@
#![allow(clippy::collapsible_if)]
use cmake::Config;
use std::env;

enum CMakeBuildType {
Debug,
Release,
RelWithDebInfo,
MinSizeRel,
}

/// Determine the CMake build type that will be picked by `cmake-rs`.
///
/// This is mostly pasted from `cmake-rs`:
/// https://github.com/alexcrichton/cmake-rs/blob/7f85e323/src/lib.rs#L498
fn get_cmake_build_type() -> Result<CMakeBuildType, String> {
fn getenv(v: &str) -> Result<String, String> {
env::var(v).map_err(|_| format!("environment variable `{}` not defined", v))
}

// Determine Rust's profile, optimization level, and debug info:
#[derive(PartialEq)]
enum RustProfile {
Debug,
Release,
}
#[derive(PartialEq, Debug)]
enum OptLevel {
Debug,
Release,
Size,
}

let rust_profile = match getenv("PROFILE")?.as_str() {
"debug" => RustProfile::Debug,
"release" | "bench" => RustProfile::Release,
_ => RustProfile::Release,
};

let opt_level = match getenv("OPT_LEVEL")?.as_str() {
"0" => OptLevel::Debug,
"1" | "2" | "3" => OptLevel::Release,
"s" | "z" => OptLevel::Size,
_ => match rust_profile {
RustProfile::Debug => OptLevel::Debug,
RustProfile::Release => OptLevel::Release,
},
};

let debug_info: bool = match getenv("DEBUG")?.as_str() {
"false" => false,
"true" => true,
_ => true,
};

Ok(match (opt_level, debug_info) {
(OptLevel::Debug, _) => CMakeBuildType::Debug,
(OptLevel::Release, false) => CMakeBuildType::Release,
(OptLevel::Release, true) => CMakeBuildType::RelWithDebInfo,
(OptLevel::Size, _) => CMakeBuildType::MinSizeRel,
})
}

fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let mut cfg = &mut Config::new("c_src/mimalloc");
let mut build = cc::Build::new();

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)),
);
Comment on lines +6 to +26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Faster builds and links, only one object file produced, all headers can only be parsed once.
  2. 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)
  3. More robust against changes to the mimalloc C source, as we just have to worry about one .c file.
  4. 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.


build.define("MI_STATIC_LIB", None);

let target_os = env::var("CARGO_CFG_TARGET_OS").expect("target_os not defined!");

if cfg!(feature = "override") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Actually, probably best to address that separately)

cfg = cfg.define("MI_OVERRIDE", "ON");
} else {
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" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should check the target family, not target_os

build.define("MI_MALLOC_OVERRIDE", None);
}
}

cfg = cfg.define("MI_BUILD_TESTS", "OFF");

if cfg!(feature = "secure") {
cfg = cfg.define("MI_SECURE", "ON");
} else {
cfg = cfg.define("MI_SECURE", "OFF");
build.define("MI_SECURE", "4");
}

if cfg!(feature = "local_dynamic_tls") {
cfg = cfg.define("MI_LOCAL_DYNAMIC_TLS", "ON");
} else {
cfg = cfg.define("MI_LOCAL_DYNAMIC_TLS", "OFF");
}

// Inject MI_DEBUG=0
// This set mi_option_verbose and mi_option_show_errors options to false.
cfg = cfg.define("mi_defines", "MI_DEBUG=0");

let (is_debug, win_folder) = match get_cmake_build_type() {
Ok(CMakeBuildType::Debug) => (true, "Debug"),
Ok(CMakeBuildType::Release) => (false, "Release"),
Ok(CMakeBuildType::RelWithDebInfo) => (false, "RelWithDebInfo"),
Ok(CMakeBuildType::MinSizeRel) => (false, "MinSizeRel"),
Err(e) => panic!("Cannot determine CMake build type: {}", e),
};

// Turning off shared lib, tests, PADDING.
// See also: https://github.com/microsoft/mimalloc/blob/master/CMakeLists.txt
cfg = cfg.define("MI_PADDING", "OFF");
cfg = cfg.define("MI_BUILD_TESTS", "OFF");
cfg = cfg.define("MI_BUILD_SHARED", "OFF");
cfg = cfg.define("MI_BUILD_OBJECT", "OFF");
let dynamic_tls = cfg!(feature = "local_dynamic_tls");

let target_env = env::var("CARGO_CFG_TARGET_ENV").unwrap();
if target_env == "msvc" {
cfg = cfg.define("CMAKE_SH", "CMAKE_SH-NOTFOUND");

// cc::get_compiler have /nologo /MD default flags that are cmake::Config
// defaults to. Those flags prevents mimalloc from building on windows
// extracted from default cmake configuration on windows
if is_debug {
// CMAKE_C_FLAGS + CMAKE_C_FLAGS_DEBUG
cfg = cfg.cflag("/DWIN32 /D_WINDOWS /W3 /MDd /Zi /Ob0 /Od /RTC1");
} else {
// CMAKE_C_FLAGS + CMAKE_C_FLAGS_RELEASE
cfg = cfg.cflag("/DWIN32 /D_WINDOWS /W3 /MD /O2 /Ob2 /DNDEBUG");
}
} else if target_env == "gnu" {
cfg = cfg.define("CMAKE_SH", "CMAKE_SH-NOTFOUND");
// Those flags prevents mimalloc from building on windows
if is_debug {
// CMAKE_C_FLAGS + CMAKE_C_FLAGS_DEBUG
cfg = cfg.cflag("-ffunction-sections -fdata-sections -m64 -O2 -fpic");
if env::var("CARGO_CFG_TARGET_FAMILY").expect("target family not set") == "unix"
&& target_os != "haiku"
{
if dynamic_tls {
build.flag_if_supported("-ftls-model=local-dynamic");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
// CMAKE_C_FLAGS + CMAKE_C_FLAGS_RELEASE
cfg = cfg.cflag("-ffunction-sections -fdata-sections -m64 -O3 -fpic");
build.flag_if_supported("-ftls-model=initial-exec");
}
};
}

let mut out_dir = "./build".to_string();
if cfg!(all(windows)) {
if target_env == "msvc" {
out_dir.push('/');
out_dir.push_str(win_folder);
}
// Remove heavy debug assertions etc
build.define("MI_DEBUG", "0");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if build.get_compiler().is_like_msvc() {
build.cpp(true);
}
let out_name = if cfg!(all(windows)) {
if is_debug {
if cfg!(feature = "secure") {
"mimalloc-static-secure-debug"
} else {
"mimalloc-static-debug"
}
} else if cfg!(feature = "secure") {
"mimalloc-static-secure"
} else {
"mimalloc-static"
}
} else if is_debug {
if cfg!(feature = "secure") {
"mimalloc-secure-debug"
} else {
"mimalloc-debug"
}
} else if cfg!(feature = "secure") {
"mimalloc-secure"
} else {
"mimalloc"
};

// Build mimalloc-static
let mut dst = cfg.build_target("mimalloc-static").build();
dst.push(out_dir);
println!("cargo:rustc-link-search=native={}", dst.display());
println!("cargo:rustc-link-lib={}", out_name);
build.compile("mimalloc");
}