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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Nicholas Nethercote [inactive]]

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 (Exited; not receiving bugmail, old account, do not use)]

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

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)]

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 [inactive]]

(In reply to comment #1)
> 1 MB * 3 threads * N audio/video elements

Bug 592833 will avoid the "* 3" term.

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

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

Comment 9

[Julian Seward [:jseward]]

(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 (Not reading bugmail)]]

Attachment: 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 (Not reading bugmail)]]

bsmedberg: review ping?

Comment 12

[Benjamin Smedberg]

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 (Not reading bugmail)]]

Attachment: 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 (Not reading bugmail)]]

Attachment: 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]]

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 (Not reading bugmail)]]

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

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 19

[Marco Bonardo [:mak]]

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.