-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
Conversation
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
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. |
It looks like adding this flag is a pretty unambiguously the right thing to do: https://bugs.chromium.org/p/webrtc/issues/detail?id=2902 cc @alexcrichton LGTY? |
@bors: r+ This is an awesome find! Sounds like a great idea to me |
📌 Commit fc8bb9c has been approved by |
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
☀️ Test successful - status-appveyor, status-travis |
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
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
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
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
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