std: make HEAP initializer never inline

[Berrysoft]

The system allocator for Windows calls init_or_get_process_heap every time allocating. It generates very much useless code and makes the binary larger. The HEAP only needs to initialize once before the main fn.

Concerns:

• I'm not sure if init will be properly called in cdylib.
• Do we need to ensure the allocator works if the user enables no_main?
• Should we panic if GetProcessHeap returns null?

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ChrisDenton (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[ChrisDenton]

init is not guaranteed to run as you say. For example cdylibs, staticlib linked with a C++ main, etc. I think a better approach might be to put the null case in a function marked as #[cold] and see if that helps to produce better codegen (i.e. less duplication). Another approach would be to always call GetProcessHeap if it doesn't significantly impact performance.

We can use init as an optimization but we can't rely on it.

[Berrysoft]

Another approach would be to always call GetProcessHeap if it doesn't significantly impact performance.

That's the original implementation before adding HEAP static variable:)

init is not guaranteed to run as you say.

I've noticed another init method here:

rust/library/std/src/sys/pal/windows/compat.rs

Line 55 in a58ec8f

unsafe extern "C" fn init() {

And it seems to run before main in exe and DllMain in dll. I'll try to initialize HEAP in a similar way.

[ChrisDenton]

While that is certainly a lot better, I'm still very nervous about this change.

Allocation is absolutely fundamental to libstd (as opposed to libcore); it's unusable without it. Having no fallback at all makes this risky should it not get initialized. It's also been a problem in the past that users can run their own code before our pre-main code. We now try to mitigate against that but it's only a "best effort" attempt.

[Berrysoft]

Having no fallback at all makes this risky should it not get initialized.

Now I place the initializer in .CRT$XCB. It will execute before any other initializers. In the mod compat the comments say

rust/library/std/src/sys/pal/windows/compat.rs

Lines 36 to 39 in a58ec8f

	// NOTE: User code should instead use .CRT$XCU to reliably run after std's initializer.
	// If you're reading this and would like a guarantee here, please
	// file an issue for discussion; currently we don't guarantee any functionality
	// before main.

I think XCB is fairly enough for the users even if they want to put their initializers before XCT. Anyway we didn't and don't guarantee any functionality before main.

[ChrisDenton]

Anyway we didn't and don't guarantee any functionality before main.

That was recently clarified: https://doc.rust-lang.org/std/#use-before-and-after-main

While there aren't any strict guarantees, we do attempt to make sure everything works pre-main unless platform limitations mean it's not possible.

[ChrisDenton]

I think I'll need to sleep on this one. My thoughts at the moment are:

• I do recognise the issue and that this PR will improve the situation, even if I'm wary of relying on init.
• I would first like to explore other ways to solve the issue (e.g. refactoring so it doesn't generate useless code). After all, there isn't an issue with performance here only code size. Maybe we can do something about that in another way.
• If there is not another way then I am leaning towards accepting this.

[Berrysoft]

Another possible solution, is to move GetProcessHeap call to a cold, never inline function. Would you like that one?

[ChrisDenton]

Yes, I think it'd be easier for me to accept something like that.

Another way might be to wrap the call to HeapAlloc with an inline(never) function that both gets the heap and then calls HeapAlloc. After all, I don't think there's a reason to get the heap so early.

[Berrysoft]

OK. I just marked GetProcessHeap cold and inline(never).

[ChrisDenton]

Looks good to me

@bors r+ rollup

[bors]

📌 Commit 936e0fd has been approved by ChrisDenton

It is now in the queue for this repository.

[klensy]

It generates very much useless code and makes the binary larger.

What the diff with PR?

[Berrysoft]

What the diff with PR?

Just now, I've noticed that in some cases this PR will not shrink the binary size. I'll turn the PR to draft and may try some other approaches.

[mati865]

@bors r-

[bors]

@mati865: 🔑 Insufficient privileges: Not in reviewers

[ChrisDenton]

@bors r-

[ChrisDenton]

My suggestion would be to try something like:

fn process_heap_alloc(flags: c::DWORD, dwBytes: c::SIZE_T) -> c::LPVOID {
    let heap = init_or_get_process_heap();
    if heap.is_null() {
        return ptr::null_mut();
    }
    unsafe { HeapAlloc(heap, flags, dwBytes) }
}

And call process_heap_alloc instead of HeapAlloc everywhere else. That way the alignment check can still be inlined (and removed, if statically known) without generating more code for getting the process heap.

[Berrysoft]

My suggestion would be to try something like...

It shrinks the binary size of course, but I'm a little afraid that it introduces a longer calling chain to call HeapAlloc, which may brings some performance regression...

[ChrisDenton]

I'd be surprised if it did but would be open to being wrong. HeapAlloc is not such a fast function that I'd expect that it'd make a difference. And HeapAlloc is definitely one of the slower allocators, especially in multi-threaded code (where allocation is synchronized).

[Berrysoft]

OK, I finally wrapped HeapAlloc and made the binary smaller. For each HeapAlloc call, the binary will be about 30 bytes smaller (I'm using GNU toolchain, so this 30 bytes also includes the debug symbol). Didn't test the performance.

P.S. Comparing stripped binary, for each HeapAlloc call, the binary will be about 25 bytes smaller. I tested with LTO and build-std enabled.

[ChrisDenton]

From my personal testing this does not appear to affect performance in a way I can discern from noise. So I'll r+ this. In the worse case, it can always be reverted if people do find it does affects them.

@bors r+

[bors]

📌 Commit 27a6e6e has been approved by ChrisDenton

It is now in the queue for this repository.