1479250 - Style threads are allocated with eagerly committed stacks on Windows

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Kris Maglione [:kmag]]

This also applies to other Rust threads, but Stylo threads are the biggest chunk of that.

All of the non-Rust threads we create on Windows use the STACK_SIZE_PARAM_IS_A_RESERVATION flag, which causes us to reserve stack space, but not commit it until it's used. std::thread doesn't do that, which means that all of the thread stack space is eagerly committed, and immediately consumes resources.

With 6 Stylo threads at 256KiB each, that's 1.5MiB per process.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Would this become irrelevant if we can fix bug 1475091?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Kris Maglione [:kmag] from comment #0)
> This also applies to other Rust threads, but Stylo threads are the biggest
> chunk of that.
> 
> All of the non-Rust threads we create on Windows use the
> STACK_SIZE_PARAM_IS_A_RESERVATION flag, which causes us to reserve stack
> space, but not commit it until it's used. std::thread doesn't do that, which
> means that all of the thread stack space is eagerly committed, and
> immediately consumes resources.
> 
> With 6 Stylo threads at 256KiB each, that's 1.5MiB per process.

Do you think just modifying libstd to pass the flag by default is feasible / desirable? Or should we try to add an API to configure it the same way as thread names can be configured?

Comment 3

[Kris Maglione [:kmag]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)
> Do you think just modifying libstd to pass the flag by default is feasible /
> desirable? Or should we try to add an API to configure it the same way as
> thread names can be configured?

I think having libstd pass the flag by default probably makes the most sense. That would make the behavior on Windows match other platforms, and is pretty standard practice for threading libraries on Windows anyway.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Discussed with emilio on IRC, I'm going to send a patch to Rust to fix this.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Submitted rust-lang/rust#52847 for the change.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

The Rust PR has been merged, should be in 1.30.