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

RESOLVED FIXED in mozilla8

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: njn, Assigned: cpearce)

Tracking

unspecified
mozilla8
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
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.
(Reporter)

Comment 4

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

Bug 592833 will avoid the "* 3" term.
(Reporter)

Comment 5

8 years ago
Just to clarify:  we'd only do this for audio/video threads, right?
(Assignee)

Comment 6

8 years ago
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)

Updated

8 years ago
Assignee: nobody → chris
Status: NEW → ASSIGNED
(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.
(Assignee)

Comment 10

8 years ago
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)
(Assignee)

Comment 11

8 years ago
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+
(Assignee)

Comment 13

8 years ago
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+
(Assignee)

Updated

8 years ago
Attachment #541516 - Attachment is obsolete: true
(Assignee)

Comment 14

8 years ago
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)

XP_LINUX?
Attachment #548632 - Flags: review?(kinetik) → review+
(Assignee)

Comment 16

8 years ago
Nice catch, thanks. MEDIA_THREAD_STACK_SIZE should be nsIThreadManager::DEFAULT_STACK_SIZE too, not 0.
(Assignee)

Comment 17

8 years ago
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)
(Assignee)

Comment 18

8 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/c56067ea7988
http://hg.mozilla.org/integration/mozilla-inbound/rev/685b3762558c
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.