Open Bug 1257522 Opened 4 years ago Updated 10 months ago

Stop using default stack size for background threads

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

People

(Reporter: jandem, Unassigned)

Details

Attachments

(3 files)

We'd like to bump the main thread's stack size on Windows (and maybe Linux too), see bug 1257234.

Most of our background threads don't need such large stack sizes though, so we should pass an explicit stack size to not waste address space.
Attached patch Part 1 - NSPRSplinter Review
Here's a patch for the NSPR repo to use a default stack size of 512 KB for new threads on Windows, if we don't get an explicit stack size passed in.

I used VMMap to verify this works.

The alternative is to use 1 MB, the default stack size on Windows, but AFAICS all threads that potentially need large stacks (web workers, JS helper threads, etc) already pass in an explicit stack size. 512 KB is still very large and on 32-bit this is an easy address space win of at least 5-10 MB.
Attachment #8732306 - Flags: review?(ted)
This might also work more nicely together with jemalloc and the GC, both of which use 1MB-aligned 1MB chunks, unless thread stacks are also aligned in the same way. With a 1MB reservation, a thread's stack would be pretty likely to straddle two chunks - it still might with 512kB, but it's more likely to be fully inside a 1MB-aligned region, potentially freeing up more address space for jemalloc and the GC.
This uses a 512 KB stack for WorkerThreadWin32 and 256 KB for the ReadbackManagerD3D11 thread (256 KB because this thread looks very simple and it shouldn't recurse AFAICS).
Attachment #8732321 - Flags: review?(dvander)
Comment on attachment 8732321 [details] [diff] [review]
Part 2 - gfx threads

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

Seems fine, I doubt D3D11 is going to use 256KB of stack space.
Attachment #8732321 - Flags: review?(dvander) → review+
This changes a bunch of places where we call _beginthreadex or CreateThread directly. 256-512 KB should be more than enough.
Attachment #8732332 - Flags: review?(ted)
Comment on attachment 8732332 [details] [diff] [review]
Part 3 - More threads

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

It seems like it'd be nicer to have this as a constant somewhere.
Attachment #8732332 - Flags: review?(ted) → review+
Comment on attachment 8732306 [details] [diff] [review]
Part 1 - NSPR

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

I'm a little bit worried that this could have unexpected consequences for consumers, what would you think about simply making this:
#ifndef _MD_DEFAULT_STACK_SIZE
#define _MD_DEFAULT_STACK_SIZE
#endif

and then in configure adding -D_MD_DEFAULT_STACK_SIZE=524288 for Windows?

(Also we only use the win95 backend, FWIW.)
Attachment #8732306 - Flags: review?(ted) → review-
FWIW, there are also a few places where _beginthread (not _beginthreadex) is called directly:
https://dxr.mozilla.org/mozilla-central/search?q=%22_beginthread(%22&redirect=false&case=false

Most of them are for OS2 though. These two are the exceptions as far as I can tell:
https://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/testplugin/nptest.cpp#3257
https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/third_party/libevent/event_iocp.c#201

_beginthread doesn't have a flags parameter, so you'd probably need to change them to use _beginthreadex instead.
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #8)
> Most of them are for OS2 though.

We should remove those!

> https://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/testplugin/
> nptest.cpp#3257

This is test-only code (for the test plugin).

> https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/third_party/
> libevent/event_iocp.c#201

This code doesn't get built on Windows:
https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/moz.build#48
We didn't actually need this for bug 1257234. This still might be nice to fix at some point but I don't have time for it atm.
Assignee: jdemooij → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.