1153244 - increase JS helper threads stack space when MOZ_TSAN is enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

TSan enforces a minimum stack size that's just slightly larger than our
default helper stack size.  It does this to store blobs of TSan-specific
data on each thread's stack.  Unfortunately, that means that even though
we'll actually receive a larger stack than we requested, the effective
usable space of that stack is significantly less than what we expect.
To offset TSan stealing our stack space from underneath us, increase the
default when we're running with MOZ_TSAN.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: increase JS helper threads stack space when MOZ_TSAN is enabled

The comment/commit message should explain everything here.  It's not completely
obvious to me why we don't need this for MOZ_ASAN.  The comment makes a stab at
it, but I don't understand why the comments in XPCJSRuntime.cpp about ASan
needing an increased amount of stack space aren't relevant here as well.
Empirically speaking, TSan fails (on modern clang) to run the browser without
this patch, while ASan appears to be OK, so I'm letting reality win this round.

Comment 2

[Nathan Froyd [:froydnj]]

Christian might be interested in this, especially comment 1.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8590856 [details] [diff] [review]
increase JS helper threads stack space when MOZ_TSAN is enabled

Forwarding to bholley to make sure this fits into the general scheme we use for stack sizes which he's reviewed/written in the past.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8590856 [details] [diff] [review]
increase JS helper threads stack space when MOZ_TSAN is enabled

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

Why is this set here rather than with all the other stack limits on the embedding side in XPCJSRuntime.cpp? Seems like this stuff should all be embedding-controlled in general, and we should try to keep it in one place.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) from comment #4)
> Why is this set here rather than with all the other stack limits on the
> embedding side in XPCJSRuntime.cpp? Seems like this stuff should all be
> embedding-controlled in general, and we should try to keep it in one place.

I do not know the answer to that.

It looks like bug 942984 added the bits to explicitly set the size of helper threads.  Quoth bug 942984 comment 11: "This patch just uses 512K as trying to figure out what the 'default' setting should be is super baroque, see the XPCJSRuntime constructor."

But at the same time, it's not obvious to me that the XPCJSRuntime bits should necessarily be used wholesale for the helper threads, and I can imagine that trying to say "half of the XPCJSRuntime value" (or some other fraction) might result in problems here.

Brian, since you added the stack size bits, any thoughts here?

Comment 6

[Brian Hackett [Laid off!]]

The stack size for the helper threads should be independent from the stack size used for the main thread's runtime.  The things the helper threads do don't generally require recursion, except in the JS parser/emitter (which is really a design issue with the parser anyways).  Additionally, the helper threads are used by both the main thread runtime and DOM worker runtimes, which set their stack size in a different place.  It would be fine though to set the JS helper thread stack size somewhere in XPCJSRuntime, rather than in a JS source file.

Comment 7

[Bobby Holley (:bholley)]

Ok. That sounds somewhat preferable, but I'm not going to insist that Nathan do it. Nathan, feel free to flag Brian for review on this patch, since I think he's probably a better reviewer for it than I am.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8590856 [details] [diff] [review]
increase JS helper threads stack space when MOZ_TSAN is enabled

Redirecting review per bholley.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b896382f5368