Last Comment Bug 664341 - Reduce video thread stack size, on 32-bit linux at least
: Reduce video thread stack size, on 32-bit linux at least
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: x86 Linux
: -- normal with 2 votes (vote)
: mozilla8
Assigned To: Chris Pearce (:cpearce)
:
: Maire Reavy [:mreavy]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-14 17:15 PDT by Nicholas Nethercote [:njn]
Modified: 2011-07-27 18:05 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch: Add API to specify amount reserved for thread stack size (7.87 KB, patch)
2011-06-23 15:06 PDT, Chris Pearce (:cpearce)
benjamin: review+
Details | Diff | Splinter Review
Patch v2: Add API to specify amount reserved for thread stack size (7.99 KB, patch)
2011-07-25 18:08 PDT, Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review
Patch: Specify media threads' stack size (4.33 KB, patch)
2011-07-26 16:39 PDT, Chris Pearce (:cpearce)
kinetik: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-06-14 17:15:37 PDT
From bug 592833 comment 9:

> Reducing the thread stack size is a good idea, as we reserve 10MB per thread
> stack on Linux, which kills us on 32bit Linux. IIRC, we use 1MB thread stacks
> on Windows and OSX, so the problem isn't so pronounced there, but we can
> probably still make some easy gains by reducing stack size there anyway.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-14 17:19:22 PDT
How deep do these threads get?  If they don't use much of the stack we should shrink windows too ... 1 MB * 3 threads * N audio/video elements is a fair amount of virtual address space.  I've seen minidumps with 100s of AV threads.
Comment 2 Matthew Gregan [:kinetik] 2011-06-14 17:26:45 PDT
Linux is 8MB by default, not 10MB.  OS X uses 512kB by default and we haven't seen stack exhaustion there, so we could probably reduce Win32 thread stack sizes safely.

Fixing this will require some new API for nsIThread/NS_NewThread.
Comment 3 Timothy B. Terriberry (:derf) 2011-06-14 17:32:20 PDT
libvorbis uses alloca, potentially of several times the largest block size (which is 8192). libtheora requires less than 2kB of stack. I'd need to double-check for libvpx, but I would expect this to be similarly small.
Comment 4 Nicholas Nethercote [:njn] 2011-06-14 17:32:52 PDT
(In reply to comment #1)
> 1 MB * 3 threads * N audio/video elements

Bug 592833 will avoid the "* 3" term.
Comment 5 Nicholas Nethercote [:njn] 2011-06-14 17:34:05 PDT
Just to clarify:  we'd only do this for audio/video threads, right?
Comment 6 Chris Pearce (:cpearce) 2011-06-14 17:36:56 PDT
Why does Linux need such large stack sizes compared to other platforms in general? bent/cjones?
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-14 17:38:01 PDT
(In reply to comment #6)
> Why does Linux need such large stack sizes compared to other platforms in
> general? bent/cjones?

I don't think it needs them ... that's just the default.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-14 18:39:58 PDT
Right.  Threads' stack memory isn't committed on alloc, so a large default usually makes sense.
Comment 9 Julian Seward [:jseward] 2011-06-20 04:37:42 PDT
(In reply to comment #8)
8M seems excessive though.  Perhaps we should institute a 1M limit
for all stacks on all platforms, and try to stay within (eg) 256k
so as to leave a good safety margin.  If the Linux kernel can get
by in an 8K stack, 8M seems wildly extravagant.

gcc-4.6.0 at least has a 

  -Wframe-larger-than=<number> Warn if a function's stack frame requires more
                              than <number> bytes

which might be useful.
Comment 10 Chris Pearce (:cpearce) 2011-06-23 15:06:08 PDT
Created attachment 541516 [details] [diff] [review]
Patch: Add API to specify amount reserved for thread stack size

Add an API nsIThreadManager and nsThread to specify the size of thread stacks. This just passes the value through to PR_CreateThread(), which already allows you to set stack size.

I have another patch to set the size of thread stacks in the <video>, but I'll wait until this patch gets r+ before posting that.
Comment 11 Chris Pearce (:cpearce) 2011-07-18 16:14:37 PDT
bsmedberg: review ping?
Comment 12 Benjamin Smedberg [:bsmedberg] 2011-07-20 13:01:07 PDT
Comment on attachment 541516 [details] [diff] [review]
Patch: Add API to specify amount reserved for thread stack size

JS shouldn't be using this API, but you will at least need to fix up test_bug608142.js

I'd really like a constant for the stack size, instead of hardcoded 0 (e.g. const unsigned int DEFAULT_STACK_SIZE = 0; in nsIThreadManager)

And can you just add a default argument to the existing NS_NewThread instead of declaring another version?

r=me with at least the constant
Comment 13 Chris Pearce (:cpearce) 2011-07-25 18:08:56 PDT
Created attachment 548350 [details] [diff] [review]
Patch v2: Add API to specify amount reserved for thread stack size

With bmsedberg's review comments addressed. Carrying forward r=bsmedberg.

I made the stack size argument in nsIThreadManager [optional], so that existing js which uses this API (in our tests) don't need to change. I also added the constant DEFAULT_STACK_SIZE in nsIThreadManager as requested.
Comment 14 Chris Pearce (:cpearce) 2011-07-26 16:39:32 PDT
Created attachment 548632 [details] [diff] [review]
Patch: Specify media threads' stack size

Specify the media decoder's threads' stacks to reserve 128KB. I found that on my x64 Linux box I needed at least 98KB thread stacks when running mochitests with a debug build (opt builds may use less), so I rounded stack size up to 128KB to give us some wiggle room.
Comment 15 Matthew Gregan [:kinetik] 2011-07-26 16:48:43 PDT
Comment on attachment 548632 [details] [diff] [review]
Patch: Specify media threads' stack size

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

::: content/media/VideoUtils.h
@@ +157,5 @@
>  // before being used!
>  void ScaleDisplayByAspectRatio(nsIntSize& aDisplay, float aAspectRatio);
>  
> +// The amount of virtual memory reserved for thread stacks.
> +#if defined(XP_WIN) || defined(XP_MACOSX) || defined(LINUX)

XP_LINUX?
Comment 16 Chris Pearce (:cpearce) 2011-07-26 17:15:50 PDT
Nice catch, thanks. MEDIA_THREAD_STACK_SIZE should be nsIThreadManager::DEFAULT_STACK_SIZE too, not 0.
Comment 17 Chris Pearce (:cpearce) 2011-07-26 17:20:10 PDT
Ooops, XP_LINUX isn't actually defined centrally:
http://mxr.mozilla.org/mozilla-central/search?string=XP_LINUX
Have to go with using defined(LINUX)
Comment 19 Marco Bonardo [::mak] 2011-07-27 03:45:01 PDT
http://hg.mozilla.org/mozilla-central/rev/c56067ea7988
http://hg.mozilla.org/mozilla-central/rev/685b3762558c

please don't mark as fixed till bugs are merged to central.

Note You need to log in before you can comment on or make changes to this bug.