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.

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.

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.

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.

(In reply to comment #1) > 1 MB * 3 threads * N audio/video elements Bug 592833 will avoid the "* 3" term.

Just to clarify: we'd only do this for audio/video threads, right?

Why does Linux need such large stack sizes compared to other platforms in general? bent/cjones?

(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.

Right. Threads' stack memory isn't committed on alloc, so a large default usually makes sense.

(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.

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.

bsmedberg: review ping?

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

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.

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 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?

Nice catch, thanks. MEDIA_THREAD_STACK_SIZE should be nsIThreadManager::DEFAULT_STACK_SIZE too, not 0.

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)

http://hg.mozilla.org/integration/mozilla-inbound/rev/c56067ea7988 http://hg.mozilla.org/integration/mozilla-inbound/rev/685b3762558c

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.