1443932 - reduce the stack space required for image decoder threads

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Nathan Froyd [:froydnj]]

We don't need to use the platform's default, which can be quite large.  We
ought to be able to get by with soemthing much smaller--everything that uses
SharedThreadPool, for example, already does.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add nsIThreadManager::kThreadPoolStackSize

A lot of our thread pools use the default stack size for the platform
they're on, which can be rather large (8MB, usually, on Linux and OS X)
and is probably too much for the typical thread in the thread pool
regardless.  SharedThreadPool already has some logic for selecting a
reasonable stack size for worker threads; let's move that logic to
nsIThreadManager so that logic (and constant) can be shared more
broadly.  (That we already have a couple of instances of
SharedThreadPool usage solely for this constant suggests that it is a
concept that should be available in a more central location.)

I considered putting this constant in nsIThreadPool, but decided it went better
next to DEFAULT_STACK_SIZE.

I realize that I'm using something named kThreadPoolStackSize as the stack size
for non-thread pool threads; perhaps we should add something like
kReasonableStackSize that's merely a copy of kThreadPoolStackSize so the naming
lines up better?

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - lower the default stack size for image decoder threads

These threads should not have deep stacks, and as we can have a number
of them running simultaneously, it's beneficial to set the stack size to
something reasonably low.

Comment 3

[Eric Rahm [:erahm]]

Comment on attachment 8956965 [details] [diff] [review]
part 1 - add nsIThreadManager::kThreadPoolStackSize

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

r=me, I'm fine with just using kThreadPoolStackSize. I guess we could call it kHelperStackSize or something to imply it's suitable for things with shallow stacks.

::: xpcom/threads/nsIThreadManager.idl
@@ +45,5 @@
> +   * a thread pool worker thread.
> +   */
> +#if defined(MOZ_ASAN) || defined(MOZ_TSAN)
> +  // Use the system default in ASAN builds, because the default is assumed
> +  // to be larger than the size we wwant to use and is hopefully sufficient

nit: wwant

@@ +48,5 @@
> +  // Use the system default in ASAN builds, because the default is assumed
> +  // to be larger than the size we wwant to use and is hopefully sufficient
> +  // for ASAN.
> +  static const uint32_t kThreadPoolStackSize = DEFAULT_STACK_SIZE;
> +#elif defined(XP_WIN) || defined(XP_MACOSX) || defined(LINUX)

Is android covered by linux?

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8956966 [details] [diff] [review]
part 2 - lower the default stack size for image decoder threads

Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e98924c2ee35
part 1 - add nsIThreadManager::kThreadPoolStackSize; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/18336fd064bd
part 2 - lower the default stack size for image decoder threads; r=tnikkel

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e98924c2ee35
https://hg.mozilla.org/mozilla-central/rev/18336fd064bd