Skip to content
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.

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

Switch to direct HeapAlloc on Windows when not using jemalloc #26265

Merged
merged 1 commit into from
Jun 15, 2015

Conversation

retep998
Copy link
Member

This removes our dependency on the CRT for memory allocation.

@rust-highfive
Copy link
Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -395,6 +425,12 @@ mod imp {
}

pub fn stats_print() {}
Copy link
Member Author

Choose a reason for hiding this comment

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

I would provide a really neat implementation using HeapSummary, but I don't know of an easy way to print things from inside this crate.

Copy link
Member

Choose a reason for hiding this comment

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

Is there no easy way to write to stdout on windows without the CRT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I could directly WriteConsole to GetStdHandle(STD_OUTPUT_HANDLE) but then I'd have to format numbers into strings without std.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to leave this as-is, I don't think it's really worth adding an implementation unless it's easy (this is very rarely called)

@petrochenkov
Copy link
Contributor

Did you consulted with the implementations of malloc/free/realloc in crt\src\{malloc, free, realloc}.c?
They seem to perform some additional checks and actions (besides calling hooks) comparing to this implementation, especially realloc.

@nagisa
Copy link
Member

nagisa commented Jun 13, 2015

They seem to perform some additional checks and actions

Looks mostly like optimisations, backward compatibility, crt specifics, and locking (in case NO_SERIALIZE heap is misused?) code. Since this code currently can only be used by the standard library, I feel its fine to go for the simple approach.

@alexcrichton
Copy link
Member

Thanks @retep998! It's my impression that the CRT malloc functions and friends just call these underlying functions, right? I looked around and didn't really find a definitive answer on why we'd want to favor these over malloc, and having to deal with alignment ourselves is always a pain.

Also, could you elaborate a little more on why we want to eliminate the dependency on the CRT? It sounds like issues like #26258 mean that it'd be nice to not have to deal with it altogether, but I just want to make sure we've got a good picture of what's going on here.

@retep998
Copy link
Member Author

malloc and friends do simply call HeapAlloc. _aligned_malloc does the exact same trick as I do here with overallocating and embedding a pointer to the original directly before the aligned address. The CRT versions do handle errors through _errno though, and having it in Rust also means the function can be inlined to give a slight performance advantage.

I want to eliminate the dependency on the CRT so that Rust can be independent of it. Such a Rust library could be used anywhere without concerns over the ABI of the C runtime. A binary built without the CRT and having its own entry point wouldn't depend on the CRT redistributables and would have a quicker startup time.


#[inline]
unsafe fn align_ptr(ptr: *mut u8, align: usize) -> *mut u8 {
let aligned = ptr.offset((align - (ptr as usize & align - 1)) as isize);
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some parens to the align - 1 just to make the binding explicit? (I can never remember the precedence of - vs &

@alexcrichton
Copy link
Member

Sounds good to me! r=me with just a few nits

Signed-off-by: Peter Atashian <retep998@gmail.com>
@retep998
Copy link
Member Author

Nits addressed and an implementation of stats_print added.

@klutzy
Copy link
Contributor

klutzy commented Jun 14, 2015

cc #20861

@alexcrichton
Copy link
Member

@bors: r+ ebbd90d

@bors
Copy link
Contributor

bors commented Jun 15, 2015

⌛ Testing commit ebbd90d with merge 8937ec1...

bors added a commit that referenced this pull request Jun 15, 2015
This removes our dependency on the CRT for memory allocation.
@bors bors merged commit ebbd90d into rust-lang:master Jun 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants