1476828 - Decrease the default thread size on (at least) Linux

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Kris Maglione [:kmag]]

Our default thread size on Linux currently falls back to the system default, which is 2MB (unless `unlimit -s` is something other than unlimited).

The problem with this default is that 2MB also happens to be the size of VM huge pages, which means that the kernel sometimes allocates an entire huge page for a thread stack the first time it's touched. We don't generally expect any of our thread stacks to get anywhere near as large as 2MB. But even if we did, the random allocation generally doesn't land on our largest threads.

We should set our default thread size to something closer to 256K, which is our default for thread pools. I don't see any reason not to do that on all platforms.

And, on Linux, we should probably adjust the sizes of our DOM Worker stacks to be slightly smaller than 2MB, so we don't risk allocating huge pages for them, either.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1476828: Part 1 - Reduce the default thread manager thread stack size.

Review commit: https://reviewboard.mozilla.org/r/257990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257990/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1476828: Part 2 - Slightly decrease the DOM Worker thread stack size.

Review commit: https://reviewboard.mozilla.org/r/257992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257992/

Comment 3

[Eric Rahm [:erahm]]

Comment on attachment 8993190 [details]
Bug 1476828: Part 1 - Reduce the default thread manager thread stack size.

https://reviewboard.mozilla.org/r/257990/#review265172

::: xpcom/threads/nsIThreadManager.idl:32
(Diff revision 1)
>  [scriptable, uuid(1be89eca-e2f7-453b-8d38-c11ba247f6f3)]
>  interface nsIThreadManager : nsISupports
>  {
>    /**
>     * Default number of bytes reserved for a thread's stack, if no stack size
>     * is specified in newThread(). 0 means use platform default.

We should move "0 means platform default" to the function docs I guess.

::: xpcom/threads/nsIThreadManager.idl:50
(Diff revision 1)
> +   * ASan and TSan builds, however, often need a bit more, so give them a full
> +   * megabyte.
>     */
> +%{C++
>  #if defined(MOZ_ASAN) || defined(MOZ_TSAN)
> -  // Use the system default in ASAN builds, because the default is assumed
> +  static constexpr size_t DEFAULT_STACK_SIZE = 1024 * 1024;

Leaving this system default probably makes sense (we shouldn't be measuring memory on those targets). If you still want to explicitly set it can you do several asan runs on each platform before landing?

nit: Lets be consistent and use uint32_t

::: xpcom/threads/nsIThreadManager.idl:52
(Diff revision 1)
> -  // for ASAN.
> -  static const uint32_t kThreadPoolStackSize = DEFAULT_STACK_SIZE;
> -#elif defined(XP_WIN) || defined(XP_MACOSX) || defined(LINUX)
> -  static const uint32_t kThreadPoolStackSize = (256 * 1024);
>  #else
> -  // All other platforms use their system default.
> +  static constexpr size_t DEFAULT_STACK_SIZE = 256 * 1024;

256KiB is a lot smaller. We should defintely do a fair amount of try runs on each platform and before landing coordinate with the stability folks to let them know we might see stack overrun related crashes.

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8993191 [details]
Bug 1476828: Part 2 - Slightly decrease the DOM Worker thread stack size.

https://reviewboard.mozilla.org/r/257992/#review265174

Nice find, I wonder if we should audit for any other explicit large thread stack sizes.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8993190 [details]
Bug 1476828: Part 1 - Reduce the default thread manager thread stack size.

https://reviewboard.mozilla.org/r/257990/#review265182

You may want to announce this on dev-platform, so if there is bustage from the decreased stack size, people have this patch in mind as a possible cause.  I know I reviewed a patch for the media thread pool that bumped the stack limits up because av1 is apparently stack-hungry.

::: xpcom/threads/nsIThreadManager.idl:43
(Diff revision 1)
> +   * if the stack size is unlimited, we fall back to 2MB. This causes particular
> +   * problems on Linux, which allocates 2MB huge VM pages, and will often
> +   * immediately allocate them for any stacks which are 2MB or larger.
> +   *
> +   * The default on Windows is 1MB, which is a little more reasonable. But the
> +   * vast marjority of our threads don't need anywhere near that much space.

Nit: "majority"

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce1767f52fe6312a247702c5a215ec9429c675e7
Bug 1476828: Part 1 - Reduce the default thread manager thread stack size. r=erahm f=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/779936e3d0af68a3d6b03d42f41452c893a978f5
Bug 1476828: Part 2 - Slightly decrease the DOM Worker thread stack size. r=erahm

Comment 7

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/ce1767f52fe6
https://hg.mozilla.org/mozilla-central/rev/779936e3d0af

Comment 8

[Ionuț Goldan [:igoldan]]

Big AWSY wins on Linux 64:

== Change summary for alert #14520 (as of Tue, 24 Jul 2018 03:30:30 GMT) ==

Improvements:

 12%  Base Content Resident Unique Memory linux64 opt stylo     28,016,384.00 -> 24,760,490.67
 10%  Base Content Resident Unique Memory linux64-qr opt stylo  27,637,162.67 -> 24,963,413.33

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14520