reduce the stack space required for image decoder threads

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

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.
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?
Attachment #8956965 - Flags: review?(erahm)
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.
Attachment #8956966 - Flags: review?(tnikkel)
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?
Attachment #8956965 - Flags: review?(erahm) → review+
Comment on attachment 8956966 [details] [diff] [review]
part 2 - lower the default stack size for image decoder threads

Thanks!
Attachment #8956966 - Flags: review?(tnikkel) → review+
Pushed by nfroyd@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/e98924c2ee35
https://hg.mozilla.org/mozilla-central/rev/18336fd064bd
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.