1256306 - Bump Windows stack limit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Our stack quota on Windows is 900 KB. From that we subtract 190 KB (kTrustedScriptBuffer + kSystemCodeBuffer) so we're left with 710 KB for untrusted code. Unfortunately Win64 PGO builds can pretty easily hit this limit.

The default Windows stack size is 1 MB, so I think we can bump our stack quota to 980 KB or so.

At some point we should probably ask the linker to give us a bigger stack, https://msdn.microsoft.com/en-us/library/8cxs58a6.aspx

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

bholley, I know you're busy but not sure if someone else can review this.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8730198 [details] [diff] [review]
Patch

Review of attachment 8730198 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +3409,5 @@
>      //
>      // Linux 32-bit Debug: 2MB stack, 426 stack frames => ~4.8k per stack frame
>      // Linux 64-bit Debug: 4MB stack, 455 stack frames => ~9.0k per stack frame
>      //
> +    // Windows (Opt+Debug): 980K stack, 235 stack frames => ~4.3k per stack frame

Did you actually remeasure this? The point of this comment is to record the measurements of stack frame size, which is used to compute the trusted buffer. Changing the stack size here without remeasuring the number of stack frames it allowed is meaningless, so please revert that part.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a6f90eef68e

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (busy) from comment #2)
> Did you actually remeasure this?

Oops, I misread/misunderstood that comment. Reverted.

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to Jan de Mooij [:jandem] from comment #0)
> At some point we should probably ask the linker to give us a bigger stack,
> https://msdn.microsoft.com/en-us/library/8cxs58a6.aspx

The only worry about that is the memory usage of the sum total of our threads. I believe we create some of our threads with a smaller stack size, so it may not be an enormous problem. It's certainly easy enough to test...

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> The only worry about that is the memory usage of the sum total of our
> threads. I believe we create some of our threads with a smaller stack size,
> so it may not be an enormous problem. It's certainly easy enough to test...

True, we could audit all places where we create threads...

I think it'd be really nice to have a larger stack - stack overflow errors usually show up out of the blue (PGO, random code changes, etc), don't happen on all platforms, and usually make websites completely unusable.

If we do this, we should increase the stack size on Linux as well, to avoid Linux-only stack overflow issues. OS X already has an 8 MB stack.

Comment 7

[Jan de Mooij [:jandem]]

Filed bug 1257234 to increase our stack size.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2a6f90eef68e