Closed Bug 664341 Opened 9 years ago Closed 9 years ago

Reduce video thread stack size, on 32-bit linux at least


(Core :: Audio/Video, defect)

Not set





(Reporter: njn, Assigned: cpearce)


(Whiteboard: [MemShrink:P2])


(2 files, 1 obsolete file)

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.
Whiteboard: [MemShrink:P2]
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?
Summary: Reduce thread stack size, on 32-bit linux at least → Reduce video thread stack size, on 32-bit linux at least
(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.
Assignee: nobody → chris
(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.
Attachment #541516 - Flags: review?(benjamin)
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
Attachment #541516 - Flags: review?(benjamin) → review+
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.
Attachment #548350 - Flags: review+
Attachment #541516 - Attachment is obsolete: true
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.
Attachment #548632 - Flags: review?(kinetik)
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)

Attachment #548632 - Flags: review?(kinetik) → review+
Nice catch, thanks. MEDIA_THREAD_STACK_SIZE should be nsIThreadManager::DEFAULT_STACK_SIZE too, not 0.
Ooops, XP_LINUX isn't actually defined centrally:
Have to go with using defined(LINUX)
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.