Skip to content

Don't commit thread stack on Windows #52847

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

Merged
merged 1 commit into from
Aug 2, 2018
Merged

Don't commit thread stack on Windows #52847

merged 1 commit into from
Aug 2, 2018

Conversation

upsuper
Copy link
Contributor

@upsuper upsuper commented Jul 30, 2018

On Windows, there is a system level resource limitation called commit limit, which is roughly the sum of physical memory + paging files[1]. CreateThread by default commits the stack size[2], which unnecessarily takes such resource from the shared limit.

This PR changes it to only reserve the stack size rather than commit it. Reserved memory would only take the address space of the current process until it's actually accessed.

This should make the behavior on Windows match other platforms, and is also a pretty standard practice on Windows nowadays.

[1] https://blogs.technet.microsoft.com/markrussinovich/2008/11/17/pushing-the-limits-of-windows-virtual-memory/
[2] https://docs.microsoft.com/zh-cn/windows/desktop/api/processthreadsapi/nf-processthreadsapi-createthread

@rust-highfive
Copy link
Contributor

r? @sfackler

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2018
@upsuper
Copy link
Contributor Author

upsuper commented Jul 30, 2018

I have verified locally with the following program:

use std::thread;
use std::time::Duration;

fn main() {
    let threads: Vec<_> = (0..10).map(|_| {
        thread::Builder::new()
            .stack_size(100 * 1024 * 1024)
            .spawn(|| {
                thread::sleep(Duration::from_secs(3600));
            }).unwrap()
    }).collect();
    for thread in threads {
        thread.join().unwrap();
    }
}

It normally commits ~1GB memory, while building with a toolchain with this change, it only commits ~1MB.

@sfackler
Copy link
Member

sfackler commented Jul 30, 2018

@alexcrichton
Copy link
Member

@bors: r+

This is an awesome find! Sounds like a great idea to me

@bors
Copy link
Collaborator

bors commented Jul 30, 2018

📌 Commit fc8bb9c has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2018
@sfackler sfackler added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 30, 2018
@bors
Copy link
Collaborator

bors commented Aug 2, 2018

⌛ Testing commit fc8bb9c with merge db54765...

bors added a commit that referenced this pull request Aug 2, 2018
Don't commit thread stack on Windows

On Windows, there is a system level resource limitation called commit limit, which is roughly the sum of physical memory + paging files[1]. `CreateThread` by default commits the stack size[2], which unnecessarily takes such resource from the shared limit.

This PR changes it to only reserve the stack size rather than commit it. Reserved memory would only take the address space of the current process until it's actually accessed.

This should make the behavior on Windows match other platforms, and is also a pretty standard practice on Windows nowadays.

[1] https://blogs.technet.microsoft.com/markrussinovich/2008/11/17/pushing-the-limits-of-windows-virtual-memory/
[2] https://docs.microsoft.com/zh-cn/windows/desktop/api/processthreadsapi/nf-processthreadsapi-createthread
@bors
Copy link
Collaborator

bors commented Aug 2, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing db54765 to master...

@bors bors merged commit fc8bb9c into rust-lang:master Aug 2, 2018
@upsuper upsuper deleted the thread-stack-reserve branch August 3, 2018 05:20
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 4, 2018
This updates our windows builders to use a version of rustc that includes
rust-lang/rust#52847 We're not commiting to letting
this change ride the trains at this time.

Differential Revision: https://phabricator.services.mozilla.com/D7663

--HG--
extra : moz-landing-system : lando
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
This updates our windows builders to use a version of rustc that includes
rust-lang/rust#52847 We're not commiting to letting
this change ride the trains at this time.

Differential Revision: https://phabricator.services.mozilla.com/D7663

UltraBlame original commit: 556b2f4cd653aa7642956ba66e60839b4ea9f474
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
This updates our windows builders to use a version of rustc that includes
rust-lang/rust#52847 We're not commiting to letting
this change ride the trains at this time.

Differential Revision: https://phabricator.services.mozilla.com/D7663

UltraBlame original commit: 556b2f4cd653aa7642956ba66e60839b4ea9f474
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
This updates our windows builders to use a version of rustc that includes
rust-lang/rust#52847 We're not commiting to letting
this change ride the trains at this time.

Differential Revision: https://phabricator.services.mozilla.com/D7663

UltraBlame original commit: 556b2f4cd653aa7642956ba66e60839b4ea9f474
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants